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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1122
+  Info.addFactForInductionPhi(cast<CmpInst>(And->getOperand(0)), Pred, A, B,
+                              CB.NumIn, CB.NumOut, DFSInStack, DT, LI);
   // Optimistically add fact from first condition.
----------------
nikic wrote:
> Why is this only done for the "and" case and not for normal dominating conditions?
It's done when adding facts in general (around line 1454) or do you mean something else?


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1206
+  // in the loop.
+  unsigned StepSize = StepInst->getSourceElementType()->getScalarSizeInBits();
+  if (StepSize != 8) {
----------------
nikic wrote:
> GEP steps by the type alloc size, which is generally not the same as the type size.
Updated the code to use `collectOffset` which should take care of this  and added extra tests in  36999318f091


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1210
+    if (!UpperGEP ||
+        StepSize != UpperGEP->getSourceElementType()->getScalarSizeInBits())
+      return;
----------------
nikic wrote:
> I think you need significantly more checks to make this valid reasoning. There is no guarantee here that both GEPs are based on the same value, so maybe the induction GEP is going over 16 byte aligned pointers, while the UpperGEP is working on pointers offset by 8, in which case the condition will never be true.
Added some extra tests in  36999318f091, d34bc66ef9bd & c92c9462a0ca. For now, I restricted them to cases where both UpperGEP and the phi are based on the same start values as the later ULE check is not enough to avoid this in all cases.

I also adjusted the code to collect the `collectOffset` (this allows for easy handling of GEPs accessing structs & constant offsets) and check if the upper offset is a multiple of the step offset.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152730