[PATCH] D76434: [SCEV] Query expanded immediate cost at minsize

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

Apologies if i'm missing the point here.



================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2204
+
+  auto getIntImmCostInst = [&TTI, &CostKind](
+    const SCEV *User, const SCEV *S, unsigned Idx, int &BudgetRemaining) {
----------------
s/get/accountFor/


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2214-2217
+    case scAddExpr:
+      BudgetRemaining -=
+        TTI.getIntImmCostInst(Instruction::Add, Idx, Imm, Ty, CostKind);
+      break;
----------------
I'm not convinced this modelling is correct.
(which is why i didn't respond, but apparently i forgot to actually post that)

If we have SCEV `x + y + 42`, `42` will be modelled as-if it's at index `2`,
but we should model this as `(x + y) + 42`, because there's usually some
form of an `add` that takes an immediate as second param.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2218-2221
+    case scMulExpr:
+      BudgetRemaining -=
+        TTI.getIntImmCostInst(Instruction::Mul, Idx, Imm, Ty, CostKind);
+      break;
----------------
Same


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2222-2229
+    case scSMaxExpr:
+    case scUMaxExpr:
+    case scSMinExpr:
+    case scUMinExpr:
+      BudgetRemaining -=
+        TTI.getIntImmCostInst(Instruction::ICmp, Idx, Imm, Ty, CostKind);
+      BudgetRemaining -=
----------------
And again, if we have `umin(x, y, 42)`, it's lowered as `z = (x u< y) ? x : y  ;  z u< 42 ? z : 42`,
so you can't possibly have third operand here.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2356-2357
+    if (CostKind == TTI::TCK_CodeSize) {
+      unsigned Idx = 0;
+      for (auto *Op : NAry->operands()) {
+        if (!getIntImmCostInst(NAry, Op, Idx, BudgetRemaining))
----------------
enumerate(NAry->operands())


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2409-2410
+    if (CostKind == TTI::TCK_CodeSize) {
+      unsigned Idx = 0;
+      for (auto *Op : NAry->operands()) {
+        if (!getIntImmCostInst(NAry, Op, Idx, BudgetRemaining))
----------------
enumerate(NAry->operands())


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

https://reviews.llvm.org/D76434