Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[llvm][TypeSize] Fix addition/subtraction in TypeSize. #72979

Merged
merged 2 commits into from
Nov 22, 2023

Conversation

sdesmalen-arm
Copy link
Collaborator

@sdesmalen-arm sdesmalen-arm commented Nov 21, 2023

It seems TypeSize is currently broken in the sense that:

TypeSize::Fixed(4) + TypeSize::Scalable(4) => TypeSize::Fixed(8)

without failing its assert that explicitly tests for this case:

assert(LHS.Scalable == RHS.Scalable && ...);

The reason this fails is that Scalable is a static method of class TypeSize,
and LHS and RHS are both objects of class TypeSize. So this is evaluating
if the pointer to the function Scalable == the pointer to the function Scalable,
which is always true because LHS and RHS have the same class.

This patch fixes the issue by renaming TypeSize::Scalable ->
TypeSize::getScalable, as well as TypeSize::Fixed to TypeSize::getFixed,
so that it no longer clashes with the variable in FixedOrScalableQuantity.

The new methods now also better match the coding standard, which specifies that:

  • Variable names should be nouns (as they represent state)
  • Function names should be verb phrases (as they represent actions)

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 21, 2023

@llvm/pr-subscribers-coroutines
@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-llvm-selectiondag
@llvm/pr-subscribers-mlir-llvm
@llvm/pr-subscribers-backend-directx
@llvm/pr-subscribers-mlir
@llvm/pr-subscribers-backend-webassembly
@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-debuginfo
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-llvm-support

Author: Sander de Smalen (sdesmalen-arm)

Changes

It seems TypeSize is currently broken in the sense that:

TypeSize::Fixed(4) + TypeSize::Scalable(4) => TypeSize::Fixed(8)

without failing its assert that explicitly tests for this case:

assert(LHS.Scalable == RHS.Scalable && ...);

The reason this fails is that Scalable is a static method of class TypeSize, and LHS and RHS are both objects of class TypeSize. So this is evaluating if the pointer to the function Scalable == the pointer to the function Scalable, which is always true because LHS and RHS have the same class.

This patch fixes the issue by renaming FixedOrScalableQuantity::Scalable -> FixedOrScalableQuantity::IsScalable so that it no longer clashes with the static method in TypeSize.

This patch also adds some new functionality, where it allows adding any quantity to a zero-initialized TypeSize, e.g.

TypeSize::Scalable(0) + TypeSize::Fixed(4) == TypeSize::Fixed(4)

This makes it easier to implement add-reductions using TypeSize.


Full diff: https://github.com/llvm/llvm-project/pull/72979.diff

2 Files Affected:

  • (modified) llvm/include/llvm/Support/TypeSize.h (+25-17)
  • (modified) llvm/unittests/Support/TypeSizeTest.cpp (+13)
diff --git a/llvm/include/llvm/Support/TypeSize.h b/llvm/include/llvm/Support/TypeSize.h
index 8e638f8278e828b..34c6c0ebfef25de 100644
--- a/llvm/include/llvm/Support/TypeSize.h
+++ b/llvm/include/llvm/Support/TypeSize.h
@@ -93,20 +93,26 @@ template <typename LeafTy, typename ValueTy> class FixedOrScalableQuantity {
 
 protected:
   ScalarTy Quantity = 0;
-  bool Scalable = false;
+  bool IsScalable = false;
 
   constexpr FixedOrScalableQuantity() = default;
-  constexpr FixedOrScalableQuantity(ScalarTy Quantity, bool Scalable)
-      : Quantity(Quantity), Scalable(Scalable) {}
+  constexpr FixedOrScalableQuantity(ScalarTy Quantity, bool IsScalable)
+      : Quantity(Quantity), IsScalable(IsScalable) {}
 
   friend constexpr LeafTy &operator+=(LeafTy &LHS, const LeafTy &RHS) {
-    assert(LHS.Scalable == RHS.Scalable && "Incompatible types");
+    assert((LHS.Quantity == 0 || RHS.Quantity == 0 ||
+            LHS.IsScalable == RHS.IsScalable) &&
+           "Incompatible types");
+    LHS.IsScalable = LHS.Quantity ? LHS.IsScalable : RHS.IsScalable;
     LHS.Quantity += RHS.Quantity;
     return LHS;
   }
 
   friend constexpr LeafTy &operator-=(LeafTy &LHS, const LeafTy &RHS) {
-    assert(LHS.Scalable == RHS.Scalable && "Incompatible types");
+    assert((LHS.Quantity == 0 || RHS.Quantity == 0 ||
+            LHS.IsScalable == RHS.IsScalable) &&
+           "Incompatible types");
+    LHS.IsScalable = LHS.Quantity ? LHS.IsScalable : RHS.IsScalable;
     LHS.Quantity -= RHS.Quantity;
     return LHS;
   }
@@ -140,11 +146,11 @@ template <typename LeafTy, typename ValueTy> class FixedOrScalableQuantity {
 
 public:
   constexpr bool operator==(const FixedOrScalableQuantity &RHS) const {
-    return Quantity == RHS.Quantity && Scalable == RHS.Scalable;
+    return Quantity == RHS.Quantity && IsScalable == RHS.IsScalable;
   }
 
   constexpr bool operator!=(const FixedOrScalableQuantity &RHS) const {
-    return Quantity != RHS.Quantity || Scalable != RHS.Scalable;
+    return Quantity != RHS.Quantity || IsScalable != RHS.IsScalable;
   }
 
   constexpr bool isZero() const { return Quantity == 0; }
@@ -155,14 +161,14 @@ template <typename LeafTy, typename ValueTy> class FixedOrScalableQuantity {
 
   /// Add \p RHS to the underlying quantity.
   constexpr LeafTy getWithIncrement(ScalarTy RHS) const {
-    return LeafTy::get(Quantity + RHS, Scalable);
+    return LeafTy::get(Quantity + RHS, IsScalable);
   }
 
   /// Returns the minimum value this quantity can represent.
   constexpr ScalarTy getKnownMinValue() const { return Quantity; }
 
   /// Returns whether the quantity is scaled by a runtime quantity (vscale).
-  constexpr bool isScalable() const { return Scalable; }
+  constexpr bool isScalable() const { return IsScalable; }
 
   /// A return value of true indicates we know at compile time that the number
   /// of elements (vscale * Min) is definitely even. However, returning false
@@ -277,8 +283,8 @@ template <typename LeafTy, typename ValueTy> class FixedOrScalableQuantity {
 //  - ElementCount::getScalable(4) : A scalable vector type holding 4 values.
 class ElementCount
     : public details::FixedOrScalableQuantity<ElementCount, unsigned> {
-  constexpr ElementCount(ScalarTy MinVal, bool Scalable)
-      : FixedOrScalableQuantity(MinVal, Scalable) {}
+  constexpr ElementCount(ScalarTy MinVal, bool IsScalable)
+      : FixedOrScalableQuantity(MinVal, IsScalable) {}
 
   constexpr ElementCount(
       const FixedOrScalableQuantity<ElementCount, unsigned> &V)
@@ -293,8 +299,8 @@ class ElementCount
   static constexpr ElementCount getScalable(ScalarTy MinVal) {
     return ElementCount(MinVal, true);
   }
-  static constexpr ElementCount get(ScalarTy MinVal, bool Scalable) {
-    return ElementCount(MinVal, Scalable);
+  static constexpr ElementCount get(ScalarTy MinVal, bool IsScalable) {
+    return ElementCount(MinVal, IsScalable);
   }
 
   /// Exactly one element.
@@ -315,11 +321,13 @@ class TypeSize : public details::FixedOrScalableQuantity<TypeSize, uint64_t> {
       : FixedOrScalableQuantity(V) {}
 
 public:
-  constexpr TypeSize(ScalarTy Quantity, bool Scalable)
-      : FixedOrScalableQuantity(Quantity, Scalable) {}
+  constexpr TypeSize() : FixedOrScalableQuantity(0, false) {}
 
-  static constexpr TypeSize get(ScalarTy Quantity, bool Scalable) {
-    return TypeSize(Quantity, Scalable);
+  constexpr TypeSize(ScalarTy Quantity, bool IsScalable)
+      : FixedOrScalableQuantity(Quantity, IsScalable) {}
+
+  static constexpr TypeSize get(ScalarTy Quantity, bool IsScalable) {
+    return TypeSize(Quantity, IsScalable);
   }
   static constexpr TypeSize Fixed(ScalarTy ExactSize) {
     return TypeSize(ExactSize, false);
diff --git a/llvm/unittests/Support/TypeSizeTest.cpp b/llvm/unittests/Support/TypeSizeTest.cpp
index 2850705d39f69f5..8ba68159e5f82ac 100644
--- a/llvm/unittests/Support/TypeSizeTest.cpp
+++ b/llvm/unittests/Support/TypeSizeTest.cpp
@@ -81,4 +81,17 @@ static_assert(INT64_C(2) * TSFixed32 == TypeSize::Fixed(64));
 static_assert(UINT64_C(2) * TSFixed32 == TypeSize::Fixed(64));
 static_assert(alignTo(TypeSize::Fixed(7), 8) == TypeSize::Fixed(8));
 
+static_assert(TypeSize() == TypeSize::Fixed(0));
+static_assert(TypeSize() != TypeSize::Scalable(0));
+static_assert(!TypeSize().isScalable());
+static_assert(TypeSize::Fixed(0) + TypeSize::Scalable(8) == TypeSize::Scalable(8));
+static_assert(TypeSize::Scalable(8) + TypeSize::Fixed(0) == TypeSize::Scalable(8));
+static_assert(TypeSize::Fixed(8) + TypeSize::Scalable(0) == TypeSize::Fixed(8));
+static_assert(TypeSize::Scalable(0) + TypeSize::Fixed(8) == TypeSize::Fixed(8));
+
+TEST(TypeSize, FailIncompatibleTypes) {
+  EXPECT_DEBUG_DEATH(TypeSize::Fixed(8) + TypeSize::Scalable(8),
+                     "Incompatible types");
+}
+
 } // namespace

Copy link

github-actions bot commented Nov 21, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@gchatelet
Copy link
Contributor

Thx for the PR. Can we rename the Scalable variable as a separate change?

@paulwalker-arm
Copy link
Collaborator

Strictly speaking, based on the coding standard the function naming is wrong rather than the variable?

@sdesmalen-arm
Copy link
Collaborator Author

@gchatelet I've split out the new feature (adding any value to zero-initialized TypeSize) into a separate PR #72994.
@paulwalker-arm I'm happy to rename the name of TypeSize::Scalable -> TypeSize::getScalable (or something) if there is consensus on the name, but I'd prefer do that in a separate NFC patch.

It seems TypeSize is currently broken in the sense that:

  TypeSize::Fixed(4) + TypeSize::Scalable(4) => TypeSize::Fixed(8)

without failing its assert that explicitly tests for this case:

  assert(LHS.Scalable == RHS.Scalable && ...);

The reason this fails is that `Scalable` is a static method of class TypeSize,
and LHS and RHS are both objects of class TypeSize. So this is evaluating
if the pointer to the function Scalable == the pointer to the function Scalable,
which is always true because LHS and RHS have the same class.

This patch fixes the issue by renaming `TypeSize::Scalable` ->
`TypeSize::getScalable`, as well as `TypeSize::Fixed` to `TypeSize::getFixed`,
so that it no longer clashes with the variable in FixedOrScalableQuantity.

The new methods now also better match the coding standard, which specifies that:
* Variable names should be nouns (as they represent state)
* Function names should be verb phrases (as they represent actions)
@gchatelet
Copy link
Contributor

The static functions renaming is going to produce a lot of noise but I guess this is too late already...
Shall we revert to keep the change minimal? @nikic @paulwalker-arm WDYT ?

Copy link
Collaborator

@paulwalker-arm paulwalker-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there's a code formatter issue with the MLIR change but otherwise this looks good to me.

@paulwalker-arm
Copy link
Collaborator

paulwalker-arm commented Nov 21, 2023

The static functions renaming is going to produce a lot of noise but I guess this is too late already... Shall we revert to keep the change minimal? @nikic @paulwalker-arm WDYT ?

For my money the functions were originally named correctly and then erroneously changed to (a) diverge from the coding standard, and (b) unknowingly introduced a bug caused by the new naming. For both reasons I'm happy to revert to the original naming via this patch.

@gchatelet
Copy link
Contributor

The static functions renaming is going to produce a lot of noise but I guess this is too late already... Shall we revert to keep the change minimal? @nikic @paulwalker-arm WDYT ?

For my money the functions were originally named correctly and then erroneously changed to (a) diverge from the coding standard, and (b) unknowingly introduced a bug caused by the new naming. For both reasons I'm happy to revert to the original naming via this patch.

That would be my patch indeed. Let's revert to the original naming via this patch.

@sdesmalen-arm
Copy link
Collaborator Author

Thanks @paulwalker-arm @gchatelet!

@sdesmalen-arm sdesmalen-arm merged commit 81b7f11 into llvm:main Nov 22, 2023
3 checks passed
nstester pushed a commit to nstester/llvm-project that referenced this pull request Nov 22, 2023
sdesmalen-arm added a commit that referenced this pull request Nov 27, 2023
…ic. (#72994)

This patch allows adding any quantity to a zero-initialized TypeSize,
such
that e.g.:

  TypeSize::Scalable(0) + TypeSize::Fixed(4) == TypeSize::Fixed(4)
  TypeSize::Fixed(0) + TypeSize::Scalable(4) == TypeSize::Scalable(4)

This makes it easier to implement add-reductions using TypeSize where
the 'scalable' flag is not yet known before starting the reduction.

(this PR follows on from #72979)
@sdesmalen-arm sdesmalen-arm deleted the fix-typesize branch February 23, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants