[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;
----------------
paulwalker-arm wrote:
> 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.
Sure, I completely agree with you. My problem here is that I simply didn't know what value to set it to when invalid. It has to be set to something and I wasn't trying to second guess here - I was just trying to ensure it had some sort of obvious, debuggable value, rather than random data. The flag "Metrics.notDuplicatable" will be set to true when invalid and you'll see this is checked below, however I couldn't be sure that Props wouldn't still be checked even if notDuplicatable is true. The Props structure goes into a LoopsProperties map that is kept around for later - it's not thrown away if not duplicatable.


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

https://reviews.llvm.org/D91957