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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-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;
----------------
david-arm wrote:
> 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.
> 
For the case where the number of instructions is unknown why not treat this as a reason not to unswitch the loop rather than than trying to second guess how `Props.SizeEstimation` will be used when determining a sensible value to set it to?  You could even add an LLVM_DEBUG to report when this happens.

I think this will be a general comment of my for any such code where we want to support not knowing the cost of something or more specifically when the cost is invalid and thus unusable.


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

https://reviews.llvm.org/D91957