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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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);
 
----------------
Probably doesn't really matter, but this should be `true`. Note that the expression here is `Val * 0 + Const`.


================
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;
----------------
This should be `E.IsNSW &= NSW`, to keep it as a plain NSW flag without further semantics.


================
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)
----------------
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.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1718
+        // Conservatively assume modulo is not preserved.
+        Dest[j].IsNSW = false;
+      } else
----------------
This doesn't appear to be tested.


================
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
----------------
With the suggested change to the power of two handling, this change shouldn't be needed anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99424