[PATCH] D80274: [MachineVerifier] Handle the PHI node for verifyLiveVariables()

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:2309
+    for (const MachineInstr &MI : MBB.instrs()) {
+      if (!MI.isPHI())
+        break;
----------------
Should be possible to use MBB.phis() on previous line, to iterate over the range of MI:s that are PHI nodes.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:2319
+
+        // If the Reg is in MInfo.vregsLiveIn, it has been added to
+        // vregsRequired.
----------------
I had expected something like this, simply dealing with the vregs being live in and used in the PHI instructions (earlier left out from vregsLiveIn since we need to care about the control flow edges).

```
for (unsigned i = 1, e = MI.getNumOperands(); i != e; i += 2) {
  // Skip those Operands which are undef regs or not regs.
  if (!MI.getOperand(i).isReg() || !MI.getOperand(i).readsReg())
    continue;.

  // Get register and predecessor for one PHI edge.
  unsigned Reg = MI.getOperand(i).getReg();
  const MachineBasicBlock *Pred = MI.getOperand(i + 1).getMBB();

  BBInfo &PInfo = MBBInfoMap[Pred];
  if (PInfo.addRequired(Reg))
    todo.insert(Pred);
}
```

Notice that addRequired will return false if Reg isn't a virtual register, or if Reg already is in the required set. So the outer if-statement here seems redundant due to that.

Another question is if you need to do the `MRI->getVRegDef(Reg)->getParent() != FromMBB` check or not.
That would map to something like this

```
bb8:
  %t = phi i32 [ ... ], [ %y, %bb8 ]
  %y = add ...
  ...
  br i1 ..., %bb8
```

>From the LiveVariables verifications point of view we expect that vregsRequired should match AliveBlocks information. And afaict %bb8 won't be in AliveBlocks for %y (since %y is defined in %bb8), so it should not be in vregsRequired. However, since addRequired also checks regsLiveOut before adding anything to vregsRequired, that situation should be covered as well.





Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80274/new/

https://reviews.llvm.org/D80274