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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau 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 &&
----------------
fpetrogalli wrote:
> 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
> }
> ```
> 
> 
Honestly, I think this is actually worse. My issue is the fact that, from a mathematical perspective, `vscale_1 * min_1 < vscale_2 * min_2` is a function of `vscale_1` and `vscale_2`. In principle, we can know some ordering relationships between certain element counts (such as `vscale * min_1 >= min_2 = true`), but in general, this function does not make sense. However, an `operator<` is useful because it allows you to put an `ElementCount` into an ordered set, and it will just work. Renaming the function to `olt` just makes it so that you can't put `ElementCount`s into an ordered set, but still implies that `ElementCount`s are comparable in general. This will also blow up if you actually try to mix fixed width and scalable `ElementCount`s in an ordered set, which should work IMO.

Here is what I propose:

1) Add a predicate for establishing an arbitrary ordering. The predicate would be completely arbitrary, because it's only useful for establishing an ordering for an ordered set or in a sorting algorithm. It could look something like this:

```
static bool orderedBefore(const ElementCount &LHS, const ElementCount &RHS) {
  auto l = std::tie(LHS.Scalable, LHS.Min);
  auto r = std::tie(RHS.Scalable, RHS.Min);
  return l < r;
}
```

2) don't add any sort of mathematical comparison functions. Code working with `ElementCount` almost certainly either inspects the Scalable field and does something with the Min:

```
if (EC.isScalable()) {
    unsigned min = EC.getKnownMinValue();
    ... // do stuff with min
```

... or just uses it as a unit:

```
auto * VecTy = VectorType::get(SomeTy, EC);
```

I do not think that having the relation operators on ElementCount would simplify very much code. However, it is very easy to use incorrectly, and if it is ever extended in the future (one machine with two different `vscale` values? `vscale == 0` becoming valid?), it would become even worse. Best just not open that door.


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

https://reviews.llvm.org/D86065