[PATCH] D153752: [InstSimplify] Fold icmp comparing GEPs with local allocation

[Nikita Popov via Phabricator via llvm-commits]

nikic requested changes to this revision.
nikic added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:2798-2799
+          getObjectSize(RHS, RHSSize, DL, TLI, Opts)) {
+        APInt dist;
+        dist = LHSOffset - RHSOffset;
+        dist = dist.abs();
----------------
Variable names in LLVM start with an uppercase letter.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:2801
+        dist = dist.abs();
+        if (dist.ult(LHSSize) && dist.ult(RHSSize))
+          return ConstantInt::get(getCompareTy(LHS),
----------------
This condition isn't wrong, but it's also not as accurate as it could be.

Let's say `LHSSize = 4`, `LHSOffset = 0`, `RHSSize = 2`, `RHSOffset = 2` with `LHSOffset - RHSOffset = -2`. These can be equal.

Then consider `LHSSize = 4`, `LHSOffset = 2`, `RHSSize = 2`, `RHSOffset = 0` with `LHSOffset - RHSOffset = 2`. These cannot be equal.

As you can see, the sign of the result matters and we should not just take the absolute value.

I believe the condition should be `Dist.isNonNegative() ? Dist.ult(LHSSize) : (-Dist).ult(RHSSize)`.


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

https://reviews.llvm.org/D153752