[PATCH] D97466: NFC: Change getUserCost to return InstructionCost

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/Support/InstructionCost.h:213-221
+inline bool operator==(const InstructionCost::CostType LHS,
+                       const InstructionCost &RHS) {
+  return RHS == LHS;
+}
+
+inline bool operator!=(const InstructionCost::CostType LHS,
+                       const InstructionCost &RHS) {
----------------
paulwalker-arm wrote:
> Are these strictly necessary? I'm wondering if there's just the odd comparison somewhere whose operands can be switched.
> 
> Or is it the case that these are a temporary measure that will be removed once the current LHS becomes an InstructionCost?  If so I think it's worth adding a `//TODO: Remove me when...`
I've removed them, it was indeed the odd comparison in InlineCost.cpp.


================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:2155
   // inlining.
-  unsigned Cost = 0;
+  InstructionCost Cost = 0;
   for (auto *BB : L->getBlocks()) {
----------------
paulwalker-arm wrote:
> More of an open question but is this initialisation required/preferred? Or to put another way, what is the expected meaning of InstructionCost()? Invalid? Zero?
The default `InstructionCost()` initialises with a valid `0`. I would prefer to remove that constructor in favour of initializing it with an explicit value (when this is used for an accumulating cost value) or Invalid when used as 'uninitialized'. The only issue is that this requires some more changes, because it's used in a `std::pair<>` in e.g. LoopVectorize.cpp, where it needs a default constructor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97466