[PATCH] D79794: Change the INLINEASM_BR MachineInstr to be a non-terminating instruction.

[James Y Knight via Phabricator via llvm-commits]

jyknight marked 14 inline comments as done.
jyknight added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:474
+  /// in the function).
+  bool hasInlineAsmBr() const;
+
----------------
nickdesaulniers wrote:
> Sorry to bikeshed the name of this method, but I find it currently doesn't match the implementation well.
> 
> I'd take `hasInlineAsmBr` to mean "this `MachineBasicBlock` has an `INLINEASM_BR` `MCInst`."  Instead, the implementation is checking whether any of the successors of this `MachineBasicBlock` is the indirect target of an `INLINEASM_BR` `MCInst`.  Those two aren't the same thing.  In fact, you could have a `MachineBasicBlock` where the current implementation of `hasInlineAsmBR` would return `true`, and yet the `MachineBasicBlock` does not actually contain a `INLINEASM_BR` `MCInst`.
> 
> I know that's what you're alluding to in the comment, but I think the method should be named `hasInlineAsmBrSuccessors` or something of the sort.  Maybe `MaybeHasInlineAsmBr` since it sounds like you'd rather it not be precise by scanning all instructions for the relevant opcode?  (Couldn't ISEL mark these when creating the MBB? I guess having a `bool` member is tricky, since transforms would have to update that correctly.  In this case, rematerializing the value by rescanning is simpler, and harder to get wrong.)
> 
> I guess the name sounds precise, though the comment and implementation denote that it's not, and that's what I'm most hung up on.
Renamed to mayHaveInlineAsmBr. I don't like "hasInlineAsmBrSuccessors", because the important property is whether there is such an instruction in the block. That it currently tests via looking at the successor in the current implementation is just an implementation detail.

It would be nice for it to be precise, but as I mentioned in the other thread, it's probably not really worth the expense.


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:1715
-            !SuccPrev->canFallThrough() && !CurUnAnalyzable &&
-            !SuccBB->isEHPad()) {
-          MBB->moveBefore(SuccBB);
----------------
nickdesaulniers wrote:
> was removing the `!isEHPad` check intentional?
Yes, no longer need to filter out EHpads, because it's now only looking at the analyzeBranch results CurFBB/CurTBB, not all successors.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:588
           MBB->getIterator() != MBB->getParent()->begin()) {
         report("MBB has allocatable live-in, but isn't entry or landing-pad.", MBB);
         report_context(LI.PhysReg);
----------------
nickdesaulniers wrote:
> should the report string be updated, too, to mention `INLINEASM_BR fallthrough/default target`?
Maybe it should've before, but this change _removed_ the allowance for that.


================
Comment at: llvm/lib/CodeGen/SplitKit.cpp:110
       return LIP.first;
-    // There may not be a call instruction (?) in which case we ignore LPad.
-    LIP.second = LIP.first;
     for (MachineBasicBlock::const_iterator I = MBB.end(), E = MBB.begin();
          I != E;) {
----------------
nickdesaulniers wrote:
> I wonder why we don't use a reverse const iterator here?
Who knows. =) Changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79794