[PATCH] D106041: [SimpleLoopUnswitch] Re-fix introduction of UB when hoisted condition may be undef or poison

[Philip Reames via Phabricator via llvm-commits]

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

See inline review comments.  Overall intent of my suggestions is to cut away everything needing further discussion so that we can get the simplest patch in tree (off by default) and then revisit pieces needing discussion in separate reviews.



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2217
+          auto FrozenCond = new FreezeInst(Cond, Cond->getName() + ".fr");
+          if (dyn_cast<Instruction>(Cond)) {
+            if (PHINode *PN = dyn_cast<PHINode>(Cond))
----------------
This block of code appears to be re-implementing LICM, and it's not clear we should do this here at all.  Please refresh the patch to place the freeze unconditionally before the branch, and then revisit this piece in a separate review with it's own justification.  


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2249
 
+      if (InsertFreeze) {
+        auto Cond = SI->getCondition();
----------------
xbolva00 wrote:
> Consider new helper function
If we keep the code, agreed.  See also comment on previous copy of code, also applies here.  


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2449
+
+        ReplaceIfDominated(U);
+
----------------
The use of the lambda here is NFC, the code change is reasonable on it's own (without the addition), please pull out and submit separately (without additional review) to simplify this patch.  


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2451
+
+        // If V is invariant, Freeze(V) is also invariant. As we try to replace
+        // the use of V to constant, we need to try replace the use of Freeze(V)
----------------
Do you mind moving this to a separate patch?  I think we can do better than this.  Specifically, for the case where V is a constant, *many* instructions constant fold and thus we could possibly do something for all users which happens to constant fold freeze without special magic.  


================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-freeze.ll:1
+; RUN: opt -freeze-loop-unswitch-cond -passes='loop(simple-loop-unswitch<nontrivial>),verify<loops>' -S < %s | FileCheck %s
+; RUN: opt -freeze-loop-unswitch-cond -passes='loop-mssa(simple-loop-unswitch<nontrivial>),verify<loops>' -S < %s | FileCheck %s
----------------
Please use ./utils/update_test_checks.py


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106041