Skip to content

[CodeGen] Correctly handle non-standard cases in RemoveLoadsIntoFakeUses #111551

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 10 commits into from
Jan 28, 2025
Merged

[CodeGen] Correctly handle non-standard cases in RemoveLoadsIntoFakeUses #111551

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3ce74d6
[CodeGen] Correctly handle non-standard cases in RemoveLoadsIntoFakeUses
SLTozer Oct 8, 2024
fd2c94e
Remove assertions, handle 0-op FAKE_USES, use RegUnits
SLTozer Oct 8, 2024
0a8b5a6
Use a SmallVector of fake uses instead of a SmallDenseMap
SLTozer Oct 14, 2024
c9d1b6e
Use 'readsRegister'
SLTozer Oct 15, 2024
52b95c7
Update test to check full output
SLTozer Dec 4, 2024
6aadd23
Turn off pass when using varloc-based LDV
SLTozer Dec 5, 2024
a5fd7ee
Remove unnecessary isReg check
SLTozer Dec 13, 2024
0d4a75b
Fix check for instr-ref enabled, update test to check disabled case
SLTozer Jan 24, 2025
026fc06
Use instr-ref bit instead of high-level function
SLTozer Jan 28, 2025
92c0631
Update test to be more update_test_checks-friendly
SLTozer Jan 28, 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
2 changes: 1 addition & 1 deletion llvm/lib/CodeGen/RemoveLoadsIntoFakeUses.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ INITIALIZE_PASS_END(RemoveLoadsIntoFakeUses, DEBUG_TYPE,
bool RemoveLoadsIntoFakeUses::runOnMachineFunction(MachineFunction &MF) {
// Skip this pass if we would use VarLoc-based LDV, as there may be DBG_VALUE
// instructions of the restored values that would become invalid.
if (debuginfoShouldUseDebugInstrRef(MF.getTarget().getTargetTriple()))
if (!MF.shouldUseDebugInstrRef())
Copy link
Member

@jmorse jmorse Jan 28, 2025

Choose a reason for hiding this comment

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

I think this should call useDebugInstrRef, which queries the bit of state that MachineFunction carries around with it. IIRC, the "should" method is testing which mode should be used given the optimisation configuration, while the bit records the actual truth of whether instr-ref is being used or not. This allows for (semi-legitimate) situations where some MIR that uses instr-ref is loaded into a configuration where we would have chosen to not use instr-ref.

Copy link
Member

@jmorse jmorse Jan 28, 2025

Choose a reason for hiding this comment

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

And uh, that awkwardly has an effect on the test you're writing too.

return false;
// Only run this for functions that have fake uses.
if (!MF.hasFakeUses() || skipFunction(MF.getFunction()))
Expand Down
80 changes: 56 additions & 24 deletions llvm/test/CodeGen/X86/fake-use-remove-loads.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
# Ensure that loads into FAKE_USEs are correctly removed by the
# remove-loads-into-fake-uses pass.
Copy link
Member

@jmorse jmorse Jan 28, 2025

Choose a reason for hiding this comment

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

"... and that if we do not use instruction referencing, no modifications are made"

# RUN: llc -run-pass remove-loads-into-fake-uses -mtriple=x86_64-unknown-linux -debug-only=remove-loads-into-fake-uses 2>&1 -o - %s | FileCheck %s
# RUN: llc -run-pass remove-loads-into-fake-uses -mtriple=x86_64-unknown-linux -debug-only=remove-loads-into-fake-uses 2>&1 -o - %s | FileCheck %s --check-prefix=ENABLED
# RUN: llc -run-pass remove-loads-into-fake-uses -mtriple=x86_64-unknown-linux -debug-only=remove-loads-into-fake-uses -experimental-debug-variable-locations=false 2>&1 -o - %s | FileCheck %s --check-prefixes=DISABLED
# REQUIRES: asserts
#
## We verify that:
Expand Down Expand Up @@ -39,35 +40,66 @@ body: |
bb.0:
liveins: $esi, $rdi, $rdx, $r15, $r14, $r13, $r12, $r11, $rbx

; CHECK-LABEL: name: _ZN1g1jEv
; CHECK: liveins: $esi, $rdi, $rdx, $r15, $r14, $r13, $r12, $r11, $rbx
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: $rbx = MOV64rr $rdx
; CHECK-NEXT: $r14d = MOV32rr $esi
; CHECK-NEXT: $r15 = MOV64rr $rdi
; CHECK-NEXT: renamable $r12d = XOR32rr undef $r12d, undef $r12d, implicit-def dead $eflags, implicit-def $r12
; CHECK-NEXT: renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, implicit-def $rax




; ENABLED-LABEL: name: _ZN1g1jEv
; ENABLED: liveins: $esi, $rdi, $rdx, $r15, $r14, $r13, $r12, $r11, $rbx
; ENABLED-NEXT: {{ $}}
; ENABLED-NEXT: $rbx = MOV64rr $rdx
; ENABLED-NEXT: $r14d = MOV32rr $esi
; ENABLED-NEXT: $r15 = MOV64rr $rdi
; ENABLED-NEXT: renamable $r12d = XOR32rr undef $r12d, undef $r12d, implicit-def dead $eflags, implicit-def $r12
; ENABLED-NEXT: renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, implicit-def $rax

;; The store to the stack slot is still present.
; CHECK-NEXT: MOV64mr $rbp, 1, $noreg, -48, $noreg, killed renamable $rax :: (store (s64) into %stack.0)
; CHECK-NEXT: MOV64mr $rbp, 1, $noreg, -40, $noreg, killed renamable $r11 :: (store (s64) into %stack.1)
; CHECK-NEXT: renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags
; CHECK-NEXT: $r13d = MOV32rr killed $eax
; CHECK-NEXT: $rdi = MOV64rr $r15
; CHECK-NEXT: CALL64r renamable $r12, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp
; CHECK-NEXT: dead renamable $eax = MOV32rm renamable $rbx, 1, $noreg, 0, $noreg
; CHECK-NEXT: renamable $eax = MOV32ri 1
; CHECK-NEXT: TEST8ri renamable $r14b, 1, implicit-def $eflags
; ENABLED-NEXT: MOV64mr $rbp, 1, $noreg, -48, $noreg, killed renamable $rax :: (store (s64) into %stack.0)

; ENABLED-NEXT: MOV64mr $rbp, 1, $noreg, -40, $noreg, killed renamable $r11 :: (store (s64) into %stack.1)
; ENABLED-NEXT: renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags
; ENABLED-NEXT: $r13d = MOV32rr killed $eax
; ENABLED-NEXT: $rdi = MOV64rr $r15
; ENABLED-NEXT: CALL64r renamable $r12, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp
; ENABLED-NEXT: dead renamable $eax = MOV32rm renamable $rbx, 1, $noreg, 0, $noreg
; ENABLED-NEXT: renamable $eax = MOV32ri 1
; ENABLED-NEXT: TEST8ri renamable $r14b, 1, implicit-def $eflags

;; First FAKE_USE and its corresponding load are removed; second FAKE_USE of
;; a restored value that is also used is preserved.
; CHECK-NEXT: renamable $r11 = MOV64rm $rbp, 1, $noreg, -40, $noreg :: (load (s64) from %stack.1)
; CHECK-NEXT: renamable $r12d = XOR32rr $r12d, $r11d, implicit-def dead $eflags
; CHECK-NEXT: FAKE_USE killed renamable $r11d
; ENABLED-NEXT: renamable $r11 = MOV64rm $rbp, 1, $noreg, -40, $noreg :: (load (s64) from %stack.1)
; ENABLED-NEXT: renamable $r12d = XOR32rr $r12d, $r11d, implicit-def dead $eflags
; ENABLED-NEXT: FAKE_USE killed renamable $r11d

; ENABLED-NEXT: TEST32rr killed renamable $r13d, renamable $r13d, implicit-def $eflags
; ENABLED-NEXT: RET64
;
; DISABLED-LABEL: name: _ZN1g1jEv
; DISABLED: liveins: $esi, $rdi, $rdx, $r15, $r14, $r13, $r12, $r11, $rbx
; DISABLED-NEXT: {{ $}}
; DISABLED-NEXT: $rbx = MOV64rr $rdx
; DISABLED-NEXT: $r14d = MOV32rr $esi
; DISABLED-NEXT: $r15 = MOV64rr $rdi
; DISABLED-NEXT: renamable $r12d = XOR32rr undef $r12d, undef $r12d, implicit-def dead $eflags, implicit-def $r12
; DISABLED-NEXT: renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, implicit-def $rax
; DISABLED-NEXT: MOV64mr $rbp, 1, $noreg, -48, $noreg, killed renamable $rax :: (store (s64) into %stack.0)
; DISABLED-NEXT: MOV64mr $rbp, 1, $noreg, -40, $noreg, killed renamable $r11 :: (store (s64) into %stack.1)
; DISABLED-NEXT: renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags
; DISABLED-NEXT: $r13d = MOV32rr killed $eax
; DISABLED-NEXT: $rdi = MOV64rr $r15
; DISABLED-NEXT: CALL64r renamable $r12, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp
; DISABLED-NEXT: dead renamable $eax = MOV32rm renamable $rbx, 1, $noreg, 0, $noreg
; DISABLED-NEXT: renamable $eax = MOV32ri 1
; DISABLED-NEXT: TEST8ri renamable $r14b, 1, implicit-def $eflags

; CHECK-NEXT: TEST32rr killed renamable $r13d, renamable $r13d, implicit-def $eflags
; CHECK-NEXT: RET64
;; Verify that when instr-ref is disabled, we do not remove fake uses.
; DISABLED-NEXT: renamable $rax = MOV64rm $rbp, 1, $noreg, -48, $noreg :: (load (s64) from %stack.0)
; DISABLED-NEXT: FAKE_USE renamable $eax, implicit killed $rax
; DISABLED-NEXT: renamable $r11 = MOV64rm $rbp, 1, $noreg, -40, $noreg :: (load (s64) from %stack.1)
; DISABLED-NEXT: renamable $r12d = XOR32rr $r12d, $r11d, implicit-def dead $eflags
; DISABLED-NEXT: FAKE_USE killed renamable $r11d
; DISABLED-NEXT: TEST32rr killed renamable $r13d, renamable $r13d, implicit-def $eflags
; DISABLED-NEXT: RET64
$rbx = MOV64rr $rdx
$r14d = MOV32rr $esi
$r15 = MOV64rr $rdi
Expand Down
Loading