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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 4 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1195
+        StepInst->getOperand(1) ==
+            ConstantInt::get(StepInst->getOperand(1)->getType(), 1)) {
+      StartValue = PN->getIncomingValue(1 - I);
----------------
nikic wrote:
> Does this check for step 1 still make sense, given the more generic checks below?
> 
> Also, aren't we missing a check here that the GEP pointer operand is the PN? I think right now it can be unrelated...
> Does this check for step 1 still make sense, given the more generic checks below?

Not needed any longer, removed, thanks!

> Also, aren't we missing a check here that the GEP pointer operand is the PN? I think right now it can be unrelated...

Yep, added the check. The original test cases I had for that case didn't show the issue during verification, but added a couple of additional ones.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1213
+                               StepConstantOffset) ||
+      !StepVariableOffsets.empty())
+    return;
----------------
nikic wrote:
> This looks like a complicated way to write accumulateConstantOffset().
Updated to use `accumulateConstantOffset`, thanks!


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1218
+    auto *UpperGEP = dyn_cast<GetElementPtrInst>(B);
+    if (!UpperGEP || UpperGEP->getPointerOperand() != StartValue)
+      return;
----------------
nikic wrote:
> I think you need to require inbounds here as well. Otherwise, for non-power-of-two steps, you don't have a guarantee that the end value will be hit without wrapping (just that it is hit eventually).
Agreed, added the check. I don't think there's a way to add a test that triggers a miscompile because missing `inbounds` will prevent any further analysis of the GEP in the pass.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1238
+    if (!UpperScale.urem(StepConstantOffset).isZero())
+      return;
+  }
----------------
nikic wrote:
> A cleaner (and more general) way to write this would be that all the scales/offset must be divisible by StepConstantOffset.
Yep, generalizing it also results in simpler code overall, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152730