[PATCH] [BasicAA] Fix zext & sext handling

[Nick White]

@sanjoy - thanks for the review; bugs in this class are rather hard to track down as they only really manifest themselves in later optimisations, so more eyes are definitely appreciated :). I've replied to all your comments in-line - let me know if anything's not clear.

Nick


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:297
@@ +296,3 @@
+
+    // This check encodes the rule sext(zext(%x, a), b) == zext(%x, a + b)
+    if (isa<SExtInst>(V) && ZExtBits == 0) {
----------------
sanjoy wrote:
> I'm not sure I understand the comment -- this clause looks like it is implementing `sext(sext(X,a),b)` == `sext(X, a + b)`.
My latest commit adds more comments to make this clearer - does that explain it well enough?

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:302
@@ +301,3 @@
+        // into sext(%x) + sext(c). We'll sext the Offset ourselves:
+        unsigned OldWidth = Offset.getBitWidth();
+        Offset = Offset.trunc(SmallWidth).sext(NewWidth).zextOrSelf(OldWidth);
----------------
sanjoy wrote:
> Why is the `.zextOrSelf(OldWidth)` or the `.trunc(SmallWidth)` needed?  Isn't `OldWidth` always equal to `SmallWidth`?
This method can call itself recursively, and so the operator width reduces as we look through zexts + sexts, but the offset always stays at the initial width.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1004
@@ +1003,3 @@
+  // minumum difference between the two. The minimum distance may occur due to
+  // wrapping; consider "add i8 %i, 5": if %i == 7 then 7 + 5 mod 8 == 4, and so
+  // the minimum distance between %i and %i + 5 is 3.
----------------
sanjoy wrote:
> Do you mean `i3`?  Additions in `i8` are modulo `256`.
You're right - and I've added several unit tests prefixed "constantOffsetHeuristic_" to q.bad.ll to explore this behaviour (and to verify that I'm picking the right scales to multiply by in the heuristic).

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1008
@@ +1007,3 @@
+        Wrapped = APInt::getMaxValue(Width) - MinDiff + APInt(Width, 1);
+  if (Wrapped.ule(MinDiff))
+    MinDiff = Wrapped;
----------------
sanjoy wrote:
> Minor: `APIntOps::umin` may be clearer here.
Thanks - code reviews are one of the best ways to discover new utility functions in the codebase :)

http://reviews.llvm.org/D6682

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/