[PATCH] D75908: [SCEV] isHighCostExpansionHelper(): use correct TTI hooks

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: samparker, spatel, mkazantsev, reames, wmi.
lebedev.ri added a project: LLVM.
Herald added subscribers: danielkiss, hiraditya, kristof.beyls.

Cost modelling strikes again.
In PR44668 https://bugs.llvm.org/show_bug.cgi?id=44668 patch series,
i've made the same mistake of always using generic `getOperationCost()`
that i missed in reviewing D73480 <https://reviews.llvm.org/D73480>/D74495 <https://reviews.llvm.org/D74495> which was later fixed
in 62dd44d76da9aa596fb199bda8b1e8768bb41033 <https://reviews.llvm.org/rG62dd44d76da9aa596fb199bda8b1e8768bb41033>.

We should be using more specific hooks instead - `getCastInstrCost()`,
`getArithmeticInstrCost()`, `getCmpSelInstrCost()`.

Evidently, this does not have an effect on the existing testcases,
with unchanged default cost budget. But if it *does* have an effect
on some target, we'll have to segregate tests that use this function
per-target, much like we already do with other TTI-aware transform tests.

There's also an issue that @samparker has brought up in post-commit-review:

In D73501#1905171 <https://reviews.llvm.org/D73501#1905171>, @samparker wrote:

> Hi,
>  Did you get performance numbers for these patches? We track the performance
>  of our (Arm) open source DSP library and the cost model fixes were generally
>  a notable improvement, so many thanks for that! But the final patch
>  for rewriting exit values has generally been bad, especially considering
>  the gains from the modelling improvements. I need to look into it further,
>  but on my current test case I'm seeing +30% increase in stack accesses
>  with a similar decrease in performance.
>  I'm just wondering if you observed any negative effects yourself?


I don't know if this addresses that, or we need D66450 <https://reviews.llvm.org/D66450> for that.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75908

Files:
  llvm/lib/Analysis/ScalarEvolutionExpander.cpp


Index: llvm/lib/Analysis/ScalarEvolutionExpander.cpp
===================================================================
--- llvm/lib/Analysis/ScalarEvolutionExpander.cpp
+++ llvm/lib/Analysis/ScalarEvolutionExpander.cpp
@@ -2173,7 +2173,7 @@
     }
     const SCEV *Op = CastExpr->getOperand();
     BudgetRemaining -=
-        TTI.getOperationCost(Opcode, S->getType(), Op->getType());
+        TTI.getCastInstrCost(Opcode, S->getType(), Op->getType());
     return isHighCostExpansionHelper(Op, L, At, BudgetRemaining, TTI,
                                      Processed);
   }
@@ -2184,7 +2184,7 @@
     if (auto *SC = dyn_cast<SCEVConstant>(UDivExpr->getRHS())) {
       if (SC->getAPInt().isPowerOf2()) {
         BudgetRemaining -=
-            TTI.getOperationCost(Instruction::LShr, S->getType());
+            TTI.getArithmeticInstrCost(Instruction::LShr, S->getType());
         // Note that we don't count the cost of RHS, because it is a constant,
         // and we consider those to be free. But if that changes, we would need
         // to log2() it first before calling isHighCostExpansionHelper().
@@ -2206,7 +2206,8 @@
       return false; // Consider it to be free.
 
     // Need to count the cost of this UDiv.
-    BudgetRemaining -= TTI.getOperationCost(Instruction::UDiv, S->getType());
+    BudgetRemaining -=
+        TTI.getArithmeticInstrCost(Instruction::UDiv, S->getType());
     return isHighCostExpansionHelper(UDivExpr->getLHS(), L, At, BudgetRemaining,
                                      TTI, Processed) ||
            isHighCostExpansionHelper(UDivExpr->getRHS(), L, At, BudgetRemaining,
@@ -2219,8 +2220,8 @@
     assert(NAry->getNumOperands() >= 2 &&
            "Polynomial should be at least linear");
 
-    int AddCost = TTI.getOperationCost(Instruction::Add, OpType);
-    int MulCost = TTI.getOperationCost(Instruction::Mul, OpType);
+    int AddCost = TTI.getArithmeticInstrCost(Instruction::Add, OpType);
+    int MulCost = TTI.getArithmeticInstrCost(Instruction::Mul, OpType);
 
     // 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?
@@ -2277,20 +2278,22 @@
     int PairCost;
     switch (S->getSCEVType()) {
     case scAddExpr:
-      PairCost = TTI.getOperationCost(Instruction::Add, OpType);
+      PairCost = TTI.getArithmeticInstrCost(Instruction::Add, OpType);
       break;
     case scMulExpr:
       // TODO: this is a very pessimistic cost modelling for Mul,
       // because of Bin Pow algorithm actually used by the expander,
       // see SCEVExpander::visitMulExpr(), ExpandOpBinPowN().
-      PairCost = TTI.getOperationCost(Instruction::Mul, OpType);
+      PairCost = TTI.getArithmeticInstrCost(Instruction::Mul, OpType);
       break;
     case scSMaxExpr:
     case scUMaxExpr:
     case scSMinExpr:
     case scUMinExpr:
-      PairCost = TTI.getOperationCost(Instruction::ICmp, OpType) +
-                 TTI.getOperationCost(Instruction::Select, OpType);
+      PairCost = TTI.getCmpSelInstrCost(Instruction::ICmp, OpType,
+                                        CmpInst::makeCmpResultType(OpType)) +
+                 TTI.getCmpSelInstrCost(Instruction::Select, OpType,
+                                        CmpInst::makeCmpResultType(OpType));
       break;
     default:
       llvm_unreachable("There are no other variants here.");


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D75908.249318.patch
Type: text/x-patch
Size: 3417 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200310/426bd517/attachment-0001.bin>