[PATCH] D146699: [GuardWidening] Freeze the introduced use.

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:569
+// Return Instruction before which we can insert freeze for the value V.
+// If V is an instruction then next instruction is returned. Otherwise first
+// instruction in entry basic block is returned.
----------------
This is not true for Phis


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:570
+// If V is an instruction then next instruction is returned. Otherwise first
+// instruction in entry basic block is returned.
+// If there is no place to add freeze, return nullptr.
----------------
non-alloca? Better say `first available insertion point in the entry block...`


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:572
+// If there is no place to add freeze, return nullptr.
+static Instruction *getFreezeInsertPt(Value *V, DominatorTree &DT) {
+  if (auto *I = dyn_cast<Instruction>(V)) {
----------------
could be `const DominatorTree &DT`


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:576
+    // If there is no place to add freeze - return nullptr.
+    if (!Res || !DT.dominates(I, Res))
+      return nullptr;
----------------
How is this possible?


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:582
+          Instruction *User = cast<Instruction>(U);
+          return Res != User && DT.dominates(I, User) &&
+                 !DT.dominates(Res, User);
----------------
`I` by default should dominate all its users. What am I missing here?


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:588
+  }
+  return &*DT.getRoot()->getFirstNonPHIOrDbgOrAlloca();
+}
----------------
Maybe to reduce nest of code above, do
```
auto *I = dyn_cast<Instruction>(V);
if (!I)
  return &*DT.getRoot()->getFirstNonPHIOrDbgOrAlloca();
// code for instruction with 1 less nest
```


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:611
+    Instruction *I = dyn_cast<Instruction>(V);
+    if (!I || canCreateUndefOrPoison(cast<Operator>(I),
+                                     /*ConsiderFlagsAndMetadata*/ false)) {
----------------
Why we can always cast this to operator? What if it's not?


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

https://reviews.llvm.org/D146699