[PATCH] D73728: [SCEV] SCEVExpander::isHighCostExpansionHelper(): cost-model add/mul

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked 5 inline comments as done.
lebedev.ri added a comment.

In D73728#1890687 <https://reviews.llvm.org/D73728#1890687>, @mkazantsev wrote:

> LGTM


Thank you for the review!



================
Comment at: llvm/lib/Analysis/ScalarEvolutionExpander.cpp:2238
+    Type *OpType = NAry->getType();
+    int PairCost = TTI.getOperationCost(Opcode, OpType);
+
----------------
mkazantsev wrote:
> This is a very rough approximation for Mul, because of Bin Pow algorithm used in Expander. See https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/ScalarEvolutionExpander.cpp#L781. You might want to factor it in in the future, but what you did is fine for the 1st step. Please add a TODO for that.
Aha, good point. I will add a TODO.


================
Comment at: llvm/lib/Analysis/ScalarEvolutionExpander.cpp:2247
+        continue;
+      BudgetRemaining -= PairCost;
+    }
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > Just a suggestion, feel free to ignore: break if `BudgetRemaining` goes below zero in the loop to save time processing really huge SCEVs with many operands.
> Or actually nvm, I guess it should be handled inside the helper.
Indeed, the idea is that it will be handled by `isHighCostExpansionHelper()`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73728/new/

https://reviews.llvm.org/D73728