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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi @nikic

Thanks for the review.

> At least in the current implementation, the compile-time impact of this change is too large

This change makes more calls to computeKnownBits, so the compile-time impact is expected and I don't see how we can avoid any of the calls.

Do you have recommendations on how to limit the impact?

Cheers,
-Quentin



================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1461
+    // True if we track all the bits of the address.
+    bool TrackAddr = true;
+
----------------
nikic wrote:
> I don't understand the purpose of this variable and why we track both the precise known bits *and* the trailing zeros. In the one place that sets TrackAddr to false, can't you set the known bits to unknown?
> 
The trailing zeros may still be tracked while the AddrKnownBits are unknown. This happens when we hit scalable vectors.
I kept this variable to make sure we don't regress computeKnownBits in these cases.

Regarding the `TrackAddr` variable I can turn that into checking if AddrKnownBits are unknowns, I was not just what was the compile time impact of doing this.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1470
     for (unsigned i = 1, e = I->getNumOperands(); i != e; ++i, ++GTI) {
-      // TrailZ can only become smaller, short-circuit if we hit zero.
-      if (TrailZ == 0)
+      if (TrailZ == 0 && !TrackAddr)
         break;
----------------
nikic wrote:
> This should early exit if the AddrKnownBits are unknown, which would be the equivalent of the TrailZ==0 check.
That's not true because AddrKnownBits won't be tracked as soon as we hit a scalable vector whereas the TrailZ information can still be tracked.


================
Comment at: llvm/test/Transforms/InstCombine/constant-fold-gep.ll:47
+; CHECK: store i32 1, i32* getelementptr inbounds ([3 x %struct.X], [3 x %struct.X]* @Y, i64 0, i64 2, i32 1, i64 2), align 4
+  store i32 1, i32* getelementptr ([3 x %struct.X], [3 x %struct.X]* @Y, i64 0, i64 0, i32 0, i64 17), align 4
 ; CHECK: store i32 1, i32* getelementptr inbounds ([3 x %struct.X], [3 x %struct.X]* @Y, i64 1, i64 0, i32 0, i64 0), align 8
----------------
aqjune wrote:
> qcolombet wrote:
> > Note: The change in the input of the test is because the alignment was not consistent with the base pointer.
> > On this store the alignment cannot be 8, since the base pointer is aligned on 16 (17 * 4 + 16 == 17 * 4 + 4 * 4 == 21 * 4 == 10 * 8 + 4 i.e., remainder with 8 cannot be zero). More easily seen looking at the previous load: @Y + 16 is aligned to 8 then we add 4 to make @Y + 17, and we claimed it was aligned to 8.
> I agree that the changes were necessary, otherwise the stores should always raise UB due to the alignment mismatch.
> It seems that the last three stores below are always raising UB as well - the size of `@Y` is 72 bytes, so the accesses below are out-of-bounds.
> Should we treat these in this patch (by removing the three lines or expanding the size of the global variable), or is it okay to just leave them as they were?
I think the idea here is that the test was checking whether the inbounds keyword was added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86364