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

Tue Jan 29 18:25:43 PST 2019

chandlerc added a comment.

Looking at updated patch and some of the optimizer bits, but some inline responses to discussion points below:

In D53765#1375547 <https://reviews.llvm.org/D53765#1375547>, @craig.topper wrote:

> In D53765#1375112 <https://reviews.llvm.org/D53765#1375112>, @chandlerc wrote:
>
> > Sorry it took so long, but made a full pass through this. Just want to point out that this patch is already in really great shape due to the huge amount of work from the original author, Craig taking it over, and the multiple rounds of review from Bill and Nick among others.
> >
> > Most of my comments are pretty superficial and not hard to address I suspect. I have one big question and one big comment. I've referenced these in-line below as I went through the code but wanted to call them out at a highlevel:
> >
> > 1. Why do we need the block address machine operand structure? I'm not saying to get rid of it (yet), I just don't understand the need to support both it and direct MBB operands.
>
>
> Maybe I went the wrong way on this, but here's what I saw. The existence of the BlockAddress instruction in IR caused some portion of MachineIR handling to know that it needed to create a label of the form .Ltmp[0-9]+. This is helpfully annotated in the output assembly as "block address taken" and sometimes when there's a bug "address taken of block removed by CodeGen". The first version of this patch I inherited stripped the BlockAddress when the SelectionDAG was created. The resulting assembly would then print the .LBB label for the block in the inline asm instead. We ended up in a case where the LBB label didn't exist in the final output, but the Ltmp did. This failed to assemble obviously. Maybe that was really pointing to passes like BranchFolding.cpp doing bad things which we encountered later. But at the time I tried to fix it so that the inline asm printing would reference the .Ltmp label because that seemed like the label we really wanted.

I've spent more time thinking about this. I think the difference here is the following... With the "block address" behavior we assume the address more than taken -- it is *captured* and its uses may be unanalyzable. As a consequence, it *cannot* be updated when we do things like rework the control flow.

On the flip side, the MBB operands are likely often assumed to be easily enumerated and updated when we're rewriting the control flow, and so the fact that we can't do that results in breakages.

At least, this is my understanding. Match yours?

Given this, I think the current "block address" stuff may be correct -- I don't see anything that indicates the asm-goto couldn't escape or even capture the addresses of its successors and then re-use them in some way, expecting them to be "real" addresses. If this also makes sense to you, likely good to add some comments for future readers.

> 
> 
>> 2. Why do we need to special case asm-goto targets in MI? I feel like this should somewhat fall out of the handling of address-taken basic blocks that can be jumped to somewhat opaquely. It would be really nice if there were a relatively common way of handling these rather than special casing the targets of asm-goto everywhere when it feels like the fundamental special treatment isn't actually about asm-goto at all. But maybe I've missed the critical thing that actually makes it special to asm-goto. If so, just help mme spot it1 =D
> 
> Maybe it shouldn't be special to asm-goto. But its clear that we've already been treating exception handling pads specially, but not address-taken blocks so I've just been special casing asm goto to avoid changing anything about existing address taken. I'm not sure what we do for indirect branches, but I suspect analyzeBranch returning unanalyzable for indirect jumps is part of it. But that doesn't work for asm goto because our terminator is a direct jump which is analyzable.

Ugh, yeah.

I think that we should consolidate all of this behind something like address-taken, but I think that can be follow-up work. I suspect that a decent number of the things you've fixed here are actually latent bugs w/ indirectbr and address-of-label extensions that we've just never managed to hit. =[[[[

But I think it's fine to fix that incrementally going forward by digging in and commoning these representations as much as possible.

================
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:
> > 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?

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

https://reviews.llvm.org/D53765