ctetreau added a comment.
Perhaps now would be a good time to combine TypeSize and ElementCount into a
single Polynomial type? We don't have to implement the whole abstraction of
`c*x^n` (since we currently don't use the exponent, and don't distinguish
between X's) but if it's ever needed in the future it will be obvious where to
add it, and it will Just Work.
================
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:
> 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.
================
Comment at: llvm/include/llvm/Support/TypeSize.h:290
static unsigned getHashValue(const ElementCount& EltCnt) {
- if (EltCnt.Scalable)
- return (EltCnt.Min * 37U) - 1U;
+ if (EltCnt.isScalable())
+ return (EltCnt.getKnownMinValue() * 37U) - 1U;
----------------
NIT: this can be rewritten without duplicating `EltCnt.getKnownMinValue() * 37U`
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3522
+ DAG.getNode(ISD::EXTRACT_SUBVECTOR, Dl,
+ MemVT.getHalfNumVectorElementsVT(*DAG.getContext()),
+ StoreNode->getValue(),
----------------
What is this `>>` thing? Some indicator of whitespace change, or is this a hard
tab?
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
