[PATCH] D91957: [Analysis] Migrate more high level cost functions to using InstructionCost

[Caroline via Phabricator via llvm-commits]

CarolineConcatto added a comment.

Thank you @david-arm for the patch. 
It helps to understand a bit more how to use the new class InstructionCost in D91174 <https://reviews.llvm.org/D91174>
I hope you don't mind but I have a couple of questions.



================
Comment at: llvm/lib/Transforms/Scalar/LoopUnswitch.cpp:306
 
-    Props.SizeEstimation = Metrics.NumInsts;
+    if (auto NumInsts = Metrics.NumInsts.getValue())
+      Props.SizeEstimation = *NumInsts;
----------------
This assumes that  Metrics.NumInsts.getValue(), should we check  if Metrics.NumInsts.isValid()?



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2600
 static bool
 unswitchBestCondition(Loop &L, DominatorTree &DT, LoopInfo &LI,
                       AssumptionCache &AC, TargetTransformInfo &TTI,
----------------
Why we do not use InstructionCost in this function when computing the Instruction costs?
You've only replaced in one place, but not others.


================
Comment at: llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:506
           int CostSavings = CostSavingsMap.find(PN)->second;
-          if (SpecCost > CostSavings) {
+          if (!SpecCost.isValid() || SpecCost > CostSavings) {
             LLVM_DEBUG(dbgs() << "  Not profitable, speculation cost: " << *PN
----------------
The behaviour of SpecCost invalid is the same if SpecCost is lower than CostSaving?
Should LLVM_DEBUG inform that the costs are invalid?


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

https://reviews.llvm.org/D91957