[PATCH] D99424: [BasicAA] Be more careful with modulo ops on VariableGEPIndex.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:313
     return LinearExpression(Val, APInt(Val.getBitWidth(), 0),
-                            Val.evaluateWith(Const->getValue()));
+                            Val.evaluateWith(Const->getValue()), false);
 
----------------
nikic wrote:
> Probably doesn't really matter, but this should be `true`. Note that the expression here is `Val * 0 + Const`.
Ah right, `Val`  is multiplied by 0 here. Updated.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:377
         E.Scale <<= RHS.getLimitedValue();
-        return E;
+        // SHL multiplies by a power-of-2 and always preserves modulo.
+        break;
----------------
nikic wrote:
> This should be `E.IsNSW &= NSW`, to keep it as a plain NSW flag without further semantics.
Yes, I also removed the outdated comment.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1152
+      if (!DecompGEP1.VarIndices[i].IsNSW && !Scale.abs().isPowerOf2())
+        GCD = APInt(Scale.getBitWidth(), 1);
+      else if (i == 0)
----------------
nikic wrote:
> What I had in mind here is something like this:
> ```
>       APInt Scale = DecompGEP1.VarIndices[i].Scale;
>       if (!DecompGEP1.VarIndices[i].IsNSW)
>         Scale = APInt::getOneBitSet(
>             Scale.getBitWidth(), Scale.countTrailingZeros());
> ```
> And then continue with the previous implementation. This preserves a power of two factor, even if the whole scale is not power of two.
Updated, thanks!


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1718
+        // Conservatively assume modulo is not preserved.
+        Dest[j].IsNSW = false;
+      } else
----------------
nikic wrote:
> This doesn't appear to be tested.
I added a test in ef78325c1033 and also removed the outdated comment.


================
Comment at: llvm/test/Analysis/BasicAA/gep-alias.ll:164
   %i2 = shl i32 %i, 2
-  %i3 = add i32 %i2, 1
+  %i3 = add nsw nuw i32 %i2, 1
   ; P2 = P + 1 + 4*i
----------------
nikic wrote:
> With the suggested change to the power of two handling, this change shouldn't be needed anymore.
Correct, I removed the changes, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99424