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

[Bill Wendling via Phabricator via llvm-commits]

void added inline comments.


================
Comment at: llvm/docs/LangRef.rst:7157
+Outputs of a '``callbr``' instruction are valid only on the '``fallthrough``'
+path.  Use of outputs on the '``indirect``' path(s) results in :ref:`poison
+values <poisonvalues>`. Care should be taken if the '``fallthrough``' block is
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > I still don't understand how the `poison` value gets added.   Can you please clarify, or maybe add a test case that does?
> If the outputs are only valid on the fallthrough, will that allow us to build the extension to the existing `asm goto` that has been asked for?  I worry that if we implement this restriction, it may not be fully useful unless the outputs on all paths are valid.  Do you have a link to the previous discussion about this?
> I still don't understand how the poison value gets added. Can you please
> clarify, or maybe add a test case that does?

Poison values are a result of execution, not specified directly. So during execution, the value becomes poison if used in the indirect block, but not if used in the fallthrough. It's just fancy language for saying "don't do that!" :-)

>From the docs:

  Poison value behavior is defined in terms of value dependence:
    ...
    * An instruction control-depends on a terminator instruction if the terminator instruction has multiple successors and the instruction is always executed when control transfers to one of the successors, and may not be executed when control is transferred to another.


================
Comment at: llvm/docs/LangRef.rst:7157
+Outputs of a '``callbr``' instruction are valid only on the '``fallthrough``'
+path.  Use of outputs on the '``indirect``' path(s) results in :ref:`poison
+values <poisonvalues>`. Care should be taken if the '``fallthrough``' block is
----------------
void wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > I still don't understand how the `poison` value gets added.   Can you please clarify, or maybe add a test case that does?
> > If the outputs are only valid on the fallthrough, will that allow us to build the extension to the existing `asm goto` that has been asked for?  I worry that if we implement this restriction, it may not be fully useful unless the outputs on all paths are valid.  Do you have a link to the previous discussion about this?
> > I still don't understand how the poison value gets added. Can you please
> > clarify, or maybe add a test case that does?
> 
> Poison values are a result of execution, not specified directly. So during execution, the value becomes poison if used in the indirect block, but not if used in the fallthrough. It's just fancy language for saying "don't do that!" :-)
> 
> From the docs:
> 
>   Poison value behavior is defined in terms of value dependence:
>     ...
>     * An instruction control-depends on a terminator instruction if the terminator instruction has multiple successors and the instruction is always executed when control transfers to one of the successors, and may not be executed when control is transferred to another.
After going back and forth with James and Hal, I now believe that having the value valid only on the fallthrough path will be sufficient for most purposes (if not all). However, this implementation doesn't restrict us from relaxing it in the future, but I would like to see a use case for that before we sink a lot of time into it.

Below is a link to the start of the thread.

http://lists.llvm.org/pipermail/llvm-dev/2019-June/133428.html


================
Comment at: llvm/lib/IR/BasicBlock.cpp:481
+/// from a "callbr" instruction.
+bool BasicBlock::isCallBrIndirectDest() const {
+  if (!hasAddressTaken())
----------------
nickdesaulniers wrote:
> In the past, I've been shot down for adding to `BasicBlock`. Can this be implemented as a method on a `CallBrInst` that accepts a `BasicBlock`?
Better, I'll just inline it. :-)


================
Comment at: llvm/lib/IR/BasicBlock.cpp:487
+       PI != PE; ++PI)
+    if (const auto *CB = dyn_cast<CallBrInst>((*PI)->getTerminator()))
+      for (unsigned I = 0, E = CB->getNumIndirectDests(); I != E; ++I)
----------------
nickdesaulniers wrote:
> `PI` is only iterating `BasicBlocks` in this `Function`, right? Can't `callbr` refer to labels in other functions (unlike `asm goto` in C, due to scoping)?
I don't believe it can refer to labels in other functions. Anyway, I changed this so that this function is gone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868