[PATCH] D38662: [BasicAA] Support arbitrary pointer sizes (and fix an overflow bug)

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:519
+      // relevant values of V, (C2*Scale) can overflow. In that case, we cannot
+      // decompose the expression in this way.
+      APInt WideScaledOffset = IndexOffset.sextOrTrunc(MaxPointerSize*2) *
----------------
efriedma wrote:
> hfinkel wrote:
> > efriedma wrote:
> > > hfinkel wrote:
> > > > efriedma wrote:
> > > > > hfinkel wrote:
> > > > > > efriedma wrote:
> > > > > > > I'm not sure I understand this comment.
> > > > > > > 
> > > > > > > It's true that C2*Scale can overflow, but I'm not sure it makes sense to try to address that here.  The multiply by the scale can overflow no matter where the scale comes from (assuming the GEP isn't marked inbounds).
> > > > > > Can you please elaborate?
> > > > > > 
> > > > > > If this overflows, then the offset here isn't an offset, and the reasoning done later won't be correct.
> > > > > > 
> > > > > > In the test case, the GEPs are marked as inbounds. I don't think that's relevant here.
> > > > > Consider the following testcase:
> > > > > 
> > > > > ```
> > > > > target datalayout = "e-m:e-p:32:32-f64:32:64-f80:32-n8:16:32-S128"
> > > > > define i32* @b1(i32 *%a) {
> > > > > %r = getelementptr i32, i32* %a, i32 -2147483648
> > > > > ret i32* %r
> > > > > }
> > > > > define i32* @b2(i32 *%a) {
> > > > > %o = add i32 -2147483648, 0
> > > > > %r = getelementptr i32, i32* %a, i32 %o
> > > > > ret i32* %r
> > > > > }
> > > > > ```
> > > > > 
> > > > > In both cases, %a and %r should be mustalias.  If you run "-aa-eval", we somehow conclude that the pointers are mustalias in b1, and noalias in b2.   If you're trying to make overflow well-behaved, it doesn't make sense to treat the overflow introduced by GetLinearExpression differently from the overflow inherent in scaling.
> > > > I agree, however, as I attempted to explain in the comment:
> > > > 
> > > >   1. The problem is not in GetLinearExpression itself, it is with how the results of GetLinearExpression are being used here.
> > > >   2. What's happening here, where we're multiplying by the scale (which by itself is always fine), and applying the distributive property, can introduce an overflow in cases where an overflow might not have existed in the original program.
> > > > 
> > > > I believe that the test cases demonstrate this: there are certainly values for the function parameters for which they'll be no overflow in the address calculations.
> > > I think aliasGEP needs to be changed to accept that decompsed GEP arithmetic will overflow, or somehow adopt much more restrictive overflow checking (in many more places than this patch does).
> > > 
> > > ----
> > > 
> > > Suppose we want to adopt more restrictive overflow checking.  First, so we have an expression `(C1*V+C2)*Scale`.  If we treat `(C1*V+C2)` as opaque, there's one place this can overflow: the multiply by Scale.  Currently, we completely ignore this possibility.
> > > 
> > > So now, instead of treating the inner expression as opaque, we want to decompose it to something like `(C1*Scale)*V+C2*Scale`.  There are four arithmetic operations here.  All four of them can potentially overflow.  And two of them can't be overflow-checked here because V isn't known at compile-time.
> > > 
> > > Given that, I don't see how overflow-checking `C2 * Scale` accomplishes anything except hiding the problem for your exact testcase.
> > > I think aliasGEP needs to be changed to accept that decompsed GEP arithmetic will overflow, or somehow adopt much more restrictive overflow checking (in many more places than this patch does).
> > 
> > I think that it does need to be changed, but if we can fix it like this, that probably preserves our optimization abilities. Changing it in other ways, AFAIKT, will weaken it (perhaps unnecessarily). The only other place I see that might have this issue (introducing new compile-time overflows) is in BasicAAResult::constantOffsetHeuristic. There may, however, be a more-general problem...
> > 
> > > If we treat (C1*V+C2) as opaque, there's one place this can overflow: the multiply by Scale. Currently, we completely ignore this possibility.
> > 
> > If it is opaque, then it is just a value (regardless of whether or not its computation depended on some overflowing result). Is there something we'd need to do?
> > 
> > > So now, instead of treating the inner expression as opaque, we want to decompose it to something like (C1*Scale)*V+C2*Scale. There are four arithmetic operations here. All four of them can potentially overflow. And two of them can't be overflow-checked here because V isn't known at compile-time.
> > 
> > When I looked at this previously, my thought had been that only the C2*Scale overflow could happen in well-defined code. I may have been wrong about that. The issue is that if C1*Scale overflows then C1*V is likely to overflow in the original program. That, however, is clearly not necessarily true. So, yes, I agree, we should check that too.
> > 
> > Then there's the other two operations. They, indeed, might overflow. I don't see anything to prevent them from overflowing even if the original expression did not (essentially in some cases where |V| < Scale and |C1|,|C2| are large in the right ways). In the face of such overflows, the decomposition may not be sound, unfortunately.
> > 
> > > Given that, I don't see how overflow-checking C2 * Scale accomplishes anything except hiding the problem for your exact testcase.
> > 
> > This may be a fair criticism. I think that the general problem of dealing with overflows is going to need a careful audit and a lot of changes. There's a lot of logic here that likely isn't completely sound. However, I don't want to make things worse, certainly, and this was a pre-existing regression test. I should check for C1*Scale overflow too (to avoid making anything worse).
> > 
> > More-general pre-existing problems, however, I'd recommend treating as orthogonal to this change.  I think that more-consistently using APInts makes addressing these problems easier, not harder, in follow-up work.
> > I think that more-consistently using APInts makes addressing these problems easier, not harder, in follow-up work.
> 
> Okay, this makes sense.
This still needs to be updated (at least, the comment needs to change).


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1098
+  if (C1->getBitWidth() > 64 || C2->getBitWidth() > 64)
+    return MayAlias;
+
----------------
hfinkel wrote:
> mppf wrote:
> > This addition seems surprising to me. Why wouldn't we just use APInt for the computation below? Is this a workaround for something?
> Ah, sort of. Updating StructLayout::getElementOffset to take an APInt does not seem worthwhile. You're right, however: this is the wrong test. It should say:
> 
> if (C1->getActiveBits() > 64 || C2->getActiveBits() > 64)
> 
This still needs to be updated.


https://reviews.llvm.org/D38662