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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers added a comment.

The only other concern I have is whether we have enough test coverage of the scheduling boundary changes?  (Does removing the added checks there cause any existing or new test from the change to file?  If not, seems like a lack of test coverage.) In the case of `PostRASchedulerList`, that implies a MIR test (writing MIR tests isn't something I'd wish on my worst enemy though).



================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:474
+  /// in the function).
+  bool hasInlineAsmBr() const;
+
----------------
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.


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:1715
-            !SuccPrev->canFallThrough() && !CurUnAnalyzable &&
-            !SuccBB->isEHPad()) {
-          MBB->moveBefore(SuccBB);
----------------
was removing the `!isEHPad` check intentional?


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:281-284
+  for (const_succ_iterator I = succ_begin(), E = succ_end(); I != E; ++I)
+    if ((*I)->isInlineAsmBrIndirectTarget())
+      return true;
+  return false;
----------------
jyknight wrote:
> void wrote:
> > nickdesaulniers wrote:
> > > ```
> > > return any_of(successors(), [](MachineBasicBlock* Succ) {
> > >   return Succ->isInlineAsmBrIndirectTarget(); };
> > > ```
> > > 
> > > or better yet, why not be precise and iterate `terminators()` checking the `getOpcode() == TargetOpcode::INLINEASM_BR`?
> > It's no longer a terminator, but could be written:
> > 
> > ```
> > return any_of(*this, [](const MachineInstr &MI) {
> >   return MI.getOpcode() == TargetOpcode::INLINEASM_BR;
> > }
> > ```
> I'm worried that iterating over all the instructions in the block to retrieve this information would be bad for performance. 
> 
> I don't think the overestimating here is likely to be a major optimization issue, but if it turns out to be, I'd like to revisit in a future patch.
This could at least use a range-for:
```
for (const MachineBasicBlock *Succ : successors()) {
  if (Succ->isInlineAsmBrIndirectTarget())
    ...
```


================
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);
----------------
should the report string be updated, too, to mention `INLINEASM_BR fallthrough/default target`?


================
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;) {
----------------
I wonder why we don't use a reverse const iterator here?


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:2020
+  if (MI.getOpcode() == TargetOpcode::INLINEASM_BR)
+    return true;
+
----------------
I think there's one more `override` of this method that was missed: `AArch64InstrInfo::isSchedulingBoundary` in `llvm/lib/Target/AArch64/AArch64InstrInfo.cpp`? Ah, nvm it defers to `TargetInstrInfo::isSchedulingBoundary` first.


================
Comment at: llvm/test/CodeGen/X86/callbr-asm-outputs.ll:33
 
 define i32 @test2(i32 %out1, i32 %out2) {
 ; CHECK-LABEL: test2:
----------------
@void 's [earlier comments](https://reviews.llvm.org/D79794#2105655) mentioned modifications to this test case's `movl $-1, %eax`. Are those still needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79794