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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers requested changes to this revision.
nickdesaulniers added a comment.
This revision now requires changes to proceed.

> The value returned by "callbr" is only valid on the "normal" path. Return

values used on "abnormal" paths are poisoned.

Please edit the revision comment to use the terms "fallthrough" and "indirect" targets or paths, rather than "normal" and "abnormal."



================
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
----------------
I still don't understand how the `poison` value gets added.   Can you please clarify, or maybe add a test case that does?


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


================
Comment at: llvm/lib/IR/BasicBlock.cpp:481
+/// from a "callbr" instruction.
+bool BasicBlock::isCallBrIndirectDest() const {
+  if (!hasAddressTaken())
----------------
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`?


================
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)
----------------
`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)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868