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

Reply via email to