Skip to content

[mlir][ArmSME] Add enable_arm_streaming_ignore attribute #66911

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.

Sign up for GitHub

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

Merged
MacDue merged 3 commits into from
Sep 22, 2023
Merged

[mlir][ArmSME] Add enable_arm_streaming_ignore attribute #66911

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af4c803
[mlir][ArmSME] Add `enable_arm_streaming_ignore` attribute
MacDue Sep 20, 2023
1889687
Fixup: Add CHECK-NOTs
MacDue Sep 21, 2023
d86db01
Revert "Fixup: Add CHECK-NOTs"
MacDue Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion mlir/lib/Dialect/ArmSME/Transforms/EnableArmStreaming.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down
8 changes: 8 additions & 0 deletions mlir/test/Dialect/ArmSME/enable-arm-streaming.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Comment on lines +13 to +18
Copy link
Contributor

@banach-space banach-space Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

Copy link
Member Author

@MacDue MacDue Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

@banach-space banach-space Sep 22, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@MacDue MacDue Sep 22, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@banach-space banach-space Sep 22, 2023

Choose a reason for hiding this comment

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

There's probably some syntax for whole word matches

There's -match-full-lines

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