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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


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



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

https://reviews.llvm.org/D91174