[PATCH] D16343: [BasicAA] Fix for missing must alias information

[Gerolf Hoflehner via llvm-commits]

Gerolf added a comment.

Regarding: "Better, but.. why mask on only some updates vs every one or once at end?"

I took the wrap-around out of inner loop and made sure it is done once for each PointerSize. If we can hoist that out of the loop then the BaseOffs wrap can be pushed out of the outer loop also. That requires at least additional checks on the assumption of a constant PointerSize. That needs a separate commit - if any.


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:328
@@ +327,3 @@
+  unsigned ShiftBits = 64 - PointerSize;
+  Offset <<= ShiftBits;
+  return Offset >> ShiftBits;
----------------
reames wrote:
> Isn't shifting into the sign bit undefined behaviour?
> 
> I think what you really want to express is Offset % 0xFF..PointerSize..FF.  Why not just write it that way?
In C++ (Section 5.8.2) it actually is well defined: The value of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are zero-filled.
In older C standards I think at one point it was undefined and later implementation defined when the result did not fit.
And no, for we offset we actually need the sign extension. % doesn't do that.
So I think that code is fine. Agreed?

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:458
@@ -447,6 +457,3 @@
       // pointer size.
-      if (unsigned ShiftBits = 64 - PointerSize) {
-        Scale <<= ShiftBits;
-        Scale = (int64_t)Scale >> ShiftBits;
-      }
+      Scale = (uint64_t)adjustToPointerSize(Scale, PointerSize);
 
----------------
reames wrote:
> This line is highly likely wrong.  It looks like you're converting to a signed type, doing wrapping there, then converting back.  That's suspicious at best.
> 
> I suspect you may need to change the signage of the existing code, but that should definitely be separate.
It is  "correct" since the bits don't change in that cast: "correct" in the sense of being  equivalent to the old code. But I agree with you that this should be an independent commit and take it out.


http://reviews.llvm.org/D16343