Skip to content

[MachineLICM] Do not rely on hasSideEffect when handling impdefs #145379

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

Closed
wants to merge 4 commits into from
Closed

[MachineLICM] Do not rely on hasSideEffect when handling impdefs #145379

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df21ee2
[MachineLICM] Precommit tests for implicit-def handling
atrosinenko Jun 23, 2025
182a490
[MachineLICM] Do not rely on hasSideEffect when handling impdefs
atrosinenko Jun 10, 2025
707a36c
Fix hoisting safe dead implicit-defs
atrosinenko Jun 26, 2025
4c29db1
Drop useless corner case
atrosinenko Jul 3, 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
21 changes: 9 additions & 12 deletions llvm/lib/CodeGen/MachineLICM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,23 +554,18 @@ void MachineLICMImpl::ProcessMI(MachineInstr *MI, BitVector &RUDefs,
}

if (MO.isImplicit()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's unclear to me is why implicit defs are special here as well. I uploaded #147624 which merges the logic for implicit and explicit defs, and it passes the new test from this PR. I also ran a stage2 check-llvm and check-clang and they both pass. It requires updates to some target-specific tests; I'm not an expert on all of the affected ISAs but they seem correct.

for (MCRegUnit Unit : TRI->regunits(Reg))
RUClobbers.set(Unit);
if (!MO.isDead())
// Non-dead implicit def? This cannot be hoisted.
RuledOut = true;
// No need to check if a dead implicit def is also defined by
// another instruction.
continue;
} else {
// FIXME: For now, avoid instructions with multiple defs, unless
// it's a dead implicit def.
if (Def)
RuledOut = true;
else
Def = Reg;
}

// FIXME: For now, avoid instructions with multiple defs, unless
// it's a dead implicit def.
if (Def)
RuledOut = true;
else
Def = Reg;

// If we have already seen another instruction that defines the same
// register, then this is not safe. Two defs is indicated by setting a
// PhysRegClobbers bit.
Expand All @@ -585,6 +580,8 @@ void MachineLICMImpl::ProcessMI(MachineInstr *MI, BitVector &RUDefs,
}

RUDefs.set(Unit);
if (MO.isImplicit())
Copy link
Contributor

Choose a reason for hiding this comment

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

What makes implicit defs special here (compared to explicit defs)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropped the special case in 4c29db1, thank you!

After investigating HoistRegionPostRA and ProcessMI functions for a while, it feels like the description of RUDefs and RUClobbers are hardly accurate:

  BitVector RUDefs(NumRegUnits);     // RUs defined once in the loop.
  BitVector RUClobbers(NumRegUnits); // RUs defined more than once.

These two variables are defined in HoistRegionPostRA and used both in HoistRegionPostRA and ProcessMI by setting and testing bits as well as by mass-settint bits in applyBitsNotInRegMaskToRegUnitsMask.

I have an impression that RUClobbers is sometimes set while it would be better to first check RUDefs (and possibly set corresponding bit there instead), such as when handling MO.isRegMask() case a few lines above - it looks harmless, even though slightly pessimistic, to me.

At the same time, RUDefs seems to mix two different properties of a register: "the register is live" vs. "we have observed an instruction that defined the register that was initially dead" - while both should transition to "clobbered" state after defining that register one more time, only the latter implies some sort of non-uniformity. Thus, when HoistRegionPostRA rejects candidate instructions as follows:

    MachineInstr *MI = Candidate.MI;
    for (const MachineOperand &MO : MI->all_uses()) {
      if (!MO.getReg())
        continue;
      for (MCRegUnit Unit : TRI->regunits(MO.getReg())) {
        if (RUDefs.test(Unit) || RUClobbers.test(Unit)) {
          // If it's using a non-loop-invariant register, then it's obviously
          // not safe to hoist.
          Safe = false;
          break;
        }
      }

      if (!Safe)
        break;
    }

it looks like rejecting any instructions having any register inputs given that

    // Conservatively treat live-in's as an external def.
    // FIXME: That means a reload that're reused in successor block(s) will not
    // be LICM'ed.
    for (const auto &LI : BB->liveins()) {
      for (MCRegUnit Unit : TRI->regunits(LI.PhysReg))
        RUDefs.set(Unit);
    }

Quite surprisingly, it is not, because on X86 something like this is possible (here $rip is not a live-in):

renamable $rcx = MOV64rm $rip, 1, $noreg, target-flags(x86-gotpcrel) @x, $noreg, debug-location !22 :: (load (s64) from got); t.ll:5:7

Nevertheless, assuming that the only reason to treat implicit register operands specially is to ignore dead implicit defs for simplicity, it looks harmless to handle any defined register identically, whether it is implicit or explicit.

RUClobbers.set(Unit);
}
}

Expand Down
103 changes: 103 additions & 0 deletions llvm/test/CodeGen/AArch64/mlicm-implicit-defs.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
# RUN: llc -mtriple=aarch64 -run-pass machinelicm -verify-machineinstrs -o - %s | FileCheck %s
# RUN: llc -mtriple=aarch64 -passes machinelicm -o - %s | FileCheck %s

---
name: unsafe_to_move
tracksRegLiveness: true
body: |
; CHECK-LABEL: name: unsafe_to_move
; CHECK: bb.0:
; CHECK-NEXT: successors: %bb.1(0x80000000)
; CHECK-NEXT: liveins: $x0
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: $x16 = COPY killed $x0
; CHECK-NEXT: B %bb.1
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.1:
; CHECK-NEXT: successors: %bb.1(0x40000000), %bb.2(0x40000000)
; CHECK-NEXT: liveins: $x16
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: $x1 = COPY killed $x16
; CHECK-NEXT: $x2 = MOVi64imm 1024, implicit-def dead $x16
; CHECK-NEXT: $x16 = LDRXroX killed $x1, killed $x2, 0, 0
; CHECK-NEXT: $xzr = SUBSXri $x16, 0, 0, implicit-def $nzcv
; CHECK-NEXT: Bcc 1, %bb.1, implicit $nzcv
; CHECK-NEXT: B %bb.2
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.2:
; CHECK-NEXT: liveins: $x1
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: $x0 = COPY killed $x1
; CHECK-NEXT: RET_ReallyLR
bb.0:
liveins: $x0
$x16 = COPY killed $x0
B %bb.1

bb.1:
liveins: $x16
$x1 = COPY killed $x16
/* MOVi64imm below mimics a pseudo instruction that doesn't have any */
/* unmodelled side effects, but uses x16 as a scratch register. */
$x2 = MOVi64imm 1024, implicit-def dead $x16
$x16 = LDRXroX killed $x1, killed $x2, 0, 0
$xzr = SUBSXri $x16, 0, 0, implicit-def $nzcv
Bcc 1, %bb.1, implicit $nzcv
B %bb.2

bb.2:
liveins: $x1
$x0 = COPY killed $x1
RET_ReallyLR
...

---
name: dead_implicit_def
tracksRegLiveness: true
body: |
; CHECK-LABEL: name: dead_implicit_def
; CHECK: bb.0:
; CHECK-NEXT: successors: %bb.1(0x80000000)
; CHECK-NEXT: liveins: $x0
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: $x12 = COPY killed $x0
; CHECK-NEXT: $x2 = MOVi64imm 1024, implicit-def dead $x16
; CHECK-NEXT: B %bb.1
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.1:
; CHECK-NEXT: successors: %bb.1(0x40000000), %bb.2(0x40000000)
; CHECK-NEXT: liveins: $x12, $x2
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: $x1 = COPY killed $x12
; CHECK-NEXT: $x16 = LDRXroX killed $x1, $x2, 0, 0
; CHECK-NEXT: $xzr = SUBSXri $x16, 0, 0, implicit-def $nzcv
; CHECK-NEXT: Bcc 1, %bb.1, implicit $nzcv
; CHECK-NEXT: B %bb.2
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.2:
; CHECK-NEXT: liveins: $x1
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: $x0 = COPY killed $x1
; CHECK-NEXT: RET_ReallyLR
bb.0:
liveins: $x0
$x12 = COPY killed $x0
B %bb.1

bb.1:
liveins: $x12
$x1 = COPY killed $x12
/* MOVi64imm below mimics a pseudo instruction that doesn't have any */
/* unmodelled side effects, but uses x16 as a scratch register. */
$x2 = MOVi64imm 1024, implicit-def dead $x16
$x16 = LDRXroX killed $x1, killed $x2, 0, 0
$xzr = SUBSXri $x16, 0, 0, implicit-def $nzcv
Bcc 1, %bb.1, implicit $nzcv
B %bb.2

bb.2:
liveins: $x1
$x0 = COPY killed $x1
RET_ReallyLR
...
Loading