fpetrogalli added inline comments.
================ Comment at: llvm/include/llvm/Support/TypeSize.h:56 + friend bool operator>(const ElementCount &LHS, const ElementCount &RHS) { + assert(LHS.Scalable == RHS.Scalable && ---------------- paulwalker-arm wrote: > david-arm wrote: > > ctetreau wrote: > > > fpetrogalli wrote: > > > > I think that @ctetreau is right on > > > > https://reviews.llvm.org/D85794#inline-793909. We should not overload a > > > > comparison operator on this class because the set it represent it > > > > cannot be ordered. > > > > > > > > Chris suggests an approach of writing a static function that can be > > > > used as a comparison operator, so that we can make it explicit of what > > > > kind of comparison we are doing. > > > In C++, it's common to overload the comparison operators for the purposes > > > of being able to std::sort and use ordered sets. Normally, I would be OK > > > with such usages. However, since `ElementCount` is basically a numeric > > > type, and they only have a partial ordering, I think this is dangerous. > > > I'm concerned that this will result in more bugs whereby somebody didn't > > > remember that vectors can be scalable. > > > > > > I don't have a strong opinion what the comparator function should be > > > called, but I strongly prefer that it not be a comparison operator. > > Hi @ctetreau, yeah I understand. The reason I chose to use operators was > > simply to be consistent with what we have already in TypeSize. Also, we > > have existing "==" and "!=" operators in ElementCount too, although these > > are essentially testing that two ElementCounts are identically the same or > > not, i.e. for 2 given polynomials (a + bx) and (c + dx) we're essentially > > asking if both a==c and b==d. > > > > If I introduce a new comparison function, I'll probably keep the asserts in > > for now, but in general we can do better than simply asserting if something > > is scalable or not. For example, we know that (vscale * 4) is definitely >= > > 4 because vscale is at least 1. I'm just not sure if we have that need yet. > I think we should treat the non-equality comparison functions more like > floating point. What we don't want is somebody writing !GreaterThan when > they actually mean LessThan. > > Perhaps we should name the functions accordingly (i.e. ogt for > OrderedAndGreaterThan). We will also need matching less than functions since > I can see those being useful when analysing constant insert/extract element > indices which stand a good chance to be a known comparison (with 0 being the > most common index). > May I suggest the following name scheme? (my 2 c, will not hold the patch for not addressing this comment) ``` static bool [Non]Total<cmp>(...) ``` with `<cmp>` being * `LT` -> less than, aka `<` * `LToE` -> less than or equal, aka "<=" * `GT` -> greater than, aka ">" * `GToE` -> greater than or equal, aka ">=" and `Total` , `NonTotal` being the prefix that gives information about the behavior on the value of scalable: `Total` -> for example, all scalable ECs are bigger than fixed ECs. `NonTotal` -> asserting on `(LHS.Scalable == RHS.Scalable)` before returning `LHS.Min <cmp> RHS.Min`. Taking it further: it could also be a template on an enumeration that list the type of comparisons? ``` enum CMPType { TotalGT, NonTotalLT, fancy_one }; ... template <unsigned T> static bool cmp(ElementCount &LHS, ElementCount &RHS ); ... static bool ElementCount::cmp<ElementCount::CMPType::TotalGT>(ElementCount &LHS, ElementCount &RHS ) { /// implementation } static bool ElementCount::cmp<ElementCount::CMPType::fancy_one>(ElementCount &LHS, ElementCount &RHS ) { /// implementation } ``` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86065/new/ https://reviews.llvm.org/D86065 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits