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

[David Sherwood via Phabricator via cfe-commits]

david-arm added a comment.

Hi @ctetreau, I agree with @efriedma that keeping the two classes distinct for now seems best. The reason is I spent quite a lot of time trying to unify these classes already and I hit a stumbling block - TypeSize has the ugly uint64_t() cast operator, which makes unifying difficult. I didn't want to introduce a templated cast operator that ElementCount would then have too. I also tried making TypeSize derive from a templated parent, but that was pretty ugly too. Perhaps once we've removed the TypeSize -> uint64_t we might be better able to consider it?



================
Comment at: llvm/include/llvm/Support/TypeSize.h:56
 
+  friend bool operator>(const ElementCount &LHS, const ElementCount &RHS) {
+    assert(LHS.Scalable == RHS.Scalable &&
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86065