[PATCH] D91174: [Analysis] Introduce a new InstructionCost class

[David Sherwood via Phabricator via llvm-commits]

david-arm added a subscriber: vkmr.
david-arm added inline comments.


================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:64
+  // and comparisons.
+  CostType getValue() const { return Value; }
+
----------------
ctetreau wrote:
> sdesmalen wrote:
> > david-arm wrote:
> > > sdesmalen wrote:
> > > > Sorry for the late comment, I only just noticed this.
> > > > 
> > > > I don't think it makes sense to simply return `CostType`, because if the value is Invalid, then `Value` is garbage.
> > > > From what I can tell, the options are to either:
> > > > 1. `assert(isValid() && "Can't return value if it is invalid")`. If the assert fails, the caller needs to be modified to check the validity of the InstructionCost before continuing its calculation.
> > > > 2. Return `Optional<CostType>` to make it clear to the caller that the value _may_ not be valid.
> > > > 
> > > > Without having seen all the places that would need to use `getValue`, my preference would probably be 2, because it is more explicit that the Invalid state needs to be considered, whereas for 1, it may false give the impression that you can simply query the value without first checking it's validity.
> > > > 
> > > > Any thoughts?
> > > My personal preference would be for an assert, since I kind of thought the design was always that users should only call getValue() after checking for validity anyway. However, I didn't document this above getValue(), which I should do regardless of which choice we make. My concern with returning Optional is that users may be confused about how they are supposed to check for validity and we'll see people doing this:
> > > 
> > > if (Cost.getValue() != None)
> > >   BudgetRemaining -= *Cost.getValue()
> > > 
> > > instead of using the state-checking interfaces that we provided, i.e.
> > > 
> > > if (Cost.isValid())
> > >   BudgetRemaining -= Cost.getValue();
> > > 
> > The issue is that people won't necessarily read the documentation, so making it part of the interface seems like the better choice.
> > 
> > Also, at the moment `CostState` only has two states. If there is ever the need to add `IsUnknown`, then both `Unknown` and `Invalid` would not return a value, meaning that `isInvalid` and `Optional<>::None` would no longer be equivalent.
> > 
> > You could probably write the example above as:
> > 
> >   if (auto C = Cost.getValue())
> >     BudgetRemaining -= *C;
> > 
> I think an assert is fine. This InstructionCost is basically isomorphic to Optional<CostType> anyways, so requiring the user to convert to Optional, then get the maybe-value is a redundant conversion IMO.
I'm happy to go with the Optional route, but the class does already provide an interface that is future-proof, i.e. @vkmr requested I add the "isValid()" interface, which serves the identical purpose, i.e.

if (Cost.isValid())
  BudgetRemaining -= *(Cost.getValue())

I personally prefer users querying the state, but I guess that's personal preference. I agree having the Optional value forces callers to think about validity.


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

https://reviews.llvm.org/D91174