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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Btw, I don't think that there is any useful SCEV sharing going on at the current pipeline position (did you see worse compile-time results prior to the phase ordering change?) I believe LPM1 does not use SCEV, only preserve it.



================
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) {}
 
----------------
May be cleaner to give `ConditionTy` a default constructor?


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:140
+              CmpInst::Predicate PrecondPred = CmpInst::BAD_ICMP_PREDICATE,
+              Value *PrecondA = nullptr, Value *PrecondB = nullptr)
+      : Cond(Pred, Op0, Op1), DoesHold(PrecondPred, PrecondA, PrecondB),
----------------
And then here accept `ConditionTy DoesHold = ConditionTy()` rather than unpacked form?


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


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


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:865
+
+  Type *StepTy = AR->getType();
+  const DataLayout &DL = BB.getModule()->getDataLayout();
----------------
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)`.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152730