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

[AArch64][SME] Add remarks to flag lazy ZA saves, and SMSTART/SMSTOP transitions #68255

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

jroelofs
Copy link
Contributor

@jroelofs jroelofs commented Oct 4, 2023

No description provided.

@github-actions
Copy link

github-actions bot commented Oct 4, 2023

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 5082e827c1670f5c98d320d08979b8855253d0cc 2ed87db9fcfdb2c60bb1bf5ff95db3bf3f4943ad -- llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
View the diff from clang-format here.
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index 6c5a121d5c0f..db0103813411 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -7404,10 +7404,10 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
         TPIDR2ObjAddr);
     OptimizationRemarkEmitter ORE(&MF.getFunction());
     ORE.emit([&]() {
-      auto R = CLI.CB ? OptimizationRemarkAnalysis("sme", "SMELazySaveZA",
-                                                   CLI.CB)
-                      : OptimizationRemarkAnalysis("sme", "SMELazySaveZA",
-                                                   &MF.getFunction());
+      auto R = CLI.CB
+                   ? OptimizationRemarkAnalysis("sme", "SMELazySaveZA", CLI.CB)
+                   : OptimizationRemarkAnalysis("sme", "SMELazySaveZA",
+                                                &MF.getFunction());
       DescribeCallsite(R) << " sets up a lazy save for ZA";
       if (CalleeAttrs.preservesZA())
         R << ", but callee preserves ZA, so we request 0 slices to be saved";
@@ -7426,10 +7426,10 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
     PStateSM = getPStateSM(DAG, Chain, CallerAttrs, DL, MVT::i64);
     OptimizationRemarkEmitter ORE(&MF.getFunction());
     ORE.emit([&]() {
-      auto R = CLI.CB ? OptimizationRemarkAnalysis("sme", "SMETransition",
-                                                   CLI.CB)
-                      : OptimizationRemarkAnalysis("sme", "SMETransition",
-                                                   &MF.getFunction());
+      auto R = CLI.CB
+                   ? OptimizationRemarkAnalysis("sme", "SMETransition", CLI.CB)
+                   : OptimizationRemarkAnalysis("sme", "SMETransition",
+                                                &MF.getFunction());
       DescribeCallsite(R) << " requires a streaming mode transition";
       return R;
     });

Copy link
Collaborator

@sdesmalen-arm sdesmalen-arm left a comment

Choose a reason for hiding this comment

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

Thanks for adding this, this is very useful.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp Outdated Show resolved Hide resolved
@@ -7362,6 +7363,18 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
else if (auto *ES = dyn_cast<ExternalSymbolSDNode>(CLI.Callee))
CalleeAttrs = SMEAttrs(ES->getSymbol());

auto DescribeCallsite =
[&](OptimizationRemarkAnalysis &R) -> OptimizationRemarkAnalysis & {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Would it be easier to have a function that takes just a StringRef, so that you can do EmitSMERemarkForCall("requires a streaming mode transition");? You can then hide the construction of the ORE and OptimizationRemarkAnalysis in this lambda.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the current format would be a bit nicer for additions, i.e. if/when ZA liveness starts being tracked, the remark could be amended to mention the number of za slices that should be preserved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit weary of writing code in anticipation of future changes which may never happen. We can always rewrite the code in this form later if that makes more sense. That said, I don't feel too strongly about it since the current code isn't bad either.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp Outdated Show resolved Hide resolved
@@ -7362,6 +7363,18 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
else if (auto *ES = dyn_cast<ExternalSymbolSDNode>(CLI.Callee))
CalleeAttrs = SMEAttrs(ES->getSymbol());

auto DescribeCallsite =
[&](OptimizationRemarkAnalysis &R) -> OptimizationRemarkAnalysis & {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit weary of writing code in anticipation of future changes which may never happen. We can always rewrite the code in this form later if that makes more sense. That said, I don't feel too strongly about it since the current code isn't bad either.

@jroelofs jroelofs merged commit 2c0b6f2 into llvm:main Oct 6, 2023
2 of 3 checks passed
@jroelofs jroelofs deleted the jroelofs/sme branch October 6, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants