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

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

I generally like the idea, but I have some implementation specific comments inline.


================
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
----------------
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`.

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

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3401
@@ +3400,3 @@
+
+  if ((Op1->getSCEVType() == scConstant || Op2->getSCEVType() != scConstant) &&
+      (Op1->getSCEVType() != scConstant || Op2->getSCEVType() == scConstant))
----------------
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.

================
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
----------------
We don't repeat function names in new code.

================
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});
----------------
How about:

```
if (SetVector<ValueOffsetPair> *SV = getSCEVValues(S))
  SV->remove(...);
```
?

Same below.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3483
@@ +3482,3 @@
+      // because it may generate add/sub instead of GEP in SCEV expansion.
+      if (Offset != nullptr && Stripped->getSCEVType() != scUnknown &&
+          !isa<GetElementPtrInst>(V))
----------------
Use `isa<>` instead of directly comparing `getSCEVType`.


Repository:
  rL LLVM

http://reviews.llvm.org/D21313