[PATCH] D77523: Add CanonicalizeFreezeInLoops pass

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert 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;
----------------
aqjune wrote:
> 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.
Sure, a freeze inst, a phi node, a binary operator and a value. You have to see `Value *StepValue = std::get<3>(Candidate);` to guess what the value is. If you want to access the phi (as an Instruction or auto) you have to go back to the vector definition to determine the index. These are (IMHO) good reason against a tuple. Are there any benefits?


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:171
+      SE.forgetValue(PN);
+    }
+
----------------
aqjune wrote:
> 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.
I see.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77523