[PATCH] D92723: [BasicAA] Migrate "same base pointer" logic to decomposed GEPs

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1286
+      // of values across loop iterations.
+      if (Var0.Scale == -Var1.Scale && Var0.ZExtBits == Var1.ZExtBits &&
+          Var0.SExtBits == Var1.SExtBits && VisitedPhiBBs.empty() &&
----------------
reames wrote:
> You're missing the bug fix for the reasoning under recursive phis that I just landed.in bfda6941.  Please add the appropriate VisitedPhiBBs.empty check to your conditions.
The check for that is in the line directly below this one :)


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1289
+          isKnownNonEqual(Var0.V, Var1.V, DL)) {
+        // If the indexes are not equal, the actual offset will have at least
+        // Scale or -Scale added to it.
----------------
reames wrote:
> You're missing a necessary check here.  You need to ensure that scale > size and scale mod size == 0.  Otherwise, you can have a case such as scale == 2, size == 8, and have the (unaligned) accesses alias even if the indices differ.  
This is handled by the offset checks below, or at least supposed to. For simplicity, let's assume that `DecompGEP1.Offset` is zero, so OffsetLo/Hi below are just +/-Scale.

```
// Possibility 1, access starts at +Scale or higher
[0...V2Size) ... [Scale, Scale+V1Size]
// Possibility 2, access starts at -Scale or lower
[-Scale, -Scale+V1Size) ... [0...V2Size)
```

This checks that the sizes are smaller than the scale. I don't believe that scale mod size == 0 is a requirement. If we have a scale of 4 it shouldn't matter whether our access size is 2 or 3, unless I'm missing something.

The conditions used here are analogous to the ones a bit higher up (the AllNonNegative/AllNonPositive ones).


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1292
+        APInt Scale = Var0.Scale.abs();
+        APInt OffsetLo = DecompGEP1.Offset - Scale;
+        APInt OffsetHi = DecompGEP1.Offset + Scale;
----------------
reames wrote:
> I'm not following your logic here, and I think you may have it backwards.
> 
> If we know that the offsets are equal, scales are equal, and indices aren't, then we have no alias.  If we have different offsets, then we need to reason about the offset access sequences.  You might be trying to handle the offset pattern case, but if so, I'm not following?
Does my previous comment make this clearer? Something to keep in mind here is that the second access is always at zero. What the non-equality gives us is that the first access doesn't start in the range (-Scale, Scale). With the offset, it doesn't start in the range (-Scale+Offset, Scale+Offset).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92723