[PATCH] D79793: Simplify MachineVerifier's block-successor verification.

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:691
+    // analyzeBranch.
+    if (TBB && !MBB->isSuccessor(TBB))
+      report("MBB exits via jump or conditional branch, but its target isn't a "
----------------
arsenm wrote:
> nickdesaulniers wrote:
> > arsenm wrote:
> > > Braces
> > Why? The body is single statement.
> Spanning multiple lines
I'm pretty sure that's not an llvm style rule, so I'm going to leave it as is. All the existing code here is like that.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:708
+    // be a successor, because the block might end in unreachable.)
+    if (Fallthrough && !Cond.empty()) {
+      MachineFunction::const_iterator MBBI = std::next(MBB->getIterator());
----------------
nickdesaulniers wrote:
> It's weird seeing the `bool && <sub expr that made up the bool>`.  Maybe
> ```
> if (!Cond.empty() && !FBB) {
> ```
> Then you could lower the declaration of `Fallthrough` closer to its use below?
I like the first half of your suggestion, so done. But I think the description flows better to leave the definition of fallthrough where it is.


================
Comment at: llvm/test/CodeGen/WebAssembly/eh-labels.mir:32
     EH_LABEL <mcsymbol .Ltmp1>
+    BR %bb.2, implicit-def dead $arguments
 
----------------
jyknight wrote:
> aheejin wrote:
> > In this case, the next BB is an EH pad where we unwind to in case `@foo` throws, so the bb.2 becomes a fallthrough. Does this patch not handle this case?
> Sorry, I don't follow. There's no magic behavior of "just skip over ehpad blocks and fallthrough into the next block after it."
> 
> bb.0 says it falls through into bb.2, but the instruction stream and block layout shows a fall-through into bb.1. That's now properly detected as invalid by the verifier, but was missed before.
> 
> Normally, blocks would be placed, so as not to have EHPad blocks in the middle of a potential fallthrough (e.g. moving bb.1 to the end of the function), but given the block placement specified in this test, there should be an explicit jump around the ehpad block.
> 
(Or alternatively, if @foo was a noreturn call, then bb.2 should not be marked as a successor of bb.0)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79793