From 8737dc2d32e6b76cfffa03cddabc8b5b1dd6f911 Mon Sep 17 00:00:00 2001 From: Roman Lebedev Date: Thu, 12 Mar 2020 11:33:38 +0300 Subject: [PATCH] [SCEV] isHighCostExpansionHelper(): use correct TTI hooks Summary: Cost modelling strikes again. In PR44668 patch series, i've made the same mistake of always using generic `getOperationCost()` that i missed in reviewing D73480/D74495 which was later fixed in 62dd44d76da9aa596fb199bda8b1e8768bb41033. 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, @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 for that. Reviewers: samparker, spatel, mkazantsev, reames, wmi Reviewed By: reames Subscribers: kristof.beyls, hiraditya, danielkiss, llvm-commits, samparker Tags: #llvm Differential Revision: https://reviews.llvm.org/D75908 --- llvm/lib/Analysis/ScalarEvolutionExpander.cpp | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/llvm/lib/Analysis/ScalarEvolutionExpander.cpp b/llvm/lib/Analysis/ScalarEvolutionExpander.cpp index a09f1f3..67d0a02 100644 --- a/llvm/lib/Analysis/ScalarEvolutionExpander.cpp +++ b/llvm/lib/Analysis/ScalarEvolutionExpander.cpp @@ -2173,7 +2173,7 @@ bool SCEVExpander::isHighCostExpansionHelper( } 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 @@ bool SCEVExpander::isHighCostExpansionHelper( if (auto *SC = dyn_cast(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 @@ bool SCEVExpander::isHighCostExpansionHelper( 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 @@ bool SCEVExpander::isHighCostExpansionHelper( 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 @@ bool SCEVExpander::isHighCostExpansionHelper( 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."); -- 2.7.4