[PATCH] D11847: [BasicAA] Fix zext & sext handling

[hfinkel@anl.gov via llvm-commits]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

A few small comment improvements, but otherwise, LGTM.


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:170
@@ -174,1 +169,3 @@
 
+  // A linear transformation of a Value; this class represents ZExt(SExt(V,
+  // SExtBits), ZExtBits) * Scale + Offset.
----------------
The formatting would be better here if you broke the line before the ZExt(

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:205
@@ -198,2 +204,3 @@
 /// Note that this looks through extends, so the high bits may not be
 /// represented in the result.
+static const Value *GetLinearExpression(const Value *V, APInt &Scale,
----------------
We should document what NSW/NUW mean (I realize this is somewhat obvious, but even so) and that they should both be set to true before calling this at the top level.

Similarly, ZExtBits, SExtBits, Offset and Scale need to be zero, and we should say that as well.


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:222
@@ +221,3 @@
+  if (const ConstantInt *Const = dyn_cast<ConstantInt>(V)) {
+    // if it's a constant, just convert it to an offset and remove the variable.
+    // If we've been called recursively the Offset bit width will be greater
----------------
if -> If

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:229
@@ -217,3 +228,3 @@
     assert(Scale == 0 && "Constant values don't have a scale");
     return V;
   }
----------------
The comment here says, "and remove the variable", but this seems slightly misleading (I'd assume that means returning a nullptr or Undef). Really, the value will be ignored because the Scale is never changed from zero.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:624
@@ +623,3 @@
+    /// the addition overflows.
+    bool
+    constantOffsetHeuristic(const SmallVectorImpl<VariableGEPIndex> &VarIndices,
----------------
The comment should explain what the return value actually means.


http://reviews.llvm.org/D11847