[PATCH] D86065: [SVE] Make ElementCount members private

[Francesco Petrogalli via Phabricator via llvm-commits]

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