[PATCH] D96030: NFC: Migrate CodeMetrics to work on InstructionCost

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Analysis/CodeMetrics.cpp:122
+  // properties.
+  InstructionCost NumInstsProxy = NumInsts;
+  InstructionCost NumInstsBeforeThisBB = NumInsts;
----------------
Hi @sdesmalen, I'm not entirely sure why we're using an InstructionCost here to be honest, when NumInsts is an unsigned and any new costs are immediately dereferenced below at line 183 after calling TTI.getUserCost?


================
Comment at: llvm/lib/Analysis/CodeMetrics.cpp:183
+    NumInstsProxy += TTI.getUserCost(&I, TargetTransformInfo::TCK_CodeSize);
+    NumInsts = *NumInstsProxy.getValue();
   }
----------------
Isn't this a rolling value that needs incrementing? It's an unsigned value in CodeMetrics.h.


================
Comment at: llvm/lib/Analysis/CodeMetrics.cpp:204
+  InstructionCost NumInstsThisBB = NumInstsProxy - NumInstsBeforeThisBB;
+  NumBBInsts[BB] = *NumInstsThisBB.getValue();
 }
----------------
sdesmalen wrote:
> FYI, the reason for dereferencing the Optional cost value returned by `getValue()` unconditionally is that if the number of instructions for a basic-block cannot be costed, the cost-model is broken, which should never happen.
I think actually NumInstsThisBB is guaranteed to be valid because NumInstsBeforeThisBB is initialised with an unsigned, and NumInstsProxy must be valid because if it wasn't we'd have already crashed with an assert on line 183.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96030