[PATCH] D21313: Use ValueOffsetPair to enhance value reuse during SCEV expansion.

[Wei Mi via llvm-commits]

wmi added inline comments.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:509
@@ +508,3 @@
+    /// of SCEV -> Value:
+    /// Suppose the SCEV S1 -> Value V1 mapping is saved in ExprValueMap,
+    /// and suppose S1 is a SCEVAddExpr which is composed of another SCEV
----------------
sanjoy wrote:
> I think this first part of the explanation can be made easier to
> follow if we write in a more explicit way:
> 
> ```
> Suppose we know S1 expands to V1, and
> 
>   S1 = S2 + C_a
>   S3 = S2 + C_b
> 
> where C_a and C_b are different SCEVConstants.  Then we'd like to
> expand S3 as V1 - C_a + C_b.
> ```
> 
> The second part is fine, but please make it a obvious that it
> semantically maps `S` to `{expand(S + Offset), Offset}`, that is, if
> `S` is mapped to `{V, Offset}` then `expand(S)` is `V - Offset`.
Thanks for helping to rephrase the comment. It makes the comment much clearer. Comment fixed as suggested.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3392
@@ +3391,3 @@
+ScalarEvolution::splitAddExpr(const SCEV *S) {
+  if (static_cast<SCEVTypes>(S->getSCEVType()) != scAddExpr)
+    return {S, nullptr};
----------------
sanjoy wrote:
> Why not use `dyn_cast`?
Change it to isa<SCEVAddExpr>(S) instead.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3401
@@ +3400,3 @@
+
+  if ((Op1->getSCEVType() == scConstant || Op2->getSCEVType() != scConstant) &&
+      (Op1->getSCEVType() != scConstant || Op2->getSCEVType() == scConstant))
----------------
sanjoy wrote:
> Again, please use `dyn_cast`.  For instance, here you can do:
> 
> ```
> auto *ConstOp = dyn_cast<SCEVConstant>(Op1);
> auto *NonConstOp = dyn_cast<SCEVConstant>(Op2);
> if (!ConstOp && NonConstOp)
>   std::swap(ConstOp, NonConstOp);
> if (!ConstOp)
>   return {S, nullptr};
> ```
> 
> However, since SCEV expressions are ordered in complexity, you should only ever need to check the first operand -- if there is a constant operand, then it will be the first one.
> 
> Once you've made these simplifications, if the function becomes small enough I'd suggest just inlining it into its caller.  If it isn't small enough, please make it a static helper function -- there is no need for this to live in the ScalarEvolution interface.
Thanks for the suggestion which simplifies the func a lot. I change the function to be static.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3413
@@ +3412,3 @@
+
+/// getSCEVValues - Return the ValueOffsetPair set from S. S can be
+/// represented by the value and offset from any ValueOffsetPair in
----------------
sanjoy wrote:
> We don't repeat function names in new code.
Fixed.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3441
@@ -3414,1 +3440,3 @@
+    // Remove {V, 0} from the set of ExprValueMap[S]
     if (SV)
+      SV->remove({V, nullptr});
----------------
sanjoy wrote:
> How about:
> 
> ```
> if (SetVector<ValueOffsetPair> *SV = getSCEVValues(S))
>   SV->remove(...);
> ```
> ?
> 
> Same below.
That is better. Fixed.


Repository:
  rL LLVM

http://reviews.llvm.org/D21313