[MachineLICM][WinEH] Don't hoist register reloads out of funclets

[kalle-llvm]

Summary

This fixes https://github.com/llvm/llvm-project/issues/60766

With MSVC style exception-handling (funclets), no registers are alive when entering the funclet so they must be reloaded from the stack. MachineLICM can sometimes hoist such reloads out of the funclet which is not correct, the register will have been clobbered when entering the funclet. This can happen in any loop that contains a try-catch.

This has been tested on x86_64-pc-window-msvc. I'm not sure if funclets work the same on the other windows archs.

(This is my first patch to llvm, so please bear with me.)

[smeenai]

I added some appropriate reviewers. You should also include a test case though.

[efriedma]

The concept here seems right; we should treat funclet entry as if it clobbers all registers.

I'm not sure about the implementation here; we should have some common utility that encompasses this. Making MachineLICM compute its own version of which registers are live seems liable to lead to bugs. I guess getBeginClobberMask() is supposed to encompass what this is checking for?

[arsenm]

Needs tests

llvm/lib/CodeGen/MachineLICM.cpp

536–539

We're moving towards reverse block liveness. Modern liveness queries start at the end of the block and walk backwards, but this is walking forward looking for defs. Can this pass switch to using LiveRegUnits and stepBackwards?

[kalle-llvm]

Sorry for the delay in getting back to this.

I have updated the patch to use getBeginClobberMask() and added a test-case.

[kalle-llvm]

Ping?

[rnk]

This looks correct to me.

llvm/lib/CodeGen/MachineLICM.cpp

536–539

That sounds like a good idea, but it seems out of scope for this bug fix.

[arsenm]

llvm/lib/CodeGen/MachineLICM.cpp

536–539

Yes, but this is just a quick spot fix. For some reason a lot of machine passes invented their own buggy liveness tracking mechanisms and miss the edge cases like this one

[kalle-llvm]

Thanks @rnk @arsenm. Could someone land this for me? Any chance to have this in llvm 17?

Karl-Johan Johnsson <[e-mail]>

[mstorsjo]

In D153337#4581909, @kalle-llvm wrote:

Thanks @rnk @arsenm. Could someone land this for me?

Done, thanks for your contribution!

Any chance to have this in llvm 17?

I'd leave that up to @rnk and @arsenm. Unless someone says something to the contrary - I'd suggest opening a backport request according to https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches - i.e. open a new github issue, add it to the 17.0.x release milestone and type /cherry-pick <commit-id> in the message field; the reviewers of this patch should ideally automatically end up CCd on the actual backport request and can say whether they think it is suitable for backporting.

llvm/lib/CodeGen/MachineLICM.cpp

for (MachineBasicBlock *BB : CurLoop->getBlocks()) {
     // If the header of the loop containing this basic block is a landing pad,
     // then don't try to hoist instructions out of this loop.
     const MachineLoop *ML = MLI->getLoopFor(BB);
     if (ML && ML->getHeader()->isEHPad()) continue;
 
     // 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 (MCRegAliasIterator AI(LI.PhysReg, TRI, true); AI.isValid(); ++AI)
         PhysRegDefs.set(*AI);
     }
 
+    // Funclet entry blocks will clobber all registers
+    if (const uint32_t *Mask = BB->getBeginClobberMask(TRI))
+      PhysRegClobbers.setBitsNotInMask(Mask);
+
     SpeculationState = SpeculateUnknown;
     for (MachineInstr &MI : *BB)
       ProcessMI(&MI, PhysRegDefs, PhysRegClobbers, StoredFIs, Candidates);
   }
 
   // Gather the registers read / clobbered by the terminator.
   BitVector TermRegs(NumRegs);
   MachineBasicBlock::iterator TI = Preheader->getFirstTerminator();

llvm/test/CodeGen/X86/machine-licm-vs-wineh.mir

+# RUN: llc -o - %s -mtriple=x86_64-pc-windows-msvc -run-pass=machinelicm | FileCheck %s
+#
+# This test checks that MachineLICM doesn't hoist loads out of funclets.
+# Manually modified from the IR of the following C++ function by running
+# llc -stop-after=machine-cp.
+#
+# void may_throw();
+# void use(int);
+#
+# void test(int n, int arg)
+# {
+#    for (int i = 0 ; i < n ; i++)
+#        try {
+#            may_throw();
+#        }
+#        catch (...) {
+#            // Two uses to get 'arg' allocated to a register
+#            use(arg);
+#            use(arg);
+#        }
+# }
+
+--- |
+  target triple = "x86_64-pc-windows-msvc"
+
+  define void @test(i32 %n, i32 %arg) personality ptr @__CxxFrameHandler3 {
+  entry:
+    %cmp3 = icmp sgt i32 %n, 0
+    br i1 %cmp3, label %for.body.preheader, label %for.cond.cleanup
+
+  for.body.preheader:                               ; preds = %entry
+    br label %for.body
+
+  for.cond.cleanup:                                 ; preds = %for.inc, %entry
+    ret void
+
+  for.body:                                         ; preds = %for.body.preheader, %for.inc
+    %lsr.iv = phi i32 [ %n, %for.body.preheader ], [ %lsr.iv.next, %for.inc ]
+    invoke void @may_throw()
+            to label %for.inc unwind label %catch.dispatch
+
+  catch.dispatch:                                   ; preds = %for.body
+    %0 = catchswitch within none [label %catch] unwind to caller
+
+  catch:                                            ; preds = %catch.dispatch
+    %1 = catchpad within %0 [ptr null, i32 64, ptr null]
+    call void @use(i32 %arg) [ "funclet"(token %1) ]
+    call void @use(i32 %arg) [ "funclet"(token %1) ]
+    catchret from %1 to label %for.inc
+
+  for.inc:                                          ; preds = %catch, %for.body
+    %lsr.iv.next = add i32 %lsr.iv, -1
+    %exitcond.not = icmp eq i32 %lsr.iv.next, 0
+    br i1 %exitcond.not, label %for.cond.cleanup, label %for.body
+  }
+
+  declare i32 @__CxxFrameHandler3(...)
+
+  declare void @may_throw()
+
+  declare void @use(i32)
+
+...
+---
+name:            test
+alignment:       16
+tracksRegLiveness: true
+hasEHCatchret:   true
+hasEHScopes:     true
+hasEHFunclets:   true
+debugInstrRef:   true
+tracksDebugUserValues: true
+liveins:
+  - { reg: '$ecx' }
+  - { reg: '$edx' }
+frameInfo:
+  maxAlignment:    8
+  hasCalls:        true
+  hasOpaqueSPAdjustment: true
+stack:
+  - { id: 0, type: spill-slot, size: 4, alignment: 4 }
+  - { id: 1, type: spill-slot, size: 4, alignment: 4 }
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    successors: %bb.1, %bb.2
+    liveins: $ecx, $edx
+
+    MOV32mr %stack.1, 1, $noreg, 0, $noreg, $edx :: (store (s32) into %stack.1)
+    TEST32rr renamable $ecx, renamable $ecx, implicit-def $eflags
+    JCC_1 %bb.2, 14, implicit killed $eflags
+
+  bb.1:
+    liveins: $ecx
+
+    JMP_1 %bb.3
+
+  bb.2.for.cond.cleanup:
+    RET 0
+
+  bb.3.for.body:
+    successors: %bb.5, %bb.4
+    liveins: $ecx
+
+    EH_LABEL <mcsymbol .Leh1>
+    MOV32mr %stack.0, 1, $noreg, 0, $noreg, killed renamable $ecx :: (store (s32) into %stack.0)
+    ADJCALLSTACKDOWN64 32, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    CALL64pcrel32 @may_throw, csr_win64, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp
+    ADJCALLSTACKUP64 32, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    EH_LABEL <mcsymbol .Leh2>
+    JMP_1 %bb.5
+
+  bb.4.catch (landing-pad, ehfunclet-entry):
+    successors: %bb.5
+
+    ADJCALLSTACKDOWN64 32, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    renamable $esi = MOV32rm %stack.1, 1, $noreg, 0, $noreg :: (load (s32) from %stack.1)
+    $ecx = COPY renamable $esi
+    CALL64pcrel32 @use, csr_win64, implicit $rsp, implicit $ssp, implicit $ecx, implicit-def $rsp, implicit-def $ssp
+    ADJCALLSTACKUP64 32, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    ADJCALLSTACKDOWN64 32, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    $ecx = COPY killed renamable $esi
+    CALL64pcrel32 @use, csr_win64, implicit $rsp, implicit $ssp, implicit $ecx, implicit-def $rsp, implicit-def $ssp
+    ADJCALLSTACKUP64 32, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    CATCHRET %bb.5, %bb.0
+
+  bb.5.for.inc:
+    successors: %bb.2, %bb.3
+
+    renamable $ecx = MOV32rm %stack.0, 1, $noreg, 0, $noreg :: (load (s32) from %stack.0)
+    renamable $ecx = DEC32r killed renamable $ecx, implicit-def $eflags
+    JCC_1 %bb.2, 4, implicit killed $eflags
+    JMP_1 %bb.3
+
+...
+#
+# CHECK: bb.4.catch
+# CHECK: ADJCALLSTACKDOWN64
+# CHECK-NEXT: renamable [[REG:\$[a-z0-9]+]] = MOV32rm %stack.1
+# CHECK-NEXT: $ecx = COPY renamable [[REG]]
+# CHECK-NEXT: CALL64pcrel32 @use