[PATCH] D69868: Allow "callbr" to return non-void values

Fri Jan 3 15:38:48 PST 2020

rnk added subscribers: MaskRay, echristo.
rnk added a comment.

More reviewers: + at echristo @MaskRay

================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:133
+  /// Indicate that this basic block is the indirect dest of an INLINEASM_BR.
+  bool IsInlineAsmBrIndirectPad = false;
+
----------------
I think "IsInlineAsmBrIndirectTarget" would be better. I think of "Pad" as being something EH related, it's usually a "landing pad" or "eh pad".

================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:1114
+  // Don't do it in this generic function.
+  if (auto *bb = Succ->getBasicBlock())
+    if (auto *cbr = dyn_cast<CallBrInst>(getBasicBlock()->getTerminator()))
----------------
rnk wrote:
> As a minor efficiency golf thing, I would start off this chain of checks with `if (Succ->hasAddressTaken())`. In most cases, which will handle splitting the critical edge to the fall through destination, which will not be marked as address taken.
Somehow I missed that you added isInlineAsmBrIndirectPad, you could use that here instead of hasAddressTaken, and it would be more precise.

================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:1115
+  if (auto *bb = Succ->getBasicBlock())
+    if (auto *cbr = dyn_cast<CallBrInst>(getBasicBlock()->getTerminator()))
+      if (cbr->getDefaultDest() != bb)
----------------
void wrote:
> rnk wrote:
> > This code really should be looking at the machine IR to know if there is a callbr. I think INLINEASM_BR is target independent, so you really can just loop through the MachineInstrs looking for it, and then look for the MBB in the operand list. I wonder if we should have a flag on the MBB to indicate that it ends in an INLINEASM_BR, since those break the MIR invariant that terminators are grouped together at the end of the block.
> The problem is that I need to between the indirect branches and the default one from the `callbr` instruction. It's way more cumbersome to do that with the `INLINEASM_BR` instruction. If you feel strongly about it I'll make the change, but the `INLINEASM_BR` instruction is already strongly tied to `callbr`. Do you expect it to change anytime in the future?
I don't think it will actually be that hard to implement. The MIR already knows a few things:
- After block layout, MBB->getFallthrough() will return the default destination. If non-null, you can use it.
- If getFallthrough() is null and this block terminated in callbr in IR, there must be an analyzable unconditional branch terminator (JMP or B).
- All other address taken successors must come from abnormal control flow, i.e. exception handling or callbr.

Actually, I'm surprised this utility doesn't return false after analyzeBranch below if Succ is not one of TBB or FBB.

In any case, if these ideas don't work out, I don't feel that strongly, but I feel obligated to ask you to try a variety of ways to make this work with just MIR info.

================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:1116
+    if (auto *cbr = dyn_cast<CallBrInst>(getBasicBlock()->getTerminator()))
+      if (cbr->getDefaultDest() != bb)
+        for (unsigned i = 0, e = cbr->getNumIndirectDests(); i != e; ++i)
----------------
xbolva00 wrote:
> void wrote:
> > nickdesaulniers wrote:
> > > void wrote:
> > > > rnk wrote:
> > > > > Is it possible for a BB to be both an indirect successor and a fallthrough successor? I suppose that could be the case with the Linux macro that gets the current PC.
> > > > > 
> > > > > In any case, it's probably safe to remove this condition, and then we don't have to worry.
> > > > It is possible. I have a testcase for it in this patch. :-)
> > > But you don't have a test case for what @rnk is asking about. (Unsure if it would be necessary).
> > > 
> > > I think @rnk is getting at this case from C:
> > > 
> > > ```
> > > void foo(void) {
> > >   asm goto ("#NICK":: "r"(&&hello) :: hello);
> > > hello:
> > >   return;
> > > }
> > > ```
> > > Clang will emit this as:
> > > ```
> > > define dso_local void @foo() #0 {
> > > entry:
> > >   callbr void asm sideeffect "#NICK", "r,X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@foo, %hello), i8* blockaddress(@foo, %hello)) #1
> > >           to label %asm.fallthrough [label %hello], !srcloc !2
> > > 
> > > asm.fallthrough:                                  ; preds = %entry
> > >   br label %hello
> > > 
> > > hello:                                            ; preds = %asm.fallthrough, %entry
> > >   ret void
> > > }
> > > ```
> > > ie. the `blockaddress` is passed twice, once as the address of a label (GNU C extension), once as the indirect destination of the `asm goto`.
> > > 
> > > So to @rnk 's question:
> > > > Is it possible for a BB to be both an indirect successor and a fallthrough successor?
> > > It is valid LLVM IR to have a BB be both; Clang today (or with https://reviews.llvm.org/D69876) will not emit such formation (but could).
> > > 
> > > ie. imagine the above:
> > > 
> > > >  callbr void asm sideeffect "#NICK", "r,X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@foo, %hello), i8* blockaddress(@foo, %hello)) #1
> > > >           to label %asm.fallthrough [label %hello], !srcloc !2
> > > 
> > > to instead be:
> > > 
> > > >  callbr void asm sideeffect "#NICK", "r,X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@foo, %hello), i8* blockaddress(@foo, %asm.fallthrough)) #1
> > > >           to label %asm.fallthrough [label %asm.fallthrough], !srcloc !2
> > > 
> > > I still don't understand @rnk 's point about
> > > > In any case, it's probably safe to remove this condition, and then we don't have to worry.
> > > though.
> > I meant that I had a testcase that required the conditional he suggested I remove. Sorry for the confusion.
> > 
> > You're right that we can generate the clang code where the fallthrough is the same as the indirect. I didn't mean to imply that it wasn't the case. I can add a testcase. I asked a question on the "cfe-dev" mailing list to determine how I could identify the "fallthrough" block from the CFG. So far no one has responded. Until I can determine that, I won't be able to handle that properly with this conditional.
> Maybe @aaron.ballman can answer your question related to “fallthrough”  block..
I had thought that it would be safe to return false (don't allow this edge to be split) if BB appears in the indirect destination list, even if it's also the default destination, so this condition could be reduced to `contains(cbr->getIndirectDests(), bb)`. I looked for a test where a BB is both a default and indirect destination, so I don't see why we can't remove the default dest check.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868