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

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

After working on the previously mentioned unreachable-block-end cleanup for a bit (not done yet), and thinking about this patch more, I actually think it's okay to go ahead with this change now. The potential problem I was worried about is that the code may think that if you have INLINEASM_BR in a basic-block, where the block-end is unreachable, and the block is followed by a basic-block which is one of the indirect targets of the INLINEASM_BR, the code will think the first block falls through, when it does not.

I'm no longer concerned about this now, for two reasons:

1. I currently _suspect_ it's not actually possible for this to happen, because of the way we restrict handling/merging blocks. It will never be created that way (because callbr is a terminator in IR), and I currently believe we'll never merge the blocks to create that situation, because inlineasm_br block has multiple successors. I am not certain it can never happen, but at least I've not been able to induce it to occur. (This relates to another worry I had previously -- that we might end up with a block that has both a Call instruction that could throw, and an inlineasm_br. Or, for that matter, two inlineasm_br in the same MachineBasicBlock. But, even though it's not a terminator in MI, I believe this will not occur.)
2. Even if there _is_ some way that might happen, the badness that might occur is limited. Assuming there's a possible fallthrough, when it is in fact impossible, and the block is only an exceptional successor, seems not likely be a problem. We may insert an extraneous jump upon block rearrangement, for example, but that would not be truly problematic.

So, I think this is ready for another round of review and then to submit.

Thanks!



================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:486
+  void setIsInlineAsmBrIndirectTarget(bool V = true) {
+    IsInlineAsmBrIndirectTarget = V;
   }
----------------
nickdesaulniers wrote:
> 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.
(we do actually have MachineBasicBlock::hasAddressTaken().)

I believe the change I have here resolves efriedma's concerns you've linked, by removing the mapping of source-block to target-block. It now overestimates, but should be conservatively correct, even in the face of instructions being moved/split/etc, because the _target_ cannot be so transformed.


================
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;
----------------
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.


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:288
 bool MachineBasicBlock::isLegalToHoistInto() const {
-  if (isReturnBlock() || hasEHPadSuccessor())
+  if (isReturnBlock() || hasEHPadSuccessor() || hasInlineAsmBr())
     return false;
----------------
void wrote:
> Is this correct? We should be able to hoist into a block with `INLINEASM_BR` if it's coming from the default target.
I think it's correct -- in that it returns false strictly more often than it needs to. Just the same as we could sometimes hoist into a block which has an invoke in it, we could sometimes do so for an inlineasm_br.


================
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();
----------------
nickdesaulniers wrote:
> Is this assignment ok to remove?
The code below which is invoked when LIP.second is set is simply to decide which of LIP.first or LIP.second to return. Letting it take the !LIP.second codepath has the same effect, with marginally less work.


================
Comment at: llvm/lib/Target/Hexagon/BitTracker.cpp:958
+  // FIXME: why do we need to recompute successors info in this function?
+  // Shouldn't successors() be fine as-is?
+  if (B.hasInlineAsmBr())
----------------
kparzysz wrote:
> It would defeat the purpose of this function.  It calculates the set of possible targets for each branch, given the updated register states (i.e. branch conditions), which can be a proper subset of the set of targets listed in the branch.
Thanks for the explanation.


================
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
----------------
nickdesaulniers wrote:
> INLINEASM_BR just disappears from this test?
I changed the test to a different mechanism of testing unanalyzable branch sequences, since INLINEASM_BR doesn't trigger the problem anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79794