[PATCH] D53765: [RFC prototype] Implementation of asm-goto support in LLVM

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:505-507
+      // Similar to EH handling above, asm-goto targets allow jumping out of the
+      // middle of a basic block.
+      // FIXME: Is this sufficient?
----------------
craig.topper wrote:
> chandlerc wrote:
> > craig.topper wrote:
> > > chandlerc wrote:
> > > > Wait, they do?
> > > > 
> > > > I don't understand -- the callbr is always a terminator. I understand that it may expand to lots of instructions, but isn't that opaque at this level?
> > > This is MachineIR. CallBr isn't a thing there. CallBr was expanded to an INLINEASM instruction followed by an unconditional jump during SelectionDAGBuilder. The INLINEASM block is not a terminator so its effectively "the middle of a basic block". This pass tried to shove a RBP pop between the INLINEASM instruction and the unconditional jump. In the case that exposed the bug, the unconditional jump went to a block that also jumped to the same inline asm target. So as far as the dominator tree was concerned, the block containing the INLINEASM dominated the other blocks so it was chosen to hold the RBP pop. But since the INLINEASM itself can jump that wasn't correct.
> > Ah, that explains the thing I was missing...
> > 
> > I had assumed that we would expand this to an INLINEASM instruction that *is* a terminator. In MIR we can have multiple terminators at the end of a block, and this is a common pattern, so could we not have both instructions be marked as terminators and avoid special casing this?
> It looks like whether an instruction is a terminator is currently encoded in the MCInstrDesc entry corresponding to an opcode. So I think that means we would need a new flavor of INLINEASM opcode?
Bleh, yeah.

I think that's (marginally) cleaner, but I have no idea how much work it is to thread an `INLINEASM_BR` node through the backend. If it's too much, could defer this.

Reason that I think it is cleaner: I suspect we'll keep finding ways in which we mishandle this, but if it turns into a terminator sequence I think the rest of the backend will start to behave correctly.


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

https://reviews.llvm.org/D53765