[MachineLICM] Recognize registers clobbered at EH landing pad entry #122446

uweigand · 2025-01-10T12:31:04Z

EH landing pad entry implicitly clobbers target-specific exception pointer and exception selector registers. The post-RA MachineLICM pass needs to take these into account when deciding whether to hoist an instruction out of the loop that initializes one of these registers.

Fixes: #122315

llvmbot · 2025-01-10T12:31:38Z

@llvm/pr-subscribers-backend-systemz

Author: Ulrich Weigand (uweigand)

Changes

EH landing pad entry implicitly clobbers target-specific exception pointer and exception selector registers. The post-RA MachineLICM pass needs to take these into account when deciding whether to hoist an instruction out of the loop that initializes one of these registers.

Fixes: #122315

Full diff: https://github.com/llvm/llvm-project/pull/122446.diff

2 Files Affected:

(modified) llvm/lib/CodeGen/MachineLICM.cpp (+15)
(added) llvm/test/CodeGen/SystemZ/pr122315.ll (+59)

diff --git a/llvm/lib/CodeGen/MachineLICM.cpp b/llvm/lib/CodeGen/MachineLICM.cpp
index d1d5509dc482a2..9718e24caee0af 100644
--- a/llvm/lib/CodeGen/MachineLICM.cpp
+++ b/llvm/lib/CodeGen/MachineLICM.cpp
@@ -639,6 +639,21 @@ void MachineLICMImpl::HoistRegionPostRA(MachineLoop *CurLoop,
     if (const uint32_t *Mask = BB->getBeginClobberMask(TRI))
       applyBitsNotInRegMaskToRegUnitsMask(*TRI, RUClobbers, Mask);
 
+    // EH landing pads clobber exception pointer/selector registers
+    if (BB->isEHPad()) {
+      const MachineFunction &MF = *BB->getParent();
+      if (MF.getFunction().hasPersonalityFn()) {
+        auto PersonalityFn = MF.getFunction().getPersonalityFn();
+        const TargetLowering &TLI = *MF.getSubtarget().getTargetLowering();
+        if (unsigned Reg = TLI.getExceptionPointerRegister(PersonalityFn))
+          for (MCRegUnitIterator RUI(Reg, TRI); RUI.isValid(); ++RUI)
+            RUClobbers.set(*RUI);
+        if (unsigned Reg = TLI.getExceptionSelectorRegister(PersonalityFn))
+          for (MCRegUnitIterator RUI(Reg, TRI); RUI.isValid(); ++RUI)
+            RUClobbers.set(*RUI);
+      }
+    }
+
     SpeculationState = SpeculateUnknown;
     for (MachineInstr &MI : *BB)
       ProcessMI(&MI, RUDefs, RUClobbers, StoredFIs, Candidates, CurLoop);
diff --git a/llvm/test/CodeGen/SystemZ/pr122315.ll b/llvm/test/CodeGen/SystemZ/pr122315.ll
new file mode 100644
index 00000000000000..b094d11dbc9a56
--- /dev/null
+++ b/llvm/test/CodeGen/SystemZ/pr122315.ll
@@ -0,0 +1,59 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; Verify that MachineLICM recognizes that EH landing pad entry clobbers %r6/%r7
+;
+; RUN: llc < %s -verify-machineinstrs -mtriple=s390x-linux-gnu -mcpu=z10 | FileCheck %s
+
+declare i64 @personality(i64, i64, i64, ptr, ptr)
+declare void @callee(i64, i64, i64, i64, i64)
+declare void @panic()
+
+define void @test() uwtable personality ptr @personality {
+; CHECK-LABEL: test:
+; CHECK:       .Lfunc_begin0:
+; CHECK-NEXT:    .cfi_startproc
+; CHECK-NEXT:    .cfi_personality 0, personality
+; CHECK-NEXT:    .cfi_lsda 0, .Lexception0
+; CHECK-NEXT:  # %bb.0: # %start
+; CHECK-NEXT:    stmg %r6, %r15, 48(%r15)
+; CHECK-NEXT:    .cfi_offset %r6, -112
+; CHECK-NEXT:    .cfi_offset %r7, -104
+; CHECK-NEXT:    .cfi_offset %r14, -48
+; CHECK-NEXT:    .cfi_offset %r15, -40
+; CHECK-NEXT:    aghi %r15, -160
+; CHECK-NEXT:    .cfi_def_cfa_offset 320
+; CHECK-NEXT:  .LBB0_1: # %bb1
+; CHECK-NEXT:    # =>This Inner Loop Header: Depth=1
+; CHECK-NEXT:    lghi %r2, 0
+; CHECK-NEXT:    lghi %r3, 0
+; CHECK-NEXT:    lghi %r4, 0
+; CHECK-NEXT:    lghi %r5, 0
+; CHECK-NEXT:    lghi %r6, 0
+; CHECK-NEXT:    brasl %r14, callee@PLT
+; CHECK-NEXT:  .Ltmp0:
+; CHECK-NEXT:    brasl %r14, panic@PLT
+; CHECK-NEXT:  .Ltmp1:
+; CHECK-NEXT:    j .LBB0_3
+; CHECK-NEXT:  .LBB0_2: # %bb3
+; CHECK-NEXT:    # in Loop: Header=BB0_1 Depth=1
+; CHECK-NEXT:  .Ltmp2:
+; CHECK-NEXT:    j .LBB0_1
+; CHECK-NEXT:  .LBB0_3: # %bb2
+; CHECK-NEXT:    lmg %r6, %r15, 208(%r15)
+; CHECK-NEXT:    br %r14
+start:
+  br label %bb1
+
+bb1:
+  call void @callee(i64 0, i64 0, i64 0, i64 0, i64 0)
+  invoke void @panic()
+          to label %bb2 unwind label %bb3
+
+bb2:
+  ret void
+
+bb3:
+  %lp = landingpad { ptr, i32 }
+          catch ptr null
+  br label %bb1
+}
+

arsenm · 2025-01-10T12:46:40Z

llvm/lib/CodeGen/MachineLICM.cpp

+      const MachineFunction &MF = *BB->getParent();
+      if (MF.getFunction().hasPersonalityFn()) {
+        auto PersonalityFn = MF.getFunction().getPersonalityFn();
+        const TargetLowering &TLI = *MF.getSubtarget().getTargetLowering();
+        if (unsigned Reg = TLI.getExceptionPointerRegister(PersonalityFn))
+          for (MCRegUnitIterator RUI(Reg, TRI); RUI.isValid(); ++RUI)
+            RUClobbers.set(*RUI);
+        if (unsigned Reg = TLI.getExceptionSelectorRegister(PersonalityFn))
+          for (MCRegUnitIterator RUI(Reg, TRI); RUI.isValid(); ++RUI)
+            RUClobbers.set(*RUI);
+      }
+    }


This is the wrong place for this. This pass is already inventing its own liveness tracking instead of LiveRegUnits, which it should be using.

Why is this case special? Are these not reserved and/or callee saved?

They are not reserved, but they are callee saved. That's why they're considered candidates to be hoisted even though there's a call in the loop. And normally that would be fine except for the special EH landing pad case.

I'm not sure where else these should be handled, I simply noticed that several other places also handle these registers specially.

Should this be just

if (BB->isEHPad()) { if (const uint32_t *Mask = TRI->getCustomEHPadPreservedMask(*BB->getParent())) applyBitsNotInRegMaskToRegUnitsMask(*TRI, RUClobbers, Mask); }

Note however that this only partly addresses the issue. The continue at the beginning of the loop is wrong because it results in un-analyzed instructions. If an inner loop (with header-landingpad) had a register def/clobber, it won't be taken into account by the loop over Candidates below.

I've been debugging a similar issue for quite some time now, but I'm struggling to reproduce it on an upstream target =\

Should this be just

Please ignore this. It appears this method only needs to be implemented if the unwinder clobbers registers other than exception pointer/selector. DWARF exception handling would usually preserve those other registers.

That said, it should be queried by LICM, but it is a different issue.

I wonder whether livein_iterator (used just above) should take care of these registers implicitly like liveout_iterator does

llvm-project/llvm/include/llvm/CodeGen/MachineBasicBlock.h

Lines 550 to 559 in 66f158d

liveout_iterator &operator++() {

do {

++LiveRegI;

if (!advanceToValidPosition())

return *this;

} while ((*BlockI)->isEHPad() &&

(LiveRegI->PhysReg == ExceptionPointer ||

LiveRegI->PhysReg == ExceptionSelector));

return *this;

}

ADD
liveout_iterator seems to skip those registers when it discovers them on the live-in list of a successor block.
Doesn't that mean that they should be on that list already? It looks like instruction selector does add them to the list, but then for (const auto &LI : BB->liveins()) { should have already dealt with them making this change unnecessary?

Yes, they're already in the live-ins list of the EH landing pad. But that for loop only adds them to RUDefs, which is not sufficient - they need to be added to RUClobbers; their values are not inherited from the predecessor block as for a "normal" live-in, but their values are actually clobbered by the EH runtime library, which is not visible to the analysis at this point.

Thanks, that makes sense to me now.

arsenm · 2025-04-25T15:07:11Z

llvm/lib/CodeGen/MachineLICM.cpp

+    if (BB->isEHPad()) {
+      const MachineFunction &MF = *BB->getParent();
+      if (MF.getFunction().hasPersonalityFn()) {
+        auto PersonalityFn = MF.getFunction().getPersonalityFn();


arsenm · 2025-04-25T15:07:18Z

llvm/lib/CodeGen/MachineLICM.cpp

+      if (MF.getFunction().hasPersonalityFn()) {
+        auto PersonalityFn = MF.getFunction().getPersonalityFn();
+        const TargetLowering &TLI = *MF.getSubtarget().getTargetLowering();
+        if (unsigned Reg = TLI.getExceptionPointerRegister(PersonalityFn))


Use MCRegister

arsenm · 2025-04-25T15:07:47Z

llvm/lib/CodeGen/MachineLICM.cpp

+    // EH landing pads clobber exception pointer/selector registers
+    if (BB->isEHPad()) {
+      const MachineFunction &MF = *BB->getParent();
+      if (MF.getFunction().hasPersonalityFn()) {


Does the verifier not enforce there must be a personality function if there is an EH pad?

Indeed it does, thanks! Patch updated.

s-barannikov

LGTM with a suggestion

llvm/lib/CodeGen/MachineLICM.cpp

EH landing pad entry implicitly clobbers target-specific exception pointer and exception selector registers. The post-RA MachineLICM pass needs to take these into account when deciding whether to hoist an instruction out of the loop that initializes one of these registers. Fixes: llvm#122315

…lvm#122446) EH landing pad entry implicitly clobbers target-specific exception pointer and exception selector registers. The post-RA MachineLICM pass needs to take these into account when deciding whether to hoist an instruction out of the loop that initializes one of these registers. Fixes: llvm#122315

llvmbot added the backend:SystemZ label Jan 10, 2025

uweigand added llvm:codegen miscompilation labels Jan 10, 2025

uweigand requested review from arsenm, davemgreen and qcolombet January 10, 2025 12:39

arsenm reviewed Jan 10, 2025

View reviewed changes

arsenm reviewed Apr 25, 2025

View reviewed changes

uweigand force-pushed the machinelicm-ehpad-clobber branch from 001ecec to f736ef7 Compare April 25, 2025 15:36

s-barannikov approved these changes Apr 25, 2025

View reviewed changes

llvm/lib/CodeGen/MachineLICM.cpp Outdated Show resolved Hide resolved

llvm/lib/CodeGen/MachineLICM.cpp Outdated Show resolved Hide resolved

uweigand force-pushed the machinelicm-ehpad-clobber branch from f736ef7 to ed4eb1f Compare April 25, 2025 17:46

uweigand merged commit be7ef6c into llvm:main Apr 25, 2025
11 checks passed

uweigand deleted the machinelicm-ehpad-clobber branch April 25, 2025 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MachineLICM] Recognize registers clobbered at EH landing pad entry #122446

[MachineLICM] Recognize registers clobbered at EH landing pad entry #122446

uweigand commented Jan 10, 2025

llvmbot commented Jan 10, 2025

arsenm Jan 10, 2025

uweigand Jan 10, 2025

s-barannikov Apr 12, 2025 •

edited

Loading

s-barannikov Apr 13, 2025

s-barannikov Apr 13, 2025 •

edited

Loading

uweigand Apr 25, 2025

s-barannikov Apr 25, 2025

arsenm Apr 25, 2025

arsenm Apr 25, 2025

arsenm Apr 25, 2025

uweigand Apr 25, 2025

s-barannikov left a comment

	liveout_iterator &operator++() {
	do {
	++LiveRegI;
	if (!advanceToValidPosition())
	return *this;
	} while ((*BlockI)->isEHPad() &&
	(LiveRegI->PhysReg == ExceptionPointer \|\|
	LiveRegI->PhysReg == ExceptionSelector));
	return *this;
	}

[MachineLICM] Recognize registers clobbered at EH landing pad entry #122446

[MachineLICM] Recognize registers clobbered at EH landing pad entry #122446

Conversation

uweigand commented Jan 10, 2025

llvmbot commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-barannikov Apr 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-barannikov Apr 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-barannikov left a comment

Choose a reason for hiding this comment

s-barannikov Apr 12, 2025 •

edited

Loading

s-barannikov Apr 13, 2025 •

edited

Loading