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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:57
+  }
+
+  bool isValid() const { return State == Valid; }
----------------
david-arm wrote:
> ctetreau wrote:
> > Feature request: Please add ctors and getters such that you can implicitly convert from various numeric types and Optionals. See D92178 for motivation for this. Locally, I added:
> > 
> > ```
> >   template <typename ConvertsToCostTypeT>
> >   InstructionCost(const ConvertsToCostTypeT& Val) : Value(Val), State(Valid) {}
> >   
> >   template <typename ConvertsToCostTypeT>
> >   InstructionCost(const Optional<ConvertsToCostTypeT>& Val) {
> >     if (Val) {
> >       Value = *Val;
> >       State = Valid;
> >     }
> >     else
> >       State = Invalid;
> >   }
> > 
> >   static InstructionCost getInvalid() {
> >     return getInvalid(0);
> >   }
> > 
> >   template <typename ConvertsToCostTypeT>
> >   static InstructionCost getInvalid(const ConvertsToCostTypeT& Val) {
> >     InstructionCost Tmp(Val);
> >     Tmp.setInvalid();
> >     return Tmp;
> >   }
> > ```
> I think this request seems sensible to me, although this code does fail debug builds for me due to ambiguous operator<< overloads. I think I can work around this with an explicit getter, i.e.
> 
>   template <typename T>
>   InstructionCost getValid(const T &Val) { ... }
> 
Only make this change if it's easy and doesn't negatively impact the ergonomics of using the class. Alternatively, you could add constructors for float and/or double instead of having the templated constructors.

I think the combinatorial explosion of having ctors for int, float, Optional<int>, Optional<float> probably wouldn't be too bad.


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

https://reviews.llvm.org/D91174