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

[Philip Reames via llvm-commits]

reames added a comment.

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


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:328
@@ +327,3 @@
+  unsigned ShiftBits = 64 - PointerSize;
+  Offset <<= ShiftBits;
+  return Offset >> ShiftBits;
----------------
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?

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


http://reviews.llvm.org/D16343