[mlir][ArmSME] Add `enable_arm_streaming_ignore` attribute #66911

MacDue · 2023-09-20T14:28:18Z

This attribute makes the enable_arm_streaming pass ignore a function (i.e. not add the enable streaming/za attributes). The main use case for this is to prevent helper functions within tests being made streaming functions.

This attribute makes the `enable_arm_streaming` pass ignore a function (i.e. not add the enable streaming/za attributes). The main use case for this is to prevent helper functions within tests being made streaming functions.

llvmbot · 2023-09-20T14:29:22Z

@llvm/pr-subscribers-mlir-sme

@llvm/pr-subscribers-mlir

Changes

This attribute makes the enable_arm_streaming pass ignore a function (i.e. not add the enable streaming/za attributes). The main use case for this is to prevent helper functions within tests being made streaming functions.

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

2 Files Affected:

(modified) mlir/lib/Dialect/ArmSME/Transforms/EnableArmStreaming.cpp (+5-1)
(modified) mlir/test/Dialect/ArmSME/enable-arm-streaming.mlir (+8)

diff --git a/mlir/lib/Dialect/ArmSME/Transforms/EnableArmStreaming.cpp b/mlir/lib/Dialect/ArmSME/Transforms/EnableArmStreaming.cpp
index 97c38b546349510..1d3a090e861013b 100644
--- a/mlir/lib/Dialect/ArmSME/Transforms/EnableArmStreaming.cpp
+++ b/mlir/lib/Dialect/ArmSME/Transforms/EnableArmStreaming.cpp
@@ -52,6 +52,8 @@ using namespace mlir::arm_sme;
 static constexpr char kArmStreamingAttr[] = "arm_streaming";
 static constexpr char kArmLocallyStreamingAttr[] = "arm_locally_streaming";
 static constexpr char kArmZAAttr[] = "arm_za";
+static constexpr char kEnableArmStreamingIgnoreAttr[] =
+    "enable_arm_streaming_ignore";
 
 namespace {
 struct EnableArmStreamingPass
@@ -61,7 +63,9 @@ struct EnableArmStreamingPass
     this->enableZA = enableZA;
   }
   void runOnOperation() override {
-    std::string attr;
+    if (getOperation()->getAttr(kEnableArmStreamingIgnoreAttr))
+      return;
+    StringRef attr;
     switch (mode) {
     case ArmStreaming::Default:
       attr = kArmStreamingAttr;
diff --git a/mlir/test/Dialect/ArmSME/enable-arm-streaming.mlir b/mlir/test/Dialect/ArmSME/enable-arm-streaming.mlir
index f5cc83192f9f6e1..e7bbe8c0047687d 100644
--- a/mlir/test/Dialect/ArmSME/enable-arm-streaming.mlir
+++ b/mlir/test/Dialect/ArmSME/enable-arm-streaming.mlir
@@ -9,3 +9,11 @@
 // CHECK-ENABLE-ZA-LABEL: @arm_streaming
 // CHECK-ENABLE-ZA-SAME: attributes {arm_streaming, arm_za}
 func.func @arm_streaming() { return }
+
+// CHECK-LABEL: @not_arm_streaming
+// CHECK-SAME: attributes {enable_arm_streaming_ignore}
+// CHECK-LOCALLY-LABEL: @not_arm_streaming
+// CHECK-LOCALLY-SAME: attributes {enable_arm_streaming_ignore}
+// CHECK-ENABLE-ZA-LABEL: @not_arm_streaming
+// CHECK-ENABLE-ZA-SAME: attributes {enable_arm_streaming_ignore}
+func.func @not_arm_streaming() attributes {enable_arm_streaming_ignore} { return }

MacDue · 2023-09-20T14:30:08Z

RE: #66691 (comment)

I think <pass_name> + _ignore makes the intent clearer, disable_arm_streaming implies that it was on before the pass, which is not the case.

banach-space · 2023-09-21T08:40:02Z

The main use case for this is to prevent helper functions within tests being made streaming functions.

Any chance to add a test that would demonstrate this?

MacDue · 2023-09-21T12:17:39Z

The main use case for this is to prevent helper functions within tests being made streaming functions.

Any chance to add a test that would demonstrate this?

I'm not sure what that'd be other than the test here (which shows the streaming attributes are not added no matter what enable_arm_streaming flags you set). In #66691 I use this attribute on helper functions within tests.

c-rhodes · 2023-09-21T12:20:11Z

The main use case for this is to prevent helper functions within tests being made streaming functions.

Any chance to add a test that would demonstrate this?

I'm not sure what that'd be other than the test here (which shows the streaming attributes are not added no matter what enable_arm_streaming flags you set). In #66691 I use this attribute on helper functions within tests.

I think the test you've added is sufficient.

banach-space · 2023-09-21T14:58:18Z

mlir/test/Dialect/ArmSME/enable-arm-streaming.mlir

+// CHECK-LABEL: @not_arm_streaming
+// CHECK-SAME: attributes {enable_arm_streaming_ignore}
+// CHECK-LOCALLY-LABEL: @not_arm_streaming
+// CHECK-LOCALLY-SAME: attributes {enable_arm_streaming_ignore}
+// CHECK-ENABLE-ZA-LABEL: @not_arm_streaming
+// CHECK-ENABLE-ZA-SAME: attributes {enable_arm_streaming_ignore}


I was after something like this - I think that CHECK-NOT is more descriptive in this context (i.e., we want to make sure that in presence of enable_arm_streaming_ignore we won't see these streaming SVE attributes)

Suggested change

// CHECK-LABEL: @not_arm_streaming

// CHECK-SAME: attributes {enable_arm_streaming_ignore}

// CHECK-LOCALLY-LABEL: @not_arm_streaming

// CHECK-LOCALLY-SAME: attributes {enable_arm_streaming_ignore}

// CHECK-ENABLE-ZA-LABEL: @not_arm_streaming

// CHECK-ENABLE-ZA-SAME: attributes {enable_arm_streaming_ignore}

// CHECK-LABEL: @not_arm_streaming

// CHECK-NOT: arm_streaming

// CHECK-LOCALLY-LABEL: @not_arm_streaming

// CHECK-LOCALLY-NOT: arm_locally_streaming

// CHECK-ENABLE-ZA-LABEL: @not_arm_streaming

// CHECK-ENABLE-ZA-NOT: arm_za

I could've explained better first time round, apologies :)

I've added CHECK-NOTs between the matches now (still with the CHECK-SAMEs too). I'm a little wary of just using CHECK-NOT alone (since if it's checking the wrong line it can falsely pass too).

The way this is written now, I think that you could have the following output and the test would pass (which is not what we want):

func.func @not_arm_streaming() attributes {enable_arm_streaming_ignore, arm_locally_streaming} { return }

So you'd need this to be 100% sure:

// CHECK-LABEL: @not_arm_streaming // CHECK-NOT: arm_streaming // CHECK-LOCALLY-LABEL: @not_arm_streaming // CHECK-NOT: arm_streaming

This should be sufficient though:

// CHECK-LABEL: @not_arm_streaming // CHECK-NOT: arm_streaming

This way, you match the label (which is unique) and the make sure that there's no arm_streaming attribute starting from the label all the way to EOF.

You have to do:

// CHECK-LABEL: @not_arm_streaming // CHECK-NOT: arm_streaming // CHECK-LOCALLY-LABEL: @not_arm_streaming // CHECK-NOT: arm_streaming

Because arm_streaming is a substring of enable_arm_streaming_ignore. There's probably some syntax for whole word matches, but I think that just using // CHECK-SAME: is a little simpler and easier to understand here.

There's probably some syntax for whole word matches

There's -match-full-lines

This reverts commit 1889687.

banach-space

LGTM, ta!

banach-space · 2023-09-22T09:07:35Z

mlir/test/Dialect/ArmSME/enable-arm-streaming.mlir

+// CHECK-LABEL: @not_arm_streaming
+// CHECK-SAME: attributes {enable_arm_streaming_ignore}
+// CHECK-LOCALLY-LABEL: @not_arm_streaming
+// CHECK-LOCALLY-SAME: attributes {enable_arm_streaming_ignore}
+// CHECK-ENABLE-ZA-LABEL: @not_arm_streaming
+// CHECK-ENABLE-ZA-SAME: attributes {enable_arm_streaming_ignore}


There's probably some syntax for whole word matches

There's -match-full-lines

This adds a custom lowering for SME that loops over each row of the tile, extracting it via an SME MOVA, then printing with a normal 1D vector.print. This makes writing SME integration tests easier and less verbose. Depends on: #66910, #66911

…6691) This adds a custom lowering for SME that loops over each row of the tile, extracting it via an SME MOVA, then printing with a normal 1D vector.print. This makes writing SME integration tests easier and less verbose. Depends on: llvm#66910, llvm#66911

[mlir][ArmSME] Add enable_arm_streaming_ignore attribute

af4c803

This attribute makes the `enable_arm_streaming` pass ignore a function (i.e. not add the enable streaming/za attributes). The main use case for this is to prevent helper functions within tests being made streaming functions.

MacDue requested review from c-rhodes, banach-space and dcaballe September 20, 2023 14:28

llvmbot added mlir mlir:sme labels Sep 20, 2023

MacDue mentioned this pull request Sep 20, 2023

[mlir][ArmSME] Add custom vector.print lowering for SME tiles #66691

Merged

dcaballe approved these changes Sep 20, 2023

View reviewed changes

c-rhodes approved these changes Sep 21, 2023

View reviewed changes

banach-space reviewed Sep 21, 2023

View reviewed changes

MacDue added 2 commits September 21, 2023 16:46

Fixup: Add CHECK-NOTs

1889687

Revert "Fixup: Add CHECK-NOTs"

d86db01

This reverts commit 1889687.

banach-space approved these changes Sep 22, 2023

View reviewed changes

MacDue merged commit 06f22c9 into llvm:main Sep 22, 2023

kstoimenov mentioned this pull request Sep 22, 2023

Add memcpm test kstoimenov/llvm-project#13

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mlir][ArmSME] Add `enable_arm_streaming_ignore` attribute #66911

[mlir][ArmSME] Add `enable_arm_streaming_ignore` attribute #66911

MacDue commented Sep 20, 2023

llvmbot commented Sep 20, 2023 •

edited

Loading

MacDue commented Sep 20, 2023

banach-space commented Sep 21, 2023

MacDue commented Sep 21, 2023

c-rhodes commented Sep 21, 2023

banach-space Sep 21, 2023 •

edited

Loading

MacDue Sep 21, 2023 •

edited

Loading

banach-space Sep 22, 2023

MacDue Sep 22, 2023

banach-space Sep 22, 2023

banach-space left a comment

banach-space Sep 22, 2023

[mlir][ArmSME] Add enable_arm_streaming_ignore attribute #66911

[mlir][ArmSME] Add enable_arm_streaming_ignore attribute #66911

Conversation

MacDue commented Sep 20, 2023

llvmbot commented Sep 20, 2023 • edited Loading

MacDue commented Sep 20, 2023

banach-space commented Sep 21, 2023

MacDue commented Sep 21, 2023

c-rhodes commented Sep 21, 2023

banach-space Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

MacDue Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

banach-space Sep 22, 2023

Choose a reason for hiding this comment

MacDue Sep 22, 2023

Choose a reason for hiding this comment

banach-space Sep 22, 2023

Choose a reason for hiding this comment

banach-space left a comment

Choose a reason for hiding this comment

banach-space Sep 22, 2023

Choose a reason for hiding this comment

[mlir][ArmSME] Add `enable_arm_streaming_ignore` attribute #66911

[mlir][ArmSME] Add `enable_arm_streaming_ignore` attribute #66911

llvmbot commented Sep 20, 2023 •

edited

Loading

banach-space Sep 21, 2023 •

edited

Loading

MacDue Sep 21, 2023 •

edited

Loading