[PATCH] D15559: [SCEVExpander] Make findExistingExpansion smarter

[Junmo Park via llvm-commits]

flyingforyou marked 5 inline comments as done and an inline comment as not done.

================
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.
----------------
sanjoy wrote:
> Nit:  do you mean `I's operands`?
Yep.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1838
@@ +1837,3 @@
+    Instruction *Op = FindSameSCEVInst(
+        SE, S, dyn_cast<Instruction>(I->getOperand(i)), ++depth);
+    if (Op != nullptr)
----------------
sanjoy wrote:
> 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`.
Sure, I will remove` I == nullptr`, add nullptr check when calling this function.

Thanks. I will change `++depth` to `Depth + 1`.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1871
@@ +1870,3 @@
+        return RHS;
+    } else if (isa<ConstantInt>(RHSV)) {
+      if (SE.hasOperand(SE.getSCEV(LHS), S)) {
----------------
sanjoy wrote:
> 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)?
> 
There is no critical reason that I care about RHS beging a constant.

I will modify the logic what you said. It seems more proper way for `findExistingExpansion`.

Thanks.

================
Comment at: test/Transforms/LoopUnroll/high-cost-trip-count-computation.ll:53
@@ -27,1 +52,2 @@
+
 !0 = !{i64 1, i64 100}
----------------
sanjoy wrote:
> 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).
Thanks. I'll remain the code.


http://reviews.llvm.org/D15559