[PATCH] D86050: [SCEV] Refactor isHighCostExpansionHelper

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.
Herald added a subscriber: danielkiss.

Thanks, almost there i think.



================
Comment at: llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h:239
     int BudgetRemaining = Budget * TargetTransformInfo::TCC_Basic;
-    Worklist.emplace_back(Expr);
+    Worklist.emplace_back(0, 0, Expr);
     while (!Worklist.empty()) {
----------------
I'd maybe suggest `(-1, -1, Expr)` to really differentiate this case.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2228
+    if (auto *SC = dyn_cast<SCEVConstant>(S->getOperand(1)))
+      if (SC->getAPInt().isPowerOf2())
+        Opcode = Instruction::LShr;
----------------
As a fixme, i suppose you might want to cost `1<<SC->getAPInt()` constant?


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2290-2291
+  for (unsigned Opc : Opcodes) {
+    for (unsigned OpIdx = 0; OpIdx < S->getNumOperands(); ++OpIdx) {
+      Worklist.emplace_back(Opc, OpIdx, S->getOperand(OpIdx));
+    }
----------------
```
    for (auto I = enumerate(S->operands()))
      Worklist.emplace_back(Opc, I.index(), I.value());
```


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2329-2342
+    int Cost =
+      costAndCollectOperands<SCEVUDivExpr>(WorkItem, TTI, CostKind, Worklist);
 
     // UDivExpr is very likely a UDiv that ScalarEvolution's HowFarToZero or
     // HowManyLessThans produced to compute a precise expression, rather than a
     // UDiv from the user's code. If we can't find a UDiv in the code with some
     // simple searching, we need to account for it's cost.
----------------
This is very much not identical to the old code.
I think you want to do
```
    // UDivExpr is very likely a UDiv that ScalarEvolution's HowFarToZero or
    // HowManyLessThans produced to compute a precise expression, rather than a
    // UDiv from the user's code. If we can't find a UDiv in the code with some
    // simple searching, we need to account for it's cost.

    // At the beginning of this function we already tried to find existing
    // value for plain 'S'. Now try to lookup 'S + 1' since it is common
    // pattern involving division. This is just a simple search heuristic.
    if (getRelatedExistingExpansion(
            SE.getAddExpr(S, SE.getConstant(S->getType(), 1)), &At, L))
      return false; // Consider it to be free.

    int Cost =
      costAndCollectOperands<SCEVUDivExpr>(WorkItem, TTI, CostKind, Worklist);
```


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2184-2195
+  // In this polynominal, we may have some zero operands, and we shouldn't
+  // really charge for those. So how many non-zero coeffients are there?
+  int NumTerms = llvm::count_if(S->operands(), [](const SCEV *Op) {
+                                  return !Op->isZero();
+                                });
+
+  // Ignoring constant term (operand 0), how many of the coeffients are u> 1?
----------------
lebedev.ri wrote:
> Move this back into `case scAddRecExpr:` please
Please mark resolved comments as such.


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

https://reviews.llvm.org/D86050