[PATCH] D144927: [GVNHoist] don't hoist callbr users into the callbr's block

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:888
+    // Don't hoist into CallBr terminated blocks.
+    if (isa<CallBrInst>(TI))
+      continue;
----------------
void wrote:
> bjope wrote:
> > I think this is more restricted than needed. I mean, it could still be valid to hoist values into the CallBr terminated block if the hoisted instruction isn't using a value defined by the CallBr. That ofcourse also depends on if the CallBr might have other side effects or not. So ideally we would need to check if it is safe to hoist past the terminator and not only if it is safe to hoist to the end of BB.
> > 
> > It should at least be worth some more code comments here, explaining that we are conservative in this sense, to make a simple safe fix (while assuming that hoisting past a CallBr would be a quite rare opportunity anyway).
> With the recent changes allowing for valid outputs on the indirect branches, it could branch off somewhere with incorrect values. Or at the very least needlessly executed code. :-)
> I think this is more restricted than needed. I mean, it could still be valid to hoist values into the CallBr terminated block if the hoisted instruction isn't using a value defined by the CallBr. That ofcourse also depends on if the CallBr might have other side effects or not. So ideally we would need to check if it is safe to hoist past the terminator and not only if it is safe to hoist to the end of BB.
> 
> It should at least be worth some more code comments here, explaining that we are conservative in this sense, to make a simple safe fix (while assuming that hoisting past a CallBr would be a quite rare opportunity anyway).




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144927