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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
