[PATCH] D140263: [NFC] Vastly simplifies TypeSize

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added a comment.

I like this patch as now I almost understand what these three classes are. But please see the comments.



================
Comment at: llvm/include/llvm/Support/TypeSize.h:91
+// FixedOrScalableQuantity - base class for fixed- or scalable sizes.
+//  ^  ^
+//  |  |
----------------
Would it be better to move the below comments to the derived classes? It is not immediately clear that the arrows represent relationship between classes.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:113
 
-  friend LeafTy &operator+=(LeafTy &LHS, const LeafTy &RHS) {
-    assert(LHS.UnivariateDim == RHS.UnivariateDim && "Invalid dimensions");
-    LHS.Value += RHS.Value;
-    return LHS;
+  friend LeafTy &operator+=(LeafTy &Lhs, const LeafTy &Rhs) {
+    assert(Lhs.Scalable == Rhs.Scalable && "Invalid dimensions");
----------------
LHS and RHS (all capital) seems canonical abbreviation (892 occurences of LHS, 16 occurences of Lhs in CodeGen). Retaining the names would also make the diff smaller and make it //a lot// easier to review.



================
Comment at: llvm/include/llvm/Support/TypeSize.h:153
+private:
+  auto tie() const { return std::tie(MinValue, Scalable); }
+
----------------
Personally, this looks redundant to me (saving a few characters?).



================
Comment at: llvm/include/llvm/Support/TypeSize.h:168
 
-  /// Add \p RHS to the value at the univariate dimension.
-  LeafTy getWithIncrement(ScalarTy RHS) const {
-    return static_cast<LeafTy>(
-        UnivariateLinearPolyBase(Value + RHS, UnivariateDim));
+  /// Add \p Rhs to the value at the univariate dimension.
+  LeafTy getWithIncrement(ScalarTy Rhs) const {
----------------
univariate dimension --> fixed quantity?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140263