[PATCH] D152730: [ConstraintElim] Add A < B if A is an increasing phi for A != B.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:131
+      : Inst(Inst), DoesHold(CmpInst::BAD_ICMP_PREDICATE, nullptr, nullptr),
+        NumIn(DTN->getDFSNumIn()), NumOut(DTN->getDFSNumOut()), Ty(Ty) {}
 
----------------
nikic wrote:
> May be cleaner to give `ConditionTy` a default constructor?
Done, thanks!


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:832
+  if (!PN) {
+    std::swap(A, B);
+    PN = dyn_cast<PHINode>(A);
----------------
nikic wrote:
> `Pred = CmpInst::getSwappedPredicate(Pred)` to avoid a miscompile if this function every handles non-equality predicates...
Adjusted, thanks!


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:840
+
+  BasicBlock *Succ = nullptr;
+  if (Pred == CmpInst::ICMP_NE)
----------------
nikic wrote:
> Succ -> InLoopSucc
Renamed, thanks!


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:865
+
+  Type *StepTy = AR->getType();
+  const DataLayout &DL = BB.getModule()->getDataLayout();
----------------
nikic wrote:
> This is not the StepTy. If you used the actual StepTy here (i.e. the type of getStepRecurrence) then you would not need the pointer special case.
> 
> Alternatively you could also use `SE.getTypeSizeInBits(AR)`.
Turns it this doesn't seem to be needed in the current version, as we just assign the step-recurrence's APInt below, removed


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:908
+    WorkList.push_back(
+        FactOrCheck::getConditionFact(DTN, CmpInst::ICMP_UGE, PN, StartValue));
+  } else {
----------------
nikic wrote:
> If I understood right, for this case we don't actually need the additional gep proof above?
Yep, move this up, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152730