[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

540–543

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

540–543

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

[arsenm]

llvm/lib/CodeGen/MachineLICM.cpp

540–543

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

void MachineLICMBase::HoistRegionPostRA() {
   BitVector PhysRegClobbers(NumRegs); // Regs defined more than once.
 
   SmallVector<CandidateInfo, 32> Candidates;
   SmallSet<int, 32> StoredFIs;
 
   // Walk the entire region, count number of defs for each register, and
   // collect potential LICM candidates.
   for (MachineBasicBlock *BB : CurLoop->getBlocks()) {
+    // We can't hoist instructions out of a funclet, so if the region
+    // contains a funclet (i.e. the loop contains a try-catch) we give up
+    if (BB->isEHFuncletEntry())
+      return;
     // 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);
     }
 
     SpeculationState = SpeculateUnknown;
     for (MachineInstr &MI : *BB)
       ProcessMI(&MI, PhysRegDefs, PhysRegClobbers, StoredFIs, Candidates);
   }
 
   // Gather the registers read / clobbered by the terminator.
   BitVector TermRegs(NumRegs);