[PATCH] D77523: Add CanonicalizeFreezeInLoops pass

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:71
+  // Maintains (freeze inst, ind var, stepping instruction, step value)
+  SmallVector<std::tuple<FreezeInst *, PHINode *, BinaryOperator *, Value *>, 4>
+      Candidates;
----------------
jdoerfert wrote:
> Is there a good reason not to make this a struct with named members so the values and use sites have meaning?
Tuple was used because its types seemed to represent what they stand for.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:125
+        if (L->contains(StepBB)) {
+          // Step instruction is in the body. Freezing this can possibly block
+          // SCEV.
----------------
efriedma wrote:
> It looks like isAuxiliaryInductionVariable checks that the step is loop-invariant?
A semantically loop invariant instruction may reside in a loop. For example, if it is division of two loop-invariant variables, it is also loop invariant but cannot be hoisted.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:129
+        }
+      }
+      Candidates.emplace_back(FI, PN, SteppingInst, Step);
----------------
jdoerfert wrote:
> Shouldn't we teach SCEV about freeze instead?
I think we have two kinds of analyses conceptually: must-be analysis (any program execution should satisfy the analysis result) and may-be analysis (there exists an execution that satisfies the result). SCEV analysis seems to satisfy both.
In case of freeze, if SCEV result of freeze(val) is defined as SCEV of val, must-be analysis becomes problematic; freeze(val) can be any value if val was poison. So, teaching SCEV about freeze may not fully recover precision in general.



================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:171
+      SE.forgetValue(PN);
+    }
+
----------------
jdoerfert wrote:
> Do we handle the case where both are `GuaranteedNotToBeUndefOrPoison` somewhere else? In that case we don't need to drop the poison generating flags, right?
It can be poison if summation overflowed. But it seems SteppingInst can be non-poison in certain case (e.g. it was used by divison; it is UB if poison), so added GuaranteedNotToBeUndefOrPoison check on SteppingInst.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77523