[PATCH] D130290: [MachineVerifier] add checks for INLINEASM_BR

[Eli Friedman via Phabricator via llvm-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:904
+        report("INLINEASM_BR indirect target predecessor list missing parent",
+               &MO, i);
+    }
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > efriedma wrote:
> > > nickdesaulniers wrote:
> > > > efriedma wrote:
> > > > > I don't think you need to check both isSuccessor and isPredecessor; we should have other checks that verify the predecessor lists are consistent with the successor lists.
> > > > If you comment out these checks, then run `llc -run-pass=none -verify-machineinstrs /android0/llvm-project/llvm/test/MachineVerifier/verify-inlineasmbr.mir -o -`, you'll find that MachineVerify makes not such qualms about inconsistent predecessor or successor lists, despite BOTH being wrong.
> > > All I'm trying to say is that if you try to construct a testcase where the isSuccessor check succeeds, but the isPredecessor check fails, you'll get some other error.
> > I suspect the verifier will spot other things wrong later during its analysis, but if either list is corrupt, I feel strongly that we should eagerly diagnose that ASAP.  It makes for a significantly improved debugging experience if the verifier spots corrupted pred/succ lists ASAP.
> I was thinking more about your point last night; if I may restate what I perceive to be the distinction another way.
> 
> Your latest point sounds like if ONE of either the predecessor list or successor list in incorrect, the MachineVerifier will catch that.
> 
> Granted.
> 
> But what happens if BOTH are wrong, simultaneously?
> 
> Because that is precisely what happened in D129997. The MachineVerifier did not catch that.
> 
> For MachineInstrs that imply control flow, we should verify that their parent MachineBasicBlock's successor list contains the MachineInstrs MachineBasicBlock operand AND that operand's predecessor list contains this MachineInstrs parent MachineBasicBlock.  This patch does such checking only for the rare cases of INLINEASM_BR, but I'd go so far as to suggest this should be done for ALL branching instructions (or maybe any instruction that has a MachineBasicBlock as an operand).
> 
> Stated a third way, if MBB A has a jump to MBB B, we should check that:
> 1. B is in A's successor list.
> 2. A is in B's predecessor list.
> 
> It _might_ be that MachineVerifier can catch imbalanced cases where ONE of the above does not hold, but currently, if BOTH don't hold MachineVerifier misses that.
> 
> Removing an edge between A and B without also removing the branching instruction in A (where that instruction branches to B) is a great way to get into the above anomalous conditions, which is what happened in D129997.
It's not really a big deal either way, in any case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130290