[PATCH] D69868: Allow "callbr" to return non-void values
Nick Desaulniers via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 17 14:56:41 PST 2019
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
More information about the llvm-commits
mailing list