Skip to content

[ShrinkWrap][NFC] Test with load from constant pool preventing shrink #162476

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 22, 2025
Merged

[ShrinkWrap][NFC] Test with load from constant pool preventing shrink #162476

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ab975c2
[ShrinkWrap][NFC] Test with load from constant pool preventing shrink
sushgokh Oct 8, 2025
a21d438
address comments
sushgokh Oct 9, 2025
d240a88
address comments
sushgokh Oct 14, 2025
7cb9f1b
Test was failing in pre-CI checks. Adding -mtriple option.
sushgokh Oct 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions llvm/test/CodeGen/AArch64/shrink-wrap-const-pool-access.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
# RUN: llc -mtriple=aarch64 -simplify-mir -run-pass=shrink-wrap -o - %s | FileCheck %s
--- |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the LLVM section?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it can just be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
}
...
# FIXME: 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.
# CHECK-LABEL:name: shrink_wrap_load_from_const_pool
# CHECK-NOT: savePoint
# CHECK-NOT: restorePoint
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

---
name: shrink_wrap_load_from_const_pool
tracksRegLiveness: true
constants:
- id: 0
value: 'double 3.125500e+02'
alignment: 8
body: |
bb.0.entry:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get rid of the .llvmir_names in the basic block names so that you don't need the LLVM IR section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
...