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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


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


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


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


================
Comment at: llvm/test/CodeGen/X86/callbr-asm-outputs.ll:1
-; RUN: not llc -mtriple=i686-- < %s 2> %t
-; RUN: FileCheck %s < %t
+; RUN: llc -mtriple=i686-- < %s | FileCheck %s
 
----------------
Please add -verify-machineinstrs to the tests, since that isn't usually on by default, and it may highlight some latent verifier issues I don't know about.


================
Comment at: llvm/test/CodeGen/X86/callbr-asm.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=i686-- -O3 | FileCheck %s
 
----------------
Please add -verify-machineinstrs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868