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

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM. Thank you for your patience and continued work on this issue.



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


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


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1385
+      unsigned IndexBitWidth = Index->getType()->getScalarSizeInBits();
+      KnownBits IndexBits(IndexBitWidth);
       if (StructType *STy = GTI.getStructTypeOrNull()) {
----------------
Move these down to the computeKnownBits call? Doesn't appear to be used in between.


================
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) {
----------------
Typo: `CHECK;` instead of `CHECK:`. I would suggest to simply rerun update_test_checks.py instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86364