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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

An interesting approach.  It certainly seems to simplify some of the special handling.  I'm happy that it seems to delete much of the existing code.  Thank you for taking the time to write up this patch.

I don't fully understand what "shrink wrapping" is, or the changes to BranchFolding, but the rest of the patch looks pretty good to me.  This obviously has implications for `asm goto` without outputs, so I'd like to run this through a couple kernel builds to ensure we have regressed anything.  From there, I can test kernel builds that use outputs.

Three other things to check:

1. check we don't spill post terminators (https://reviews.llvm.org/D78166).  Probably no issue there.
2. check that `BranchFolder::RemoveDeadBlock` isn't removing any `MachineBasicBlock` that have their address taken.  (https://reviews.llvm.org/D78234) tries to do this, but we've seen cases where `asm goto` w/ outputs results in the indirect successor being removed by BranchFolder.  We have a case from the kernel that triggered the above two patches, and is still a problem for https://reviews.llvm.org/D75098 that I can send you.
3. That live ins match live outs (https://reviews.llvm.org/D78586).  Probably no issue there.



================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:486
+  void setIsInlineAsmBrIndirectTarget(bool V = true) {
+    IsInlineAsmBrIndirectTarget = V;
   }
----------------
I think @efriedma has been noting that the API at the MIR level doesn't feel symmetric with the LLVM IR level.

https://reviews.llvm.org/D78234#1987870 and
https://reviews.llvm.org/D78234#1989382

In LLVM IR, you have `BasicBlock::HasAddressTaken`, but at the MIR level the operands are still `BlockAddress` (which reference a `Function` and `BasicBlock`, two LLVM IR level concepts).  It's too bad we don't lower these to just `MachineBasicBlocks` (or a new `MachineBlockAddress`) as operands, and have equivalent machinery for detecting whether a `MachineBasicBlock` has its address taken.


================
Comment at: llvm/include/llvm/Target/Target.td:1023
+//  let isBranch = 1;
+//  let isIndirectBranch = 1;
 }
----------------
Delete


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:1705
+      if (!CurUnAnalyzable) {
+        for (MachineBasicBlock *SuccBB : {CurFBB, CurTBB}) {
+          if (!SuccBB)
----------------
That's neat, I didn't know you could use initializer lists as ranges for range based for loops.s


================
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;
----------------
```
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`?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:410
+  std::function<void(SelectionDAGBuilder *self, SDValue Chain, SDValue Flag)>
+      DeferredInlineAsmOutputCallback;
+
----------------
Unused?


================
Comment at: llvm/lib/CodeGen/SplitKit.cpp:100
-    // 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();
----------------
Is this assignment ok to remove?


================
Comment at: llvm/lib/Target/Hexagon/BitTracker.cpp:959-960
+  // Shouldn't successors() be fine as-is?
+  if (B.hasInlineAsmBr())
+    DefaultToAll = true;
+
----------------
```
DefaultToAll |= B.hasInlineAsmBr();
```


================
Comment at: llvm/test/CodeGen/ARM/ifcvt-diamond-unanalyzable-common.mir:66
+    t2Bcc %bb.4, 1, $cpsr
+    t2Bcc %bb.4, 2, killed $cpsr
     t2B %bb.3, 14, $noreg
----------------
INLINEASM_BR just disappears from this test?


================
Comment at: llvm/test/CodeGen/X86/callbr-asm-outputs.ll:60
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    movl $-1, %eax
+; CHECK-NEXT:  .LBB1_3:
----------------
does this `-1` get overwritten immediately on the next instruction?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79794