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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

@samparker why did you commit this?
I have not finished reviewing this, and i've requested changes to the previous revisions.



================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2187
   int Cost = 0;
-  // Collect the opcodes of all the instructions that will be needed to expand
-  // the SCEVExpr. This is so that when we come to cost the operands, we know
-  // what the generated user(s) will be.
-  SmallVector<unsigned, 2> Opcodes;
+  // Object to help map SCEV operands to expanded IR instructions.
+  struct OperationIndices {
----------------
This needs a better comment.
Min/Max Idx are pretty magical on the first look.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2189
+  struct OperationIndices {
+    OperationIndices(unsigned Opc, size_t min, size_t max) :
+      Opcode(Opc), MinIdx(min), MaxIdx(max) { }
----------------
Not llvm coding style in any case.
These should likely be `Min`/`Max`


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2344
+  if (auto *Constant = dyn_cast<SCEVConstant>(S)) {
+    // Only evalulate the costs of constants when optimizing for size.
+    if (CostKind != TargetTransformInfo::TCK_CodeSize)
----------------
Consider constants to be free unless we are optforsize


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2347-2348
+      return 0;
+    const APInt &Imm = Constant->getAPInt();
+    Type *Ty = S->getType();
+    BudgetRemaining -=
----------------
These are only used in a single place


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2352
+                            Imm, Ty, CostKind);
+    return BudgetRemaining < 0;
+  } else if (isa<SCEVCastExpr>(S)) {
----------------
This is inconsistent with every other return here.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2192-2193
+    unsigned Opcode;
+    size_t MinIdx;
+    size_t MaxIdx;
+  };
----------------
`SCEVOperand::OperandIdx` is `int`


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2195
   case scUnknown:
   case scConstant:
     return false; // Assume to be zero-cost.
----------------
lebedev.ri wrote:
> Can we still get a constant here?
Please do mark done comments as such.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76434