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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.

Thanks for the changes. The patch looks good to me now.



================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:64
+  // and comparisons.
+  CostType getValue() const { return Value; }
+
----------------
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.


================
Comment at: llvm/lib/Analysis/CostModel.cpp:107
+      InstructionCost Cost = TTI->getInstructionCost(&Inst, CostKind);
+      if (!Cost.isInvalid())
         OS << "Cost Model: Found an estimated cost of " << Cost;
----------------
nit: `Cost.isValid()` ?

Or now that you've added `Optional`, you can write:
```if (Optional<int> Cost = TTI->getInstructionCost(&Inst, CostKind))
  OS << "Cost Model: Found an estimated cost of " << *Cost;
else
  OS << "Cost Model: Unknown cost";
```


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

https://reviews.llvm.org/D91174