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

David Sherwood via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 8 06:04:45 PST 2020


david-arm marked 18 inline comments as done.
david-arm added a comment.

Hi all, thanks everyone for all the reviews so far! I hope I've addressed everyone's comments with either code changes or giving clear enough reasons for the motivation behind certain aspects of the new class. Given it's been accepted by two people, @ctetreau and @sdesmalen, I'd like to merge the patch this week if that's ok? I'll wait until Friday to merge in case there are any strong objections.



================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:57
+  }
+
+  bool isValid() const { return State == Valid; }
----------------
ctetreau wrote:
> 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.
Hi @ctetreau, I think it's simpler if I leave the patch as it is now if that's ok? I just thought that if we want to add new constructors then perhaps it's best to do it as part of the vectoriser work, where we  move to using InstructionCost. It makes it a bit more obvious then why we're doing it.


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

https://reviews.llvm.org/D91174



More information about the llvm-commits mailing list