[TypeSize] Extend UnivariateLinearPolyBase with getWithIncrement/Decrement methods

[sdesmalen]

Summary

This patch adds getWithIncrement/getWithDecrement methods to
ElementCount and TypeSize to allow:

TypeSize::getFixed(8).getWithIncrement(8)     <=> TypeSize::getFixed(16)
TypeSize::getFixed(16).getWithDecrement(8)    <=> TypeSize::getFixed(8)
TypeSize::getScalable(8).getWithIncrement(8)  <=> TypeSize::getScalable(16)
TypeSize::getScalable(16).getWithDecrement(8) <=> TypeSize::getScalable(8)

This patch implements parts of the POC in D90342.

[dmgreen]

Can you just overload the + operator? Would that make the calling code more confusing?

I don't know this class very well so feel free to ignore me but I feel like it should be called "getWithIncrement" or something like it. That would be more like Type methods I've seen in the past like getWithNewBitwidth. Like I said if you are going for something else feel free to ignore me though.

[sdesmalen]

In D90713#2373445, @dmgreen wrote:

Can you just overload the + operator? Would that make the calling code more confusing?

In our SVE/SVE2 sync-up calls we settled on the approach of not using overloaded operators if the operation is not mathematically correct, and instead use named methods. This was the reason for removing operator/ from TypeSize and replacing it with divideCoefficientBy, because it only divides the (compile-time known) minimum value/coefficient and not the full runtime value in (coefficient * vscale) / RHS.

I don't know this class very well so feel free to ignore me but I feel like it should be called "getWithIncrement" or something like it. That would be more like Type methods I've seen in the past like getWithNewBitwidth. Like I said if you are going for something else feel free to ignore me though.

I like getWithIncrement, it seems a better name than add which indeed feels too similar to operator+. I wasn't able to think of a better name for it and therefore went with add, but I like this better. Thanks for the suggestion!

[sdesmalen]

• s/add/getWithIncrement/
• s/sub/getWithDecrement/

[dmgreen]

llvm/include/llvm/Support/TypeSize.h

241

I'm pretty sure my C++ has gotten worse since I started working on compilers.

Is the static_cast here needed? Will that end up just calling the new conversion constructor. Can it just create a LeafTy directly?

[sdesmalen]

llvm/include/llvm/Support/TypeSize.h

241

That's a fair question, I played around a bit with the options here. In the end went for a static_cast because it otherwise forces LeafTy to have a constructor of a certain shape (i.e. of whatever is called in this function). Otherwise it depends on a conversion constructor from parent -> child class, which always has the same form.

[dmgreen]

OK fair enough. If the other folks here don't disagree, this LGTM.

[ctetreau]

looks good to me

llvm/include/llvm/Support/TypeSize.h

public:
   static constexpr auto Dimensions = LinearPolyBaseTypeTraits<LeafTy>::Dimensions;
   static_assert(Dimensions != std::numeric_limits<unsigned>::max(),
                 "Dimensions out of range");
 
 protected:
   ScalarTy Value;         // The value at the univeriate dimension.
   unsigned UnivariateDim; // The univeriate dimension.
 
-  UnivariateLinearPolyBase(ScalarTy &Val, unsigned UnivariateDim)
+  UnivariateLinearPolyBase(ScalarTy Val, unsigned UnivariateDim)
       : Value(Val), UnivariateDim(UnivariateDim) {}
 
   friend LeafTy &operator+=(LeafTy &LHS, const LeafTy &RHS) {
     assert(LHS.UnivariateDim == RHS.UnivariateDim && "Invalid dimensions");
     LHS.Value += RHS.Value;
     return LHS;
   }
 
public:
 
   bool isZero() const { return !Value; }
   bool isNonZero() const { return !isZero(); }
   explicit operator bool() const { return isNonZero(); }
   ScalarTy getValue() const { return Value; }
   ScalarTy getValue(unsigned Dim) const {
     return Dim == UnivariateDim ? Value : 0;
   }
+
+  // Aliases for operator+ and operator- for completion, since we also
+  // support add/sub with a scalar type as well.
+  LeafTy add(const LeafTy &RHS) { return *this + RHS; }
+  LeafTy sub(const LeafTy &RHS) { return *this - RHS; }
+
+  /// Add \p RHS to the value at the univariate dimension.
+  LeafTy add(ScalarTy RHS) {
+    return static_cast<LeafTy>(
+        UnivariateLinearPolyBase(Value + RHS, UnivariateDim));
+  }
+
+  /// Subtract \p RHS from the value at the univariate dimension.
+  LeafTy sub(ScalarTy RHS) {
+    return static_cast<LeafTy>(
+        UnivariateLinearPolyBase(Value - RHS, UnivariateDim));
+  }
 };
 
 
 //===----------------------------------------------------------------------===//
 // LinearPolySize - base class for fixed- or scalable sizes.
 //  ^  ^ 
 //  |  |
 //  |  +----- ElementCount - Leaf class to represent an element count
 //  |                        (vscale x unsigned)
 //  |
 //  +-------- TypeSize - Leaf class to represent a type size
 //                       (vscale x uint64_t)
 //===----------------------------------------------------------------------===//
 
 /// LinearPolySize is a base class to represent sizes. It is either
 /// fixed-sized or it is scalable-sized, but it cannot be both.
 template <typename LeafTy>
 class LinearPolySize : public UnivariateLinearPolyBase<LeafTy> {
+  // Make the parent class a friend, so that it can access the protected
+  // conversion/copy-constructor for UnivariatePolyBase<LeafTy> ->
+  // LinearPolySize<LeafTy>.
+  friend class UnivariateLinearPolyBase<LeafTy>;
+
 public:
   using ScalarTy = typename UnivariateLinearPolyBase<LeafTy>::ScalarTy;
   enum Dims : unsigned { FixedDim = 0, ScalableDim = 1 };
 
 protected:
   LinearPolySize(ScalarTy MinVal, Dims D)
       : UnivariateLinearPolyBase<LeafTy>(MinVal, D) {}
 
+  LinearPolySize(const UnivariateLinearPolyBase<LeafTy> &V)
+      : UnivariateLinearPolyBase<LeafTy>(V) {}
+
 public:
+
   static LeafTy getFixed(ScalarTy MinVal) {
     return static_cast<LeafTy>(LinearPolySize(MinVal, FixedDim));
   }
   static LeafTy getScalable(ScalarTy MinVal) {
     return static_cast<LeafTy>(LinearPolySize(MinVal, ScalableDim));
   }
   static LeafTy get(ScalarTy MinVal, bool Scalable) {
     return static_cast<LeafTy>(

llvm/unittests/Support/LinearPolyBaseTest.cpp

TEST(UnivariateLinearPolyBase, Univariate3D_Add) {
   EXPECT_EQ(Univariate3D(42, 1) + Univariate3D(42, 1), Univariate3D(84, 1));
   EXPECT_DEBUG_DEATH(Univariate3D(42, 0) + Univariate3D(42, 1),
                      "Invalid dimensions");
 
   // Test operator+=
   Univariate3D X(42, 0);
   X += Univariate3D(42, 0);
   EXPECT_EQ(X, Univariate3D(84, 0));
+
+  // Test 'add' method
+  EXPECT_EQ(Univariate3D(42, 0).add(1), Univariate3D(43, 0));
+  EXPECT_EQ(Univariate3D(42, 1).add(2), Univariate3D(44, 1));
+  EXPECT_EQ(Univariate3D(42, 2).add(3), Univariate3D(45, 2));
 }
 
 TEST(UnivariateLinearPolyBase, Univariate3D_Sub) {
   // Test operator+
   EXPECT_EQ(Univariate3D(84, 0) - Univariate3D(42, 0), Univariate3D(42, 0));
   EXPECT_EQ(Univariate3D(84, 1) - Univariate3D(42, 1), Univariate3D(42, 1));
   EXPECT_DEBUG_DEATH(Univariate3D(84, 0) - Univariate3D(42, 1),
                      "Invalid dimensions");
 
   // Test operator+=
   Univariate3D X(84, 0);
   X -= Univariate3D(42, 0);
   EXPECT_EQ(X, Univariate3D(42, 0));
+
+  // Test 'sub' method
+  EXPECT_EQ(Univariate3D(43, 0).sub(1), Univariate3D(42, 0));
+  EXPECT_EQ(Univariate3D(44, 1).sub(2), Univariate3D(42, 1));
+  EXPECT_EQ(Univariate3D(45, 2).sub(3), Univariate3D(42, 2));
 }
 
 TEST(UnivariateLinearPolyBase, Univariate3D_Scale) {
   // Test operator*
   EXPECT_EQ(Univariate3D(4, 0) * 2, Univariate3D(8, 0));
   EXPECT_EQ(Univariate3D(4, 1) * -2, Univariate3D(-8, 1));
 }
 
 TEST(UnivariateLinearPolyBase, Univariate3D_Invert) {
   // Test operator-
   EXPECT_EQ(-Univariate3D(4, 0), Univariate3D(-4, 0));
   EXPECT_EQ(-Univariate3D(4, 1), Univariate3D(-4, 1));
 }