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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


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


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:1115-1116
+  if (Succ->hasAddressTaken())
+    if (auto *bb = Succ->getBasicBlock())
+      if (auto *cbr = dyn_cast<CallBrInst>(getBasicBlock()->getTerminator()))
+        if (cbr->getDefaultDest() != bb)
----------------
I think it may be worthwhile to swap these conditions with each other, too.  It's highly unlikely the BB is terminated with a `CallBrInst`, yet highly likely the MBB refers to a BB.


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:1118-1119
+        if (cbr->getDefaultDest() != bb)
+          for (unsigned i = 0, e = cbr->getNumIndirectDests(); i != e; ++i)
+            if (cbr->getIndirectDest(i) == bb)
+              return false;
----------------
please use a range-based for:
```
for (const BasicBlock* ID : cbr->getIndirectDests())
  if (ID == bb)
```
or `llvm::any_of`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868