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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
