[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.

> This patch adds additional logic to add additional facts for A != B, if
> A is a monotonically increasing induction phi. The motivating use case
> for this is removing checks when using iterators with hardened libc++,
> e.g. https://godbolt.org/z/zhKEP37vG.

Looks like on trunk this doesn't emit bounds checks in the first place...



================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1166
+  if (Pred != CmpInst::ICMP_NE || !PN || PN->getNumIncomingValues() != 2)
+    return;
+
----------------
TODO: Handle phi on RHS? I don't think any canonicalization guarantees that it is on the LHS.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1187
+    auto *N = DT.getNode(Succ);
+    if (NumIn == N->getDFSNumIn() && NumOut == N->getDFSNumOut())
+      InLoopSucc = Succ;
----------------
Wonder whether it would be better to store the DTN in FactOrCheck rather than NumIn/NumOut.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1194
+      LI.getLoopFor(LoopExitSucc) == L)
+    return;
+
----------------
Generally, this whole "look at branch of single user of comparison" approach looks pretty hacky. This seems like something that should be done when initially queueing the fact, as we have direct knowledge of the branch structure at that point.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1222
+      DL.getIndexTypeSizeInBits(StepInst->getPointerOperand()->getType());
+  MapVector<Value *, APInt> StepVariableOffsets;
+  APInt StepOffset(BitWidth, 0);
----------------
Unused variable


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1235
+    unsigned UpperBitWidth =
+        DL.getIndexTypeSizeInBits(UpperGEP->getPointerOperand()->getType());
+    MapVector<Value *, APInt> UpperVariableOffsets;
----------------
Must be same as BitWidth?


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1250
+
+  // We know that PN != B, so if Start <= B we know that PN < B and PN >=
+  // StartValue.
----------------
Start -> StartValue


================
Comment at: llvm/test/Transforms/PhaseOrdering/iterator-with-runtime-check.ll:43
 ; CHECK-NEXT:    [[CMP_I_NOT:%.*]] = icmp eq ptr [[INCDEC_PTR_I]], [[ADD_PTR_I]]
 ; CHECK-NEXT:    br i1 [[CMP_I_NOT]], label [[COMMON_RET]], label [[FOR_BODY]]
 ;
----------------
I'm a bit confused on how this one ends up working. Here we have the condition on the latch, so it doesn't dominate for.body. How can we still eliminate a condition there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152730