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

[Sanjoy Das via llvm-commits]

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

lgtm modulo minor stuff (please fix and then directly check in)


================
Comment at: lib/Analysis/ScalarEvolution.cpp:3396
@@ +3395,3 @@
+
+  const SCEVNAryExpr *Add = cast<SCEVNAryExpr>(S);
+  if (Add->getNumOperands() != 2)
----------------
I think it is better to just cast to `SCEVAddExpr` to keep the code clear.  Also, I'd prefer `const auto *`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3470
@@ +3469,3 @@
+      // If stripped is SCEVUnknown, don't bother to save
+      // stripped -> {V, offset}. it doesn't simplify and sometimes even
+      // increase the complexity of the expansion code.
----------------
Nit: `Stripped -> {V, Offset}` to match actual code variable names.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3470
@@ +3469,3 @@
+      // If stripped is SCEVUnknown, don't bother to save
+      // stripped -> {V, offset}. it doesn't simplify and sometimes even
+      // increase the complexity of the expansion code.
----------------
sanjoy wrote:
> Nit: `Stripped -> {V, Offset}` to match actual code variable names.
Nit: Start with uppercase after period s/it/It/

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3472
@@ +3471,3 @@
+      // increase the complexity of the expansion code.
+      // If V is GetElementPtrInst, don't save stripped -> {V, offset}
+      // because it may generate add/sub instead of GEP in SCEV expansion.
----------------
same here.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1896
@@ +1895,3 @@
+  ScalarEvolution::ValueOffsetPair VO = FindValueInExprValueMap(S, At);
+  if (VO.first)
+    return VO.first;
----------------
Can't you just do:


```
if (Value *Val = FindValueInExprValueMap(S, At).first)
  ScalarEvolution::ValueOffsetPair VO = FindValueInExprValueMap(S, At);
```



Repository:
  rL LLVM

https://reviews.llvm.org/D21313