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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnswitch.cpp:306
 
-    Props.SizeEstimation = Metrics.NumInsts;
+    if (auto NumInsts = Metrics.NumInsts.getValue())
+      Props.SizeEstimation = *NumInsts;
----------------
CarolineConcatto wrote:
> This assumes that  Metrics.NumInsts.getValue(), should we check  if Metrics.NumInsts.isValid()?
> 
Yeah I could have used isValid() here too, but getValue() returns an Optional<int>, which cannot be used directly anyway. I thought it was cleaner to write it this way and only reference Metric.NumInsts once. The alternative would be to write:

if (Metrics.NumInsts.isValid())
  Props.SizeEstimation = *(Metrics.NumInsts.getValue());
else
  Props.SizeEstimation = std::numeric_limits<unsigned>::max();

Both ways of writing are functionally correct though! I've tended to use isValid() in places where we don't need to extract the value, i.e. for asserts, before doing comparisons or to bail out early from an optimisation.



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2600
 static bool
 unswitchBestCondition(Loop &L, DominatorTree &DT, LoopInfo &LI,
                       AssumptionCache &AC, TargetTransformInfo &TTI,
----------------
CarolineConcatto wrote:
> Why we do not use InstructionCost in this function when computing the Instruction costs?
> You've only replaced in one place, but not others.
This patch only changes code that's relevant to TTI.getUserCost(), etc and I've tried to avoid making the patch too large by only changing the places that absolutely had to use InstructionCost. The code below had existing asserts that the instruction cost is valid, so I thought I could do the same thing. If the InstructionCost is valid that means we can safely extract the value from InstructionCost and store as an integer in BBCostMap. It may be that at some point in the future we will encounter invalid costs, but I thought it was best not to try to guess what might happen and just wait to see if it's really a problem or not.


================
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
----------------
CarolineConcatto wrote:
> The behaviour of SpecCost invalid is the same if SpecCost is lower than CostSaving?
> Should LLVM_DEBUG inform that the costs are invalid?
The InstructionCost has an operator<< that prints out "Invalid" if the cost is invalid so the debug would look like this:

"  Not profitable, speculation cost: ...\n"
"    Cost savings:        ...\n"
"    SpecCost:        Invalid\n"

I just thought that was probably enough for the user to figure out something had gone wrong that's all.


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

https://reviews.llvm.org/D91957