[ShrinkWrap][NFC] Test with load from constant pool preventing shrink #162476
Conversation
wrapping Shrink wrapping treats a load from constant pool as a stack access. This is not correct. Constants are basically stored in read only section AFAIU. This prevents shrink wrapping from kicking in. (Related to PR llvm#160257. PR llvm#160257 will be closed.)
sushgokh
requested review from
davemgreen,
madhur13490,
qcolombet and
sjoerdmeijer
|
@llvm/pr-subscribers-backend-aarch64 Author: Sushant Gokhale (sushgokh) Changeswrapping Shrink wrapping treats a load from constant pool as a stack access. This is not correct. Constants are basically stored in read only section AFAIU. This prevents shrink wrapping from kicking in. (Related to PR #160257. PR #160257 will be closed.) Full diff: https://github.com/llvm/llvm-project/pull/162476.diff 1 Files Affected:
diff --git a/llvm/test/CodeGen/AArch64/shrink-wrap-const-pool-access.mir b/llvm/test/CodeGen/AArch64/shrink-wrap-const-pool-access.mir
new file mode 100644
index 0000000000000..ff652a58cae88
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/shrink-wrap-const-pool-access.mir
@@ -0,0 +1,71 @@
+--- |
+ ; RUN: llc -x=mir -simplify-mir -run-pass=shrink-wrap -o - %s | FileCheck %s
+ ; CHECK-NOT: savePoint
+ ; CHECK-NOT: restorePoint
+
+ declare double @foo()
+
+ define double @shrink_wrap_load_from_const_pool(double %q) {
+ entry:
+ %0 = fcmp oeq double %q, 3.125500e+02
+ br i1 %0, label %common.ret, label %if.else
+
+ common.ret: ; preds = %if.else, %entry, %exit1
+ %common.ret.op = phi double [ %3, %exit1 ], [ 0.000000e+00, %entry ], [ 0.000000e+00, %if.else ]
+ ret double %common.ret.op
+
+ if.else: ; preds = %entry
+ %1 = call double @foo()
+ %2 = fcmp oeq double %1, 0.000000e+00
+ br i1 %2, label %exit1, label %common.ret
+
+ exit1: ; preds = %if.else
+ %3 = call double @foo()
+ br label %common.ret
+ }
+...
+---
+name: shrink_wrap_load_from_const_pool
+tracksRegLiveness: true
+constants:
+ - id: 0
+ value: 'double 3.125500e+02'
+ alignment: 8
+body: |
+ bb.0.entry:
+ successors: %bb.4(0x50000000), %bb.2(0x30000000)
+ liveins: $d0
+
+ renamable $d1 = COPY $d0
+ renamable $x8 = ADRP target-flags(aarch64-page) %const.0
+ renamable $d2 = LDRDui killed renamable $x8, target-flags(aarch64-pageoff, aarch64-nc) %const.0 :: (load (s64) from constant-pool)
+ renamable $d0 = FMOVD0
+ nofpexcept FCMPDrr killed renamable $d1, killed renamable $d2, implicit-def $nzcv, implicit $fpcr
+ Bcc 1, %bb.2, implicit killed $nzcv
+
+ bb.4:
+ liveins: $d0
+
+ bb.1.common.ret:
+ liveins: $d0
+
+ RET_ReallyLR implicit $d0
+
+ bb.2.if.else:
+ successors: %bb.3(0x50000000), %bb.1(0x30000000)
+
+ ADJCALLSTACKDOWN 0, 0, implicit-def dead $sp, implicit $sp
+ BL @foo, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp, implicit-def $d0
+ ADJCALLSTACKUP 0, 0, implicit-def dead $sp, implicit $sp
+ renamable $d1 = COPY $d0
+ renamable $d0 = FMOVD0
+ nofpexcept FCMPDri killed renamable $d1, implicit-def $nzcv, implicit $fpcr
+ Bcc 1, %bb.1, implicit killed $nzcv
+ B %bb.3
+
+ bb.3.exit1:
+ ADJCALLSTACKDOWN 0, 0, implicit-def dead $sp, implicit $sp
+ BL @foo, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp, implicit-def $d0
+ ADJCALLSTACKUP 0, 0, implicit-def dead $sp, implicit $sp
+ B %bb.1
+...
|
| @@ -0,0 +1,71 @@ | |||
| --- | | |||
There was a problem hiding this comment.
Do you need the LLVM section?
There was a problem hiding this comment.
I feel it can just be removed
There was a problem hiding this comment.
IR section is not needed. But, for someone like me who is not that great with codegen, I feel LLVM IR gives good sense of how the mir code was derived in the first place.
If you still insist, will remove it.
| --- | | ||
| ; RUN: llc -x=mir -simplify-mir -run-pass=shrink-wrap -o - %s | FileCheck %s | ||
| ; CHECK-NOT: savePoint | ||
| ; CHECK-NOT: restorePoint |
There was a problem hiding this comment.
Could you add a comment on what this is testing and what we are checking here?
| value: 'double 3.125500e+02' | ||
| alignment: 8 | ||
| body: | | ||
| bb.0.entry: |
There was a problem hiding this comment.
Get rid of the .llvmir_names in the basic block names so that you don't need the LLVM IR section.
There was a problem hiding this comment.
keeping the IR names for the reason mentioned above.
Additionally, if the BB name is just like bb.0 or bb.2, MBB->getName() prints blank and this makes debugging difficult. Having names eases this.
| } | ||
| ... | ||
| # Following code has a load from constant pool. Accessing constant pool must not | ||
| # be considered as a stack access and hence, shrink wrapping must happen. |
There was a problem hiding this comment.
Please put the check lines next to this comment (you just need to change the prefix to # if you move them here)
Also, this doesn't tell me why the check lines are what we want to check to match what you mentioned.
At the very least, I would expect that we check that the constant pool is used.
There was a problem hiding this comment.
Also, this doesn't tell me why the check lines are what we want to check to match what you mentioned.
Have prepended the comment with 'FIXME' so that it better explains the intent. Does that look good?
| # must not be considered as a stack access and hence, shrink wrapping must | ||
| # happen. | ||
| # CHECK-NOT: savePoint | ||
| # CHECK-NOT: restorePoint |
There was a problem hiding this comment.
Please add a CHECK line that match on the constant pool load.
The CHECK-NOT line still don't tell me why this is testing that shrink wrapping happens/doesn't happen.
I don't remember the implementation details, but assuming that when shrink wrapping happens we create a savePoint or restorePoint basic block, just check for that and say it here.
There was a problem hiding this comment.
Just to give some context on why I am doing this:
Right now, shrink wrapping is not happening due to this constant pool load. So, by default, it considers entry/exit of the function as save/restore point and it does not emit any information on savePoint/restorePoint BB. e.g. for the concerned test,
...
---
name: shrink_wrap_load_from_const_pool
alignment: 1
tracksRegLiveness: true
noPhis: true
isSSA: true
noVRegs: true
hasFakeUses: false
frameInfo:
maxAlignment: 1
maxCallFrameSize: 0
constants:
- id: 0
value: 'double 3.125500e+02'
alignment: 8
machineFunctionInfo: {}
body: |
bb.0.entry:
Hence, the CHECK-NOT.
Now, once I have a patch to address this issue, we will have savePoint/restorePoint BBs emitted by the pass as you say e.g.
; CHECK: name: compiler_pop_stack
; CHECK: frameInfo:
; CHECK: savePoint:
; CHECK-NEXT: - point: '%bb.1'
; CHECK: restorePoint:
; CHECK-NEXT: - point: '%bb.7'
Is there still any correction needed? If yes, I am not able to understand what's needed. Maybe you can give example.
There was a problem hiding this comment.
Please add a CHECK line that match on the constant pool load.
I hope my above reasoning makes sense and thats the reason why I didnt add CHECK for this(anyway this CHECK line is not going to change even after Shrink wrapping)
|
Hello. Do you have the shrink wrapping patch yet that uses this? |
yes, the patch should be just one liner change I believe(have tried the patch with the test case). I am just waiting for someone to approve this test case. |
|
You can put the patch up with both the test and the fix. It will help show that the test is decent. |
|
Up to you. Sometimes it is nice to have the tests and the change separately in the git history. Otherwise you can squash them in #164393. |
sushgokh
deleted the
features/sgokhale/constpoolaccess-does-not-access-stack
branch
As far as I understand, constant pool access does not access stack and accesses read-only memory. This patch considers constant pool access as non-stack access allowing shrink wrapping to happen in the concerned test. We should be seeing perf improvement with povray benchmark from SPEC17(around 12% with -flto -Ofast) after this patch. An NFC PR #162476 already exists to upload the test before the patch but approval has got delayed. So, as @davemgreen suggested in that PR, I have uploaded the test and patch in this single PR to show how test looks like.
4 participants
wrapping
Shrink wrapping treats a load from constant pool as a stack access. This is not correct. Constants are basically stored in read only section AFAIU. This prevents shrink wrapping from kicking in.
(Related to PR #160257. PR #160257 will be closed.)