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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:66
+  // and comparisons. Users of this class can check for validity before trying
+  // to extract a value, for example using the isValid() interface provided
+  // above.
----------------
Is there a reason this function comment and others isn't a doxygen comment? (`///`) I am not sure if the comments will be used for the auto-generated docs with just `//`.


================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:64
+  // and comparisons.
+  CostType getValue() const { return Value; }
+
----------------
sdesmalen wrote:
> david-arm wrote:
> > 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.
> Thanks for adding the interface. You're right that at the moment there are two ways to query the same thing, but they are conceptually different. Otherwise, CostState would have been a `bool` instead of an enum and we could remove the `isValid` in favour of having `getValue` return an `Optional`. This allows for adding new states in the future without necessarily changing the places where InstructionCost is used.
It seems like the comment of this function needs to be updated now, because the current version still mentions the user can/should use the `isValid` interface?

I think for readers it would be good to describe what this function returns and when it returns none. And then follow with the notes about using it sparingly.

> Thanks for adding the interface. You're right that at the moment there are two ways to query the same thing, but they are conceptually different. Otherwise, CostState would have been a bool instead of an enum and we could remove the isValid in favour of having getValue return an Optional

Is the plan for `getValue` to return anything for any of the new states or will it only ever return the value for `Valid` states?


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

https://reviews.llvm.org/D91174