[PHIElimination] Update LiveVariables after handling an unspillable terminator

[foad]

Summary

Update the LiveVariables analysis after the special handling for
unspillable terminators which was added in D91358. This is just enough
to fix some "Block should not be in AliveBlocks" / "Block missing from
AliveBlocks" errors in the codegen test suite when machine verification
is forced to run after PHIElimination (currently it is disabled).

[dmgreen]

This sounds conceptually OK to me - that we would need to update the live variables for unspillable terminators - but I don't know a lot about updating LV, so don't know a lot of the details.

Should the Incoming blocks be the Src blocks, or should it be a combination of Incoming and Src?

[foad]

In D110939#3039375, @dmgreen wrote:

This sounds conceptually OK to me - that we would need to update the live variables for unspillable terminators - but I don't know a lot about updating LV, so don't know a lot of the details.

Should the Incoming blocks be the Src blocks, or should it be a combination of Incoming and Src?

I think at this point IncomingReg is a freshly created register, so it won't have any AliveBlocks yet.

Having said that I admit I don't understand all the details either, I was just doing the minimum to get verification to pass, since my next project is to remove LiveVariables anyway in favour of LiveIntervals.

[MatzeB]

LGTM.

Please try if the suggested changes work. If not then I'm fine with the change as-is.

llvm/lib/CodeGen/PHIElimination.cpp

465–471

As far as I can tell, the other code here that creates COPYs etc. does not bother creating a information for IncomingReg. So maybe we should do the same here and just concern ourself with dropping the old info?

[dmgreen]

If it helps, I would expect the unspillable terminators to look like:

bb1:
  ..
  %r = UnspillableTerminator ..
bb2:
  %p = phi %r, ...
  ... uses of %p

In our usecase in the Arm backend, bb2 will be a loop header, usually with a single block (but not necessarily, especially in the future).

[foad]

In D110939#3041032, @dmgreen wrote:

If it helps, I would expect the unspillable terminators to look like:

bb1:
  ..
  %r = UnspillableTerminator ..
bb2:
  %p = phi %r, ...
  ... uses of %p

In our usecase in the Arm backend, bb2 will be a loop header, usually with a single block (but not necessarily, especially in the future).

In the tests I am looking at, there is a preheader block in between your bb1 and bb2, and %r is live-through the preheader.

[foad]

llvm/lib/CodeGen/PHIElimination.cpp

465–471

As far as I can tell, the other code here that creates COPYs etc. does not bother creating a information for IncomingReg.

There is certainly some existing code that updates LV info for IncomingReg, though I haven't looked at it in detail.

So maybe we should do the same here and just concern ourself with dropping the old info?

That only fixes the "Block should not be in AliveBlocks" errors, not the "Block missing from AliveBlocks" errors.

llvm/lib/CodeGen/PHIElimination.cpp

if (SrcRegDef && TII->isUnspillableTerminator(SrcRegDef)) {
       assert(SrcRegDef->getOperand(0).isReg() &&
              SrcRegDef->getOperand(0).isDef() &&
              "Expected operand 0 to be a reg def!");
       // Now that the PHI's use has been removed (as the instruction was
       // removed) there should be no other uses of the SrcReg.
       assert(MRI->use_empty(SrcReg) &&
              "Expected a single use from UnspillableTerminator");
       SrcRegDef->getOperand(0).setReg(IncomingReg);
+
+      // Update LiveVariables.
+      if (LV) {
+        LiveVariables::VarInfo &SrcVI = LV->getVarInfo(SrcReg);
+        LiveVariables::VarInfo &IncomingVI = LV->getVarInfo(IncomingReg);
+        IncomingVI.AliveBlocks = std::move(SrcVI.AliveBlocks);
+        SrcVI.AliveBlocks.clear();
+      }
+
       continue;
     }
 
     // Find a safe location to insert the copy, this may be the first terminator
     // in the block (or end()).
     MachineBasicBlock::iterator InsertPos =
       findPHICopyInsertPoint(&opBlock, &MBB, SrcReg);