[PATCH] D15559: [SCEVExpander] Make findExistingExpansion smarter

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1825
@@ -1824,1 +1824,3 @@
 
+// Recursive search of the IR's operands to see if any of them are congruent to
+// S. This function is called by findExistingExpansion.
----------------
Nit:  do you mean `I's operands`?

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1828
@@ +1827,3 @@
+static Instruction *FindSameSCEVInst(ScalarEvolution &SE, const SCEV *S,
+                                     Instruction *I, unsigned depth) {
+  // Limit our recursion depth.
----------------
Nit: LLVM convention is `Depth`.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1833
@@ +1832,3 @@
+
+  if (SE.getSCEV(I) == S)
+    return I;
----------------
Might want to check `SE.isSCEVAble(I->getType())` before calling `getSCEV` to avoid creating too many `SCEVUnknown` objects unnecessarily.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1836
@@ +1835,3 @@
+
+  for (unsigned i = 0; i < I->getNumOperands(); ++i) {
+    Instruction *Op = FindSameSCEVInst(
----------------
Use a range `for` here.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1838
@@ +1837,3 @@
+    Instruction *Op = FindSameSCEVInst(
+        SE, S, dyn_cast<Instruction>(I->getOperand(i)), ++depth);
+    if (Op != nullptr)
----------------
I think it will be cleaner to handle the non-instruction case in the caller (and remove the `I == nullptr` check above), since there is no reason to have the initial caller (first level of recursion) pass in `nullptr` for `I`.

I don't think `depth++` is correct -- you'll blow the stack if `I` is a PHI node with itself as its first operand.  At the very least, it needs to be `++depth`. Also, since you're calling the counter `depth`, it'd make more sense to pass in `depth + 1` here, and not `++depth`.  If you **do** want to increment `depth` in place, it might be better to call it `Iter`.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1871
@@ +1870,3 @@
+        return RHS;
+    } else if (isa<ConstantInt>(RHSV)) {
+      if (SE.hasOperand(SE.getSCEV(LHS), S)) {
----------------
Why do you care about `RHS` being a constant?

I think it is better to have the logic be:

  # Check `LHS` as usual
  # If `RHSV` is an `Instruction` then check `RHS` as well
  # Use `FindSameSCEVInst` to do a deeper search on `LHS`

Is there any reason why you don't need to call `FindSameSCEVInst` on `RHS` as well (when it is an instruction)?


================
Comment at: test/Transforms/LoopUnroll/high-cost-trip-count-computation.ll:53
@@ -27,1 +52,2 @@
+
 !0 = !{i64 1, i64 100}
----------------
Reading the code for `@test`, I think the metadata is required to tell LLVM that it is okay to generate a divide by `%step` if it thinks that is profitable to do (and so the test case actually checks LLVM's heuristics, not correctness).


http://reviews.llvm.org/D15559