[PATCH] D91174: [Analysis] Introduce a new InstructionCost class
Christopher Tetreault via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 3 05:40:58 PST 2020
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
More information about the llvm-commits
mailing list