[AArch64][SME2] Add ZT0 attributes to SMEAttrs #77607

kmclaughlin-arm · 2024-01-10T13:42:40Z

This patch extends SMEAttrs to interpret the following new attributes,
which are mutually exclusive and apply to SME2 only:

aarch64_sme_zt0_in (ZT0_In)
aarch64_sme_zt0_out (ZT0_Out)
aarch64_sme_zt0_inout (ZT0_InOut)
aarch64_sme_zt0_new (ZT0_New)
aarch64_sme_zt0_preserved (ZT0_Preserved)

ZT0_In, ZT0_Out, ZT0_InOut & ZT0_Preserved are all considered to share
ZT0. These attributes will be required by later patches to determine
if ZT0 should be preserved around function calls, or cleared on entry
to the function.

This patch extends SMEAttrs to interpret the following new attributes, which are mutually exclusive and apply to SME2 only: - aarch64_sme_zt0_in (ZT0_In) - aarch64_sme_zt0_out (ZT0_Out) - aarch64_sme_zt0_inout (ZT0_InOut) - aarch64_sme_zt0_new (ZT0_New) - aarch64_sme_zt0_preserved (ZT0_Preserved) ZT0_In, ZT0_Out, ZT0_InOut & ZT0_Preserved are all considered to share ZT0. These attributes will be required by later patches to determine if ZT0 should be preserved around function calls, or cleared on entry to the function.

llvmbot · 2024-01-10T13:42:59Z

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-backend-aarch64

Author: Kerry McLaughlin (kmclaughlin-arm)

Changes

This patch extends SMEAttrs to interpret the following new attributes,
which are mutually exclusive and apply to SME2 only:

aarch64_sme_zt0_in (ZT0_In)
aarch64_sme_zt0_out (ZT0_Out)
aarch64_sme_zt0_inout (ZT0_InOut)
aarch64_sme_zt0_new (ZT0_New)
aarch64_sme_zt0_preserved (ZT0_Preserved)

ZT0_In, ZT0_Out, ZT0_InOut & ZT0_Preserved are all considered to share
ZT0. These attributes will be required by later patches to determine
if ZT0 should be preserved around function calls, or cleared on entry
to the function.

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

3 Files Affected:

(modified) llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp (+18)
(modified) llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h (+24-8)
(modified) llvm/unittests/Target/AArch64/SMEAttributesTest.cpp (+103)

diff --git a/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp b/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp
index 0082b4017986c6..82b07419a244a0 100644
--- a/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp
+++ b/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp
@@ -18,8 +18,11 @@ void SMEAttrs::set(unsigned M, bool Enable) {
   else
     Bitmask &= ~M;
 
+  // Streaming Mode Attrs
   assert(!(hasStreamingInterface() && hasStreamingCompatibleInterface()) &&
          "SM_Enabled and SM_Compatible are mutually exclusive");
+
+  // ZA Attrs
   assert(!(hasNewZABody() && hasSharedZAInterface()) &&
          "ZA_New and ZA_Shared are mutually exclusive");
   assert(!(hasNewZABody() && preservesZA()) &&
@@ -28,6 +31,11 @@ void SMEAttrs::set(unsigned M, bool Enable) {
          "ZA_New and ZA_NoLazySave are mutually exclusive");
   assert(!(hasSharedZAInterface() && (Bitmask & ZA_NoLazySave)) &&
          "ZA_Shared and ZA_NoLazySave are mutually exclusive");
+
+  // ZT0 Attrs
+  assert((!sharesZT0() || (hasNewZT0Body() ^ isZT0In() ^ isZT0InOut() ^
+                           isZT0Out() ^ preservesZT0())) &&
+         "ZT0_New,In,Out,InOut,Preserved are mutually exclusive");
 }
 
 SMEAttrs::SMEAttrs(const CallBase &CB) {
@@ -60,6 +68,16 @@ SMEAttrs::SMEAttrs(const AttributeList &Attrs) {
     Bitmask |= ZA_New;
   if (Attrs.hasFnAttr("aarch64_pstate_za_preserved"))
     Bitmask |= ZA_Preserved;
+  if (Attrs.hasFnAttr("aarch64_sme_zt0_in"))
+    Bitmask |= ZT0_In;
+  if (Attrs.hasFnAttr("aarch64_sme_zt0_out"))
+    Bitmask |= ZT0_Out;
+  if (Attrs.hasFnAttr("aarch64_sme_zt0_inout"))
+    Bitmask |= ZT0_InOut;
+  if (Attrs.hasFnAttr("aarch64_sme_zt0_preserved"))
+    Bitmask |= ZT0_Preserved;
+  if (Attrs.hasFnAttr("aarch64_sme_zt0_new"))
+    Bitmask |= ZT0_New;
 }
 
 std::optional<bool>
diff --git a/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h b/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h
index e766b778b54102..88d67ad4b51f53 100644
--- a/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h
+++ b/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h
@@ -29,14 +29,19 @@ class SMEAttrs {
   // Enum with bitmasks for each individual SME feature.
   enum Mask {
     Normal = 0,
-    SM_Enabled = 1 << 0,    // aarch64_pstate_sm_enabled
-    SM_Compatible = 1 << 1, // aarch64_pstate_sm_compatible
-    SM_Body = 1 << 2,       // aarch64_pstate_sm_body
-    ZA_Shared = 1 << 3,     // aarch64_pstate_sm_shared
-    ZA_New = 1 << 4,        // aarch64_pstate_sm_new
-    ZA_Preserved = 1 << 5,  // aarch64_pstate_sm_preserved
-    ZA_NoLazySave = 1 << 6, // Used for SME ABI routines to avoid lazy saves
-    All = ZA_Preserved - 1
+    SM_Enabled = 1 << 0,     // aarch64_pstate_sm_enabled
+    SM_Compatible = 1 << 1,  // aarch64_pstate_sm_compatible
+    SM_Body = 1 << 2,        // aarch64_pstate_sm_body
+    ZA_Shared = 1 << 3,      // aarch64_pstate_sm_shared
+    ZA_New = 1 << 4,         // aarch64_pstate_sm_new
+    ZA_Preserved = 1 << 5,   // aarch64_pstate_sm_preserved
+    ZA_NoLazySave = 1 << 6,  // Used for SME ABI routines to avoid lazy saves
+    ZT0_New = 1 << 7,        // aarch64_sme_zt0_new
+    ZT0_In = 1 << 8,         // aarch64_sme_zt0_in
+    ZT0_Out = 1 << 9,        // aarch64_sme_zt0_out
+    ZT0_InOut = 1 << 10,     // aarch64_sme_zt0_inout
+    ZT0_Preserved = 1 << 11, // aarch64_sme_zt0_preserved
+    All = ZT0_Preserved - 1
   };
 
   SMEAttrs(unsigned Mask = Normal) : Bitmask(0) { set(Mask); }
@@ -86,6 +91,17 @@ class SMEAttrs {
     return hasZAState() && Callee.hasPrivateZAInterface() &&
            !(Callee.Bitmask & ZA_NoLazySave);
   }
+
+  // Interfaces to query ZT0 State
+  bool hasNewZT0Body() const { return Bitmask & ZT0_New; }
+  bool isZT0In() const { return Bitmask & ZT0_In; }
+  bool isZT0Out() const { return Bitmask & ZT0_Out; }
+  bool isZT0InOut() const { return Bitmask & ZT0_InOut; }
+  bool preservesZT0() const { return Bitmask & ZT0_Preserved; }
+  bool sharesZT0() const {
+    return Bitmask & (ZT0_In | ZT0_Out | ZT0_InOut | ZT0_Preserved);
+  }
+  bool hasZT0State() const { return hasNewZT0Body() || sharesZT0(); }
 };
 
 } // namespace llvm
diff --git a/llvm/unittests/Target/AArch64/SMEAttributesTest.cpp b/llvm/unittests/Target/AArch64/SMEAttributesTest.cpp
index 7780c71bbc00e0..9c88ec53589a73 100644
--- a/llvm/unittests/Target/AArch64/SMEAttributesTest.cpp
+++ b/llvm/unittests/Target/AArch64/SMEAttributesTest.cpp
@@ -50,6 +50,37 @@ TEST(SMEAttributes, Constructors) {
                       ->getFunction("foo"))
                   .preservesZA());
 
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_in\"")
+                      ->getFunction("foo"))
+                  .isZT0In());
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_out\"")
+                      ->getFunction("foo"))
+                  .isZT0Out());
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_inout\"")
+                      ->getFunction("foo"))
+                  .isZT0InOut());
+
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_in\"")
+                      ->getFunction("foo"))
+                  .sharesZT0());
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_out\"")
+                      ->getFunction("foo"))
+                  .sharesZT0());
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_inout\"")
+                      ->getFunction("foo"))
+                  .sharesZT0());
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_preserved\"")
+                      ->getFunction("foo"))
+                  .sharesZT0());
+
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_preserved\"")
+                      ->getFunction("foo"))
+                  .preservesZT0());
+
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_new\"")
+                      ->getFunction("foo"))
+                  .hasNewZT0Body());
+
   // Invalid combinations.
   EXPECT_DEBUG_DEATH(SA(SA::SM_Enabled | SA::SM_Compatible),
                      "SM_Enabled and SM_Compatible are mutually exclusive");
@@ -58,6 +89,29 @@ TEST(SMEAttributes, Constructors) {
   EXPECT_DEBUG_DEATH(SA(SA::ZA_New | SA::ZA_Preserved),
                      "ZA_New and ZA_Preserved are mutually exclusive");
 
+  EXPECT_DEBUG_DEATH(SA(SA::ZT0_New | SA::ZT0_In),
+                     "ZT0_New,In,Out,InOut,Preserved are mutually exclusive");
+  EXPECT_DEBUG_DEATH(SA(SA::ZT0_New | SA::ZT0_Out),
+                     "ZT0_New,In,Out,InOut,Preserved are mutually exclusive");
+  EXPECT_DEBUG_DEATH(SA(SA::ZT0_New | SA::ZT0_InOut),
+                     "ZT0_New,In,Out,InOut,Preserved are mutually exclusive");
+  EXPECT_DEBUG_DEATH(SA(SA::ZT0_New | SA::ZT0_Preserved),
+                     "ZT0_New,In,Out,InOut,Preserved are mutually exclusive");
+
+  EXPECT_DEBUG_DEATH(SA(SA::ZT0_In | SA::ZT0_Out),
+                     "ZT0_New,In,Out,InOut,Preserved are mutually exclusive");
+  EXPECT_DEBUG_DEATH(SA(SA::ZT0_In | SA::ZT0_InOut),
+                     "ZT0_New,In,Out,InOut,Preserved are mutually exclusive");
+  EXPECT_DEBUG_DEATH(SA(SA::ZT0_Out | SA::ZT0_InOut),
+                     "ZT0_New,In,Out,InOut,Preserved are mutually exclusive");
+
+  EXPECT_DEBUG_DEATH(SA(SA::ZT0_Preserved | SA::ZT0_In),
+                     "ZT0_New,In,Out,InOut,Preserved are mutually exclusive");
+  EXPECT_DEBUG_DEATH(SA(SA::ZT0_Preserved | SA::ZT0_Out),
+                     "ZT0_New,In,Out,InOut,Preserved are mutually exclusive");
+  EXPECT_DEBUG_DEATH(SA(SA::ZT0_Preserved | SA::ZT0_InOut),
+                     "ZT0_New,In,Out,InOut,Preserved are mutually exclusive");
+
   // Test that the set() methods equally check validity.
   EXPECT_DEBUG_DEATH(SA(SA::SM_Enabled).set(SA::SM_Compatible),
                      "SM_Enabled and SM_Compatible are mutually exclusive");
@@ -95,6 +149,55 @@ TEST(SMEAttributes, Basics) {
   ASSERT_FALSE(SA(SA::Normal).hasNewZABody());
   ASSERT_FALSE(SA(SA::Normal).hasZAState());
   ASSERT_FALSE(SA(SA::Normal).preservesZA());
+
+  // Test ZT0 State interfaces
+  ASSERT_TRUE(SA(SA::ZT0_In).isZT0In());
+  ASSERT_FALSE(SA(SA::ZT0_In).isZT0Out());
+  ASSERT_FALSE(SA(SA::ZT0_In).isZT0InOut());
+  ASSERT_FALSE(SA(SA::ZT0_In).preservesZT0());
+  ASSERT_FALSE(SA(SA::ZT0_In).hasNewZT0Body());
+  ASSERT_TRUE(SA(SA::ZT0_In).sharesZT0());
+  ASSERT_TRUE(SA(SA::ZT0_In).hasZT0State());
+
+  ASSERT_TRUE(SA(SA::ZT0_Out).isZT0Out());
+  ASSERT_FALSE(SA(SA::ZT0_Out).isZT0In());
+  ASSERT_FALSE(SA(SA::ZT0_Out).isZT0InOut());
+  ASSERT_FALSE(SA(SA::ZT0_Out).preservesZT0());
+  ASSERT_FALSE(SA(SA::ZT0_Out).hasNewZT0Body());
+  ASSERT_TRUE(SA(SA::ZT0_Out).sharesZT0());
+  ASSERT_TRUE(SA(SA::ZT0_Out).hasZT0State());
+
+  ASSERT_TRUE(SA(SA::ZT0_InOut).isZT0InOut());
+  ASSERT_FALSE(SA(SA::ZT0_InOut).isZT0In());
+  ASSERT_FALSE(SA(SA::ZT0_InOut).isZT0Out());
+  ASSERT_FALSE(SA(SA::ZT0_InOut).preservesZT0());
+  ASSERT_FALSE(SA(SA::ZT0_InOut).hasNewZT0Body());
+  ASSERT_TRUE(SA(SA::ZT0_InOut).sharesZT0());
+  ASSERT_TRUE(SA(SA::ZT0_InOut).hasZT0State());
+
+  ASSERT_TRUE(SA(SA::ZT0_Preserved).preservesZT0());
+  ASSERT_FALSE(SA(SA::ZT0_Preserved).isZT0In());
+  ASSERT_FALSE(SA(SA::ZT0_Preserved).isZT0Out());
+  ASSERT_FALSE(SA(SA::ZT0_Preserved).isZT0InOut());
+  ASSERT_FALSE(SA(SA::ZT0_Preserved).hasNewZT0Body());
+  ASSERT_TRUE(SA(SA::ZT0_Preserved).sharesZT0());
+  ASSERT_TRUE(SA(SA::ZT0_Preserved).hasZT0State());
+
+  ASSERT_TRUE(SA(SA::ZT0_New).hasNewZT0Body());
+  ASSERT_FALSE(SA(SA::ZT0_New).isZT0In());
+  ASSERT_FALSE(SA(SA::ZT0_New).isZT0Out());
+  ASSERT_FALSE(SA(SA::ZT0_New).isZT0InOut());
+  ASSERT_FALSE(SA(SA::ZT0_New).preservesZT0());
+  ASSERT_FALSE(SA(SA::ZT0_New).sharesZT0());
+  ASSERT_TRUE(SA(SA::ZT0_New).hasZT0State());
+
+  ASSERT_FALSE(SA(SA::Normal).isZT0In());
+  ASSERT_FALSE(SA(SA::Normal).isZT0Out());
+  ASSERT_FALSE(SA(SA::Normal).isZT0InOut());
+  ASSERT_FALSE(SA(SA::Normal).preservesZT0());
+  ASSERT_FALSE(SA(SA::Normal).hasNewZT0Body());
+  ASSERT_FALSE(SA(SA::Normal).sharesZT0());
+  ASSERT_FALSE(SA(SA::Normal).hasZT0State());
 }
 
 TEST(SMEAttributes, Transitions) {

llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp

…_shared is set - Changed hasSharedZAInterface() to return true if either sharedZA() or sharedZT0() is true - Changed wording of the assert checking for mutually exclusive ZT0 attributes

sdesmalen-arm · 2024-01-10T15:11:43Z

llvm/unittests/Target/AArch64/SMEAttributesTest.cpp

+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_in\"")
+                      ->getFunction("foo"))
+                  .sharesZT0());
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_out\"")
+                      ->getFunction("foo"))
+                  .sharesZT0());
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_inout\"")
+                      ->getFunction("foo"))
+                  .sharesZT0());
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_preserved\"")
+                      ->getFunction("foo"))
+                  .sharesZT0());


This is already tested below by e.g. ASSERT_TRUE(SA(SA::ZT0_In).hasZT0State());. There is no point in testing that here again (same for most cases below)

sdesmalen-arm · 2024-01-10T15:13:15Z

llvm/unittests/Target/AArch64/SMEAttributesTest.cpp

+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_new\"")
+                      ->getFunction("foo"))
+                  .hasNewZT0Body());
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_new\"")


Can you test this case below straight with ASSERT_TRUE(SA(SA::ZT0_New).hasPrivateZAInterface()), rather than doing it through some LLVM IR indirection?

sdesmalen-arm · 2024-01-10T15:14:14Z

llvm/unittests/Target/AArch64/SMEAttributesTest.cpp

+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_in\"")
+                      ->getFunction("foo"))
+                  .isZT0In());
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_out\"")
+                      ->getFunction("foo"))
+                  .isZT0Out());
+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_inout\"")
+                      ->getFunction("foo"))
+                  .isZT0InOut());


This is missing cases for aarch64_sme_zt0_preserved and aarch64_sme_zt0_new.

These are tested a bit further down, on line 93:

ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_preserved\"") ->getFunction("foo")) .preservesZT0()); ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_new\"") ->getFunction("foo")) .hasNewZT0Body());

sdesmalen-arm · 2024-01-10T17:08:12Z

llvm/unittests/Target/AArch64/SMEAttributesTest.cpp

@@ -111,36 +80,36 @@ TEST(SMEAttributes, Constructors) {

  EXPECT_DEBUG_DEATH(SA(SA::ZT0_New | SA::ZT0_In),
                     "ZT0_New, ZT0_In, ZT0_Out, ZT0_InOut and ZT0_Preserved "
-                     "are all mutually exclusive");
+                     "are all \" \"mutually exclusive");


Did something go wrong here with the escapes/formatting? :)

sdesmalen-arm · 2024-01-11T09:56:51Z

llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h

+    ZT0_New = 1 << 7,        // aarch64_sme_zt0_new
+    ZT0_In = 1 << 8,         // aarch64_sme_zt0_in
+    ZT0_Out = 1 << 9,        // aarch64_sme_zt0_out
+    ZT0_InOut = 1 << 10,     // aarch64_sme_zt0_inout
+    ZT0_Preserved = 1 << 11, // aarch64_sme_zt0_preserved


Given that they are all mutually exclusive, you could reserve a few bits to represent that state, i.e.

enum class StateValue { None = 0, In = 1, Out = 2, InOut = 3, Preserved = 4, New = 5 };

with the bits in enum Mask being:

ZT0_Shift = 7, ZT0_Mask = 0b111 << ZT0_Shift,

To get/set the SMEState value, you'd do something like this:

StateValue getZT0State() const { return BitMask & ZT0_Mask >> ZT0_Shift; } void setZT0State(StateValue S) { BitMask |= S << ZT0_Shift; }

(possibly with some added casts)

sdesmalen-arm · 2024-01-11T10:31:48Z

llvm/unittests/Target/AArch64/SMEAttributesTest.cpp

+  EXPECT_DEBUG_DEATH(SA(SA::ZT0_New | SA::ZT0_In),
+                     "ZT0_New, ZT0_In, ZT0_Out, ZT0_InOut and ZT0_Preserved "
+                     "are all \" \"mutually exclusive");
+  EXPECT_DEBUG_DEATH(SA(SA::ZT0_New | SA::ZT0_Out),


If you'd implement my suggestion above, there is no longer a need for these tests here.
But I would argue that we'd need some checks in Verifier.cpp for these attributes, which would be more user-facing.

I've added some checks in the Verifier pass, for now these only check that incompatible attributes are not added to the same function.

… ZT0 attributes - Added restrictions on the ZT0 attributes in the IR Verifier pass

sdesmalen-arm · 2024-01-15T15:22:12Z

llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h

+  void setZT0State(StateValue S) {
+    Bitmask |= (static_cast<unsigned>(S) << ZT0_Shift);
+  }


To enable the use of the SMEAttrs constructor using e.g. SMEAttrs(<mask>), perhaps it makes more sense to change setZT0State to something that encodes the value given a bitmask, i.e.

static unsigned decodeZT0State(unsigned Bitmask) { return static_cast<StateValue>((Bitmask & ZT0_Mask) >> ZT0_Shift); } static unsigned encodeZT0State(StateValue S) { return static_cast<unsigned>(S) << ZT0_Shift; }

That avoids having to do:

SA ZT0_In = SA(SA::Normal); ZT0_In.setZT0State(SA::StateValue::In);

and instead allows:

SA ZT0_In = SA(SA::encodeZT0State(SA::StateValue::In));

without the extra level of indirection.

sdesmalen-arm · 2024-01-15T15:25:06Z

llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h

+  bool isInZT0() const { return getZT0State() == StateValue::In; }
+  bool isOutZT0() const { return getZT0State() == StateValue::Out; }
+  bool isInOutZT0() const { return getZT0State() == StateValue::InOut; }
+  bool preservesZT0() const { return getZT0State() == StateValue::Preserved; }


Suggested change

bool preservesZT0() const { return getZT0State() == StateValue::Preserved; }

bool isPreservesZT0() const { return getZT0State() == StateValue::Preserved; }

sdesmalen-arm · 2024-01-15T15:26:04Z

llvm/unittests/Target/AArch64/SMEAttributesTest.cpp

@@ -50,6 +54,22 @@ TEST(SMEAttributes, Constructors) {
                      ->getFunction("foo"))
                  .preservesZA());

+  ASSERT_TRUE(SA(*parseIR("declare void @foo() \"aarch64_sme_zt0_in\"")


Should we change the names of the attributes from aarch64_sme_zt0_in to the aarch64_sme_in_zt0 format as well? (and similar below)

I've changed the attributes to use this format, I think this makes sense given the Clang attribute will be __arm_in("zt0")

sdesmalen-arm · 2024-01-15T15:26:47Z

llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h

+    Bitmask |= (static_cast<unsigned>(S) << ZT0_Shift);
+  }
+
+  bool hasNewZT0Body() const { return getZT0State() == StateValue::New; }


Suggested change

bool hasNewZT0Body() const { return getZT0State() == StateValue::New; }

bool isNewZT0() const { return getZT0State() == StateValue::New; }

sdesmalen-arm · 2024-01-15T15:36:38Z

llvm/lib/IR/Verifier.cpp

@@ -2160,6 +2160,64 @@ void Verifier::verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs,
           V);
  }

+  if (Attrs.hasFnAttr("aarch64_sme_zt0_new")) {


Rather than having to spell out all possible combinations here, my preference would be to do the same thing as you did for the assert, where the error is more generic, but where the check itself is simpler by using the exclusive-or operator (and therefore less error prone to missing a case).

…ttrs constructor with ZT0 attributes - Changed preservesZT0() to isPreservesZT0() - Changed hasNewZT0Body() to isNewZT0() - Renamed attributes, for example aarch64_sme_zt0_new -> aarch64_sme_new_zt0 - Combined the warnings added to the IR Verifier

sdesmalen-arm · 2024-01-15T19:58:45Z

llvm/lib/IR/Verifier.cpp

+  if (Attrs.hasFnAttr("aarch64_sme_new_zt0") ||
+      Attrs.hasFnAttr("aarch64_sme_in_zt0") ||
+      Attrs.hasFnAttr("aarch64_sme_inout_zt0") ||
+      Attrs.hasFnAttr("aarch64_sme_out_zt0") ||
+      Attrs.hasFnAttr("aarch64_sme_preserved_zt0")) {
+    Check((Attrs.hasFnAttr("aarch64_sme_new_zt0") ^
+           Attrs.hasFnAttr("aarch64_sme_in_zt0") ^
+           Attrs.hasFnAttr("aarch64_sme_inout_zt0") ^
+           Attrs.hasFnAttr("aarch64_sme_out_zt0") ^
+           Attrs.hasFnAttr("aarch64_sme_preserved_zt0")),


What about:

Check((Attrs.hasFnAttr("aarch64_sme_new_zt0") + Attrs.hasFnAttr("aarch64_sme_in_zt0") + Attrs.hasFnAttr("aarch64_sme_inout_zt0") + Attrs.hasFnAttr("aarch64_sme_out_zt0") + Attrs.hasFnAttr("aarch64_sme_preserved_zt0")) <= 1, "<message>", V);

?

This is much better, thanks :)

sdesmalen-arm · 2024-01-15T20:02:04Z

llvm/lib/IR/Verifier.cpp

+           Attrs.hasFnAttr("aarch64_sme_inout_zt0") ^
+           Attrs.hasFnAttr("aarch64_sme_out_zt0") ^
+           Attrs.hasFnAttr("aarch64_sme_preserved_zt0")),
+          "ZT0 state attributes 'aarch64_sme_[in|inout|out|new|preserved]_zt' "


nit: Just write out in full:

"Attributes 'aarch64_new_zt0', 'aarch64_in_zt0', 'aarch64_out_zt0', 'aarch64_inout_zt0' and 'aarch64_preserves_zt0' are mutually exclusive"

sdesmalen-arm · 2024-01-15T20:04:28Z

llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp

+         "ZT0_New, ZT0_In, ZT0_Out, ZT0_InOut and ZT0_Preserved are all "
+         "mutually exclusive");


Suggested change

"ZT0_New, ZT0_In, ZT0_Out, ZT0_InOut and ZT0_Preserved are all "

"mutually exclusive");

"Attributes 'aarch64_new_zt0', 'aarch64_in_zt0', 'aarch64_out_zt0', "

"'aarch64_inout_zt0' and 'aarch64_preserves_zt0' are mutually exclusive");

sdesmalen-arm · 2024-01-15T20:05:15Z

llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp

@@ -60,6 +69,16 @@ SMEAttrs::SMEAttrs(const AttributeList &Attrs) {
    Bitmask |= ZA_New;
  if (Attrs.hasFnAttr("aarch64_pstate_za_preserved"))
    Bitmask |= ZA_Preserved;
+  if (Attrs.hasFnAttr("aarch64_sme_in_zt0"))


Let's remove the _sme part from this, so that it's aarch64_in_zt0.

sdesmalen-arm · 2024-01-15T20:05:28Z

llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp

+    Bitmask |= encodeZT0State(StateValue::Out);
+  if (Attrs.hasFnAttr("aarch64_sme_inout_zt0"))
+    Bitmask |= encodeZT0State(StateValue::InOut);
+  if (Attrs.hasFnAttr("aarch64_sme_preserved_zt0"))


nit: preserves (not preserved)

sdesmalen-arm · 2024-01-15T20:07:58Z

llvm/unittests/Target/AArch64/SMEAttributesTest.cpp

  ASSERT_TRUE(SA(SA::ZA_Shared).hasZAState());
  ASSERT_FALSE(SA(SA::ZA_Shared).preservesZA());
  ASSERT_TRUE(SA(SA::ZA_Shared | SA::ZA_Preserved).preservesZA());

  ASSERT_TRUE(SA(SA::ZA_New).hasPrivateZAInterface());
+  ASSERT_FALSE(SA(SA::ZA_New).hasSharedZAInterface());


nit: add this case too?

ASSERT_FALSE(SA(SA::ZA_Shared).sharesZA());

There is already a sharesZA() test for SA::ZA_Shared on line 105, did you mean:
ASSERT_FALSE(SA(SA::ZA_Shared).sharesZT0())?

I've added sharesZT0() cases for SA::ZA_Shared & SA::ZA_New in the latest commit.

I'm not sure why I asked for this yesterday, perhaps my eyes just didn't spot this case.

- Changed warning to use full names of all mutually exclusive attributes - Removed 'sme' from attribute names, e.g. aarch64_sme_in_zt0 -> aarch64_in_zt0

sdesmalen-arm · 2024-01-16T12:22:38Z

llvm/unittests/Target/AArch64/SMEAttributesTest.cpp

  ASSERT_TRUE(SA(SA::ZA_Shared).hasZAState());
  ASSERT_FALSE(SA(SA::ZA_Shared).preservesZA());
  ASSERT_TRUE(SA(SA::ZA_Shared | SA::ZA_Preserved).preservesZA());

  ASSERT_TRUE(SA(SA::ZA_New).hasPrivateZAInterface());
+  ASSERT_FALSE(SA(SA::ZA_New).hasSharedZAInterface());


I'm not sure why I asked for this yesterday, perhaps my eyes just didn't spot this case.

This patch extends SMEAttrs to interpret the following new attributes, which are mutually exclusive and apply to SME2 only: - aarch64_sme_zt0_in (ZT0_In) - aarch64_sme_zt0_out (ZT0_Out) - aarch64_sme_zt0_inout (ZT0_InOut) - aarch64_sme_zt0_new (ZT0_New) - aarch64_sme_zt0_preserved (ZT0_Preserved) ZT0_In, ZT0_Out, ZT0_InOut & ZT0_Preserved are all considered to share ZT0. These attributes will be required by later patches to determine if ZT0 should be preserved around function calls, or cleared on entry to the function.

kmclaughlin-arm added the backend:AArch64 label Jan 10, 2024

kmclaughlin-arm requested review from SamTebbs33, dtemirbulatov, sdesmalen-arm and MDevereau January 10, 2024 13:42

kmclaughlin-arm mentioned this pull request Jan 10, 2024

[AArch64][SME2] Preserve ZT0 state around function calls #76968

Closed

MDevereau reviewed Jan 10, 2024

View reviewed changes

llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp Outdated Show resolved Hide resolved

- Added a sharedZA() function which returns true if aarch64_pstate_za…

da7076c

…_shared is set - Changed hasSharedZAInterface() to return true if either sharedZA() or sharedZT0() is true - Changed wording of the assert checking for mutually exclusive ZT0 attributes

sdesmalen-arm reviewed Jan 10, 2024

View reviewed changes

- Removed duplicate tests from SMEAttributesTest

f39fbb8

sdesmalen-arm reviewed Jan 10, 2024

View reviewed changes

sdesmalen-arm reviewed Jan 11, 2024

View reviewed changes

- Added an enum (StateValue) to AArch64SMEAttributes for representing…

7c70ad1

… ZT0 attributes - Added restrictions on the ZT0 attributes in the IR Verifier pass

llvmbot added the llvm:ir label Jan 12, 2024

sdesmalen-arm reviewed Jan 15, 2024

View reviewed changes

- Simplified ZT0 attribute checks in Verifier.cpp

c75e1b3

- Changed warning to use full names of all mutually exclusive attributes - Removed 'sme' from attribute names, e.g. aarch64_sme_in_zt0 -> aarch64_in_zt0

sdesmalen-arm approved these changes Jan 16, 2024

View reviewed changes

kmclaughlin-arm merged commit a4ec04e into llvm:main Jan 16, 2024
4 checks passed

kmclaughlin-arm deleted the sme2-new-zt0-attrs branch January 20, 2024 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AArch64][SME2] Add ZT0 attributes to SMEAttrs #77607

[AArch64][SME2] Add ZT0 attributes to SMEAttrs #77607

kmclaughlin-arm commented Jan 10, 2024

llvmbot commented Jan 10, 2024 •

edited

sdesmalen-arm Jan 10, 2024

sdesmalen-arm Jan 10, 2024

sdesmalen-arm Jan 10, 2024

kmclaughlin-arm Jan 10, 2024

sdesmalen-arm Jan 10, 2024

sdesmalen-arm Jan 11, 2024

sdesmalen-arm Jan 11, 2024

kmclaughlin-arm Jan 12, 2024

sdesmalen-arm Jan 15, 2024

sdesmalen-arm Jan 15, 2024

sdesmalen-arm Jan 15, 2024

kmclaughlin-arm Jan 15, 2024

sdesmalen-arm Jan 15, 2024

sdesmalen-arm Jan 15, 2024

sdesmalen-arm Jan 15, 2024

kmclaughlin-arm Jan 16, 2024

sdesmalen-arm Jan 15, 2024

sdesmalen-arm Jan 15, 2024

sdesmalen-arm Jan 15, 2024

sdesmalen-arm Jan 15, 2024

sdesmalen-arm Jan 15, 2024

kmclaughlin-arm Jan 16, 2024

sdesmalen-arm Jan 16, 2024

sdesmalen-arm Jan 16, 2024

	bool preservesZT0() const { return getZT0State() == StateValue::Preserved; }
	bool isPreservesZT0() const { return getZT0State() == StateValue::Preserved; }

	bool hasNewZT0Body() const { return getZT0State() == StateValue::New; }
	bool isNewZT0() const { return getZT0State() == StateValue::New; }

		"ZT0_New, ZT0_In, ZT0_Out, ZT0_InOut and ZT0_Preserved are all "
		"mutually exclusive");

[AArch64][SME2] Add ZT0 attributes to SMEAttrs #77607

[AArch64][SME2] Add ZT0 attributes to SMEAttrs #77607

Conversation

kmclaughlin-arm commented Jan 10, 2024

llvmbot commented Jan 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llvmbot commented Jan 10, 2024 •

edited