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

[Christopher Tetreault via Phabricator via llvm-commits]

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