[PATCH] D86364: [ValueTracking] Interpret GEPs as a series of adds multiplied by the related scaling factor

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet marked 4 inline comments as done.
qcolombet added a comment.

Thanks all for your help and @nikic in particular for doing the perf measurements and pushing for a better solution.
I think this shows that reviews work and produce better code quality!

Will push shortly.



================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1363
     computeKnownBits(I->getOperand(0), LocalKnown, Depth + 1, Q);
-    unsigned TrailZ = LocalKnown.countMinTrailingZeros();
+    KnownBits AddrKnownBits(LocalKnown);
+    // Accumulate the constant indices in a separate variable
----------------
nikic wrote:
> As far as I can see, you could just rename LocalKnown to AddrKnownBits, rather than first computing into LocalKnown and then copying it.
> 
> Or, going one step further, you could directly populate `Known`, rather than populating `AddrKnownBits` and then copying it at the end.
Good point, we don't need the intermediate variables anymore.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1366
+    // to minimize the number of calls to computeForAddSub.
+    APInt AccCstIndices(BitWidth, 0, /*IsSigned*/ true);
 
----------------
nikic wrote:
> Nit: Cst -> Const. This seems like an unusual abbreviation.
Done!


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1385
+      unsigned IndexBitWidth = Index->getType()->getScalarSizeInBits();
+      KnownBits IndexBits(IndexBitWidth);
       if (StructType *STy = GTI.getStructTypeOrNull()) {
----------------
nikic wrote:
> Move these down to the computeKnownBits call? Doesn't appear to be used in between.
Good catch, we don't need that here anymore.
Moved it down.


================
Comment at: llvm/test/Transforms/InstCombine/constant-fold-gep.ll:111
+; CHECK-LABEL: @test_gep_in_struct(
+; CHECK; load i32, i32* %NS7, align 8
+define i32 @test_gep_in_struct(i64 %idx) {
----------------
nikic wrote:
> Typo: `CHECK;` instead of `CHECK:`. I would suggest to simply rerun update_test_checks.py instead.
Oh wow!
Sorry about that and nice catch!

Ran the update script (when I originally wrote the test, the checks were not autogenerated.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86364