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

Wed Nov 25 05:32:02 PST 2020

sdesmalen added inline comments.

================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:64
+  // and comparisons.
+  CostType getValue() const { return Value; }
+
----------------
fhahn wrote:
> sdesmalen wrote:
> > fhahn wrote:
> > > 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?
> > > 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?
> > The latter. The other possible state I can think of is 'Unknown', which would be similar to Invalid in that it doesn't have an actual value. It may be that Invalid can be used to represent it, and that distinguishing this state will never be necessary, but this implementation leaves that possibility open.
> > The latter. The other possible state I can think of is 'Unknown', which would be similar to Invalid in that it doesn't have an actual value. It may be that Invalid can be used to represent it, and that distinguishing this state will never be necessary, but this implementation leaves that possibility open.
> 
> Ok thanks, that makes sense! I just had another though about returning `Optional`. If getValue only ever returns not-none when the state is valid, then is the Optional needed? Couldn't we get the same thing by checking `isValid` in the caller, rather than in `getState` and have `assert(isValid())` in `getState`?
> 
> ```
> if (S.isValid())
>   Foo += S.getState();
> ```
[Assuming you mean s/getState/getValue/ ?]

If you read back the previous comments, you'll see that this was discussed.

The reason for Optional is because calling `getValue` might otherwise assert. This is not directly obvious when using this class, the only way to know that `getValue()` might assert is by reading the documentation of InstructionCost and getValue. By making that part of the interface, the caller will know it has to cope with the possibility of the value not being known, and modify the code accordingly. 

Perhaps the `isValid` and `isInvalid` methods are only creating unnecessary confusion, and perhaps we should remove those in favour of some `getCostState()` function that returns the enum value, so that the user can check why the value is not known (Invalid or Unknown or ...).

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

https://reviews.llvm.org/D91174