[AArch64] NFC: Simplify discombobulating 'requiresSMChange' interface#78703
Merged
sdesmalen-arm merged 1 commit intollvm:mainfrom Jan 19, 2024
Merged
Conversation
Having it return a `std::optional<bool>` is unnecessarily confusing. This patch changes it to a simple 'bool'. This patch also removes the 'BodyOverridesInterface' operand because there is only a single use for this which is easily rewritten.
Member
|
@llvm/pr-subscribers-backend-aarch64 Author: Sander de Smalen (sdesmalen-arm) ChangesHaving it return a This patch also removes the 'BodyOverridesInterface' operand because there is only a single use for this which is easily rewritten. Full diff: https://github.com/llvm/llvm-project/pull/78703.diff 5 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index 8a6f1dc7487bae8..b0548cc64fcf319 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -7650,8 +7650,7 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
}
SDValue PStateSM;
- std::optional<bool> RequiresSMChange =
- CallerAttrs.requiresSMChange(CalleeAttrs);
+ bool RequiresSMChange = CallerAttrs.requiresSMChange(CalleeAttrs);
if (RequiresSMChange) {
if (CallerAttrs.hasStreamingInterfaceOrBody())
PStateSM = DAG.getConstant(1, DL, MVT::i64);
@@ -7925,8 +7924,9 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
SDValue InGlue;
if (RequiresSMChange) {
- SDValue NewChain = changeStreamingMode(DAG, DL, *RequiresSMChange, Chain,
- InGlue, PStateSM, true);
+ SDValue NewChain =
+ changeStreamingMode(DAG, DL, CalleeAttrs.hasStreamingInterface(), Chain,
+ InGlue, PStateSM, true);
Chain = NewChain.getValue(0);
InGlue = NewChain.getValue(1);
}
@@ -8076,8 +8076,8 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
if (RequiresSMChange) {
assert(PStateSM && "Expected a PStateSM to be set");
- Result = changeStreamingMode(DAG, DL, !*RequiresSMChange, Result, InGlue,
- PStateSM, false);
+ Result = changeStreamingMode(DAG, DL, !CalleeAttrs.hasStreamingInterface(),
+ Result, InGlue, PStateSM, false);
}
if (RequiresLazySave) {
@@ -25479,8 +25479,7 @@ bool AArch64TargetLowering::fallBackToDAGISel(const Instruction &Inst) const {
if (auto *Base = dyn_cast<CallBase>(&Inst)) {
auto CallerAttrs = SMEAttrs(*Inst.getFunction());
auto CalleeAttrs = SMEAttrs(*Base);
- if (CallerAttrs.requiresSMChange(CalleeAttrs,
- /*BodyOverridesInterface=*/false) ||
+ if (CallerAttrs.requiresSMChange(CalleeAttrs) ||
CallerAttrs.requiresLazySave(CalleeAttrs))
return true;
}
diff --git a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp b/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
index d358a5c8bd9499f..08ae536fe9bbf0e 100644
--- a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
@@ -236,8 +236,9 @@ bool AArch64TTIImpl::areInlineCompatible(const Function *Caller,
return false;
if (CallerAttrs.requiresLazySave(CalleeAttrs) ||
- CallerAttrs.requiresSMChange(CalleeAttrs,
- /*BodyOverridesInterface=*/true)) {
+ (CallerAttrs.requiresSMChange(CalleeAttrs) &&
+ (!CallerAttrs.hasStreamingInterfaceOrBody() ||
+ !CalleeAttrs.hasStreamingBody()))) {
if (hasPossibleIncompatibleOps(Callee))
return false;
}
diff --git a/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp b/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp
index ccdec78d7808663..9693b6a664be262 100644
--- a/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp
+++ b/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp
@@ -82,27 +82,17 @@ SMEAttrs::SMEAttrs(const AttributeList &Attrs) {
Bitmask |= encodeZT0State(StateValue::New);
}
-std::optional<bool>
-SMEAttrs::requiresSMChange(const SMEAttrs &Callee,
- bool BodyOverridesInterface) const {
- // If the transition is not through a call (e.g. when considering inlining)
- // and Callee has a streaming body, then we can ignore the interface of
- // Callee.
- if (BodyOverridesInterface && Callee.hasStreamingBody()) {
- return hasStreamingInterfaceOrBody() ? std::nullopt
- : std::optional<bool>(true);
- }
-
+bool SMEAttrs::requiresSMChange(const SMEAttrs &Callee) const {
if (Callee.hasStreamingCompatibleInterface())
- return std::nullopt;
+ return false;
// Both non-streaming
if (hasNonStreamingInterfaceAndBody() && Callee.hasNonStreamingInterface())
- return std::nullopt;
+ return false;
// Both streaming
if (hasStreamingInterfaceOrBody() && Callee.hasStreamingInterface())
- return std::nullopt;
+ return false;
- return Callee.hasStreamingInterface();
+ return true;
}
diff --git a/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h b/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h
index af2854856fb9796..6f622f1996a3a0a 100644
--- a/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h
+++ b/llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h
@@ -75,14 +75,7 @@ class SMEAttrs {
/// \return true if a call from Caller -> Callee requires a change in
/// streaming mode.
- /// If \p BodyOverridesInterface is true and Callee has a streaming body,
- /// then requiresSMChange considers a call to Callee as having a Streaming
- /// interface. This can be useful when considering e.g. inlining, where we
- /// explicitly want the body to overrule the interface (because after inlining
- /// the interface is no longer relevant).
- std::optional<bool>
- requiresSMChange(const SMEAttrs &Callee,
- bool BodyOverridesInterface = false) const;
+ bool requiresSMChange(const SMEAttrs &Callee) const;
// Interfaces to query PSTATE.ZA
bool hasNewZABody() const { return Bitmask & ZA_New; }
diff --git a/llvm/unittests/Target/AArch64/SMEAttributesTest.cpp b/llvm/unittests/Target/AArch64/SMEAttributesTest.cpp
index 2f7201464ba2f23..294e5571814246a 100644
--- a/llvm/unittests/Target/AArch64/SMEAttributesTest.cpp
+++ b/llvm/unittests/Target/AArch64/SMEAttributesTest.cpp
@@ -193,86 +193,51 @@ TEST(SMEAttributes, Transitions) {
ASSERT_FALSE(SA(SA::Normal).requiresSMChange(SA(SA::Normal)));
// Normal -> Normal + LocallyStreaming
ASSERT_FALSE(SA(SA::Normal).requiresSMChange(SA(SA::Normal | SA::SM_Body)));
- ASSERT_EQ(*SA(SA::Normal)
- .requiresSMChange(SA(SA::Normal | SA::SM_Body),
- /*BodyOverridesInterface=*/true),
- true);
// Normal -> Streaming
- ASSERT_EQ(*SA(SA::Normal).requiresSMChange(SA(SA::SM_Enabled)), true);
+ ASSERT_TRUE(SA(SA::Normal).requiresSMChange(SA(SA::SM_Enabled)));
// Normal -> Streaming + LocallyStreaming
- ASSERT_EQ(*SA(SA::Normal).requiresSMChange(SA(SA::SM_Enabled | SA::SM_Body)),
- true);
- ASSERT_EQ(*SA(SA::Normal)
- .requiresSMChange(SA(SA::SM_Enabled | SA::SM_Body),
- /*BodyOverridesInterface=*/true),
- true);
+ ASSERT_TRUE(
+ SA(SA::Normal).requiresSMChange(SA(SA::SM_Enabled | SA::SM_Body)));
// Normal -> Streaming-compatible
ASSERT_FALSE(SA(SA::Normal).requiresSMChange(SA(SA::SM_Compatible)));
// Normal -> Streaming-compatible + LocallyStreaming
ASSERT_FALSE(
SA(SA::Normal).requiresSMChange(SA(SA::SM_Compatible | SA::SM_Body)));
- ASSERT_EQ(*SA(SA::Normal)
- .requiresSMChange(SA(SA::SM_Compatible | SA::SM_Body),
- /*BodyOverridesInterface=*/true),
- true);
// Streaming -> Normal
- ASSERT_EQ(*SA(SA::SM_Enabled).requiresSMChange(SA(SA::Normal)), false);
+ ASSERT_TRUE(SA(SA::SM_Enabled).requiresSMChange(SA(SA::Normal)));
// Streaming -> Normal + LocallyStreaming
- ASSERT_EQ(*SA(SA::SM_Enabled).requiresSMChange(SA(SA::Normal | SA::SM_Body)),
- false);
- ASSERT_FALSE(SA(SA::SM_Enabled)
- .requiresSMChange(SA(SA::Normal | SA::SM_Body),
- /*BodyOverridesInterface=*/true));
+ ASSERT_TRUE(
+ SA(SA::SM_Enabled).requiresSMChange(SA(SA::Normal | SA::SM_Body)));
// Streaming -> Streaming
ASSERT_FALSE(SA(SA::SM_Enabled).requiresSMChange(SA(SA::SM_Enabled)));
// Streaming -> Streaming + LocallyStreaming
ASSERT_FALSE(
SA(SA::SM_Enabled).requiresSMChange(SA(SA::SM_Enabled | SA::SM_Body)));
- ASSERT_FALSE(SA(SA::SM_Enabled)
- .requiresSMChange(SA(SA::SM_Enabled | SA::SM_Body),
- /*BodyOverridesInterface=*/true));
// Streaming -> Streaming-compatible
ASSERT_FALSE(SA(SA::SM_Enabled).requiresSMChange(SA(SA::SM_Compatible)));
// Streaming -> Streaming-compatible + LocallyStreaming
ASSERT_FALSE(
SA(SA::SM_Enabled).requiresSMChange(SA(SA::SM_Compatible | SA::SM_Body)));
- ASSERT_FALSE(SA(SA::SM_Enabled)
- .requiresSMChange(SA(SA::SM_Compatible | SA::SM_Body),
- /*BodyOverridesInterface=*/true));
// Streaming-compatible -> Normal
- ASSERT_EQ(*SA(SA::SM_Compatible).requiresSMChange(SA(SA::Normal)), false);
- ASSERT_EQ(
- *SA(SA::SM_Compatible).requiresSMChange(SA(SA::Normal | SA::SM_Body)),
- false);
- ASSERT_EQ(*SA(SA::SM_Compatible)
- .requiresSMChange(SA(SA::Normal | SA::SM_Body),
- /*BodyOverridesInterface=*/true),
- true);
+ ASSERT_TRUE(SA(SA::SM_Compatible).requiresSMChange(SA(SA::Normal)));
+ ASSERT_TRUE(
+ SA(SA::SM_Compatible).requiresSMChange(SA(SA::Normal | SA::SM_Body)));
// Streaming-compatible -> Streaming
- ASSERT_EQ(*SA(SA::SM_Compatible).requiresSMChange(SA(SA::SM_Enabled)), true);
+ ASSERT_TRUE(SA(SA::SM_Compatible).requiresSMChange(SA(SA::SM_Enabled)));
// Streaming-compatible -> Streaming + LocallyStreaming
- ASSERT_EQ(
- *SA(SA::SM_Compatible).requiresSMChange(SA(SA::SM_Enabled | SA::SM_Body)),
- true);
- ASSERT_EQ(*SA(SA::SM_Compatible)
- .requiresSMChange(SA(SA::SM_Enabled | SA::SM_Body),
- /*BodyOverridesInterface=*/true),
- true);
+ ASSERT_TRUE(
+ SA(SA::SM_Compatible).requiresSMChange(SA(SA::SM_Enabled | SA::SM_Body)));
// Streaming-compatible -> Streaming-compatible
ASSERT_FALSE(SA(SA::SM_Compatible).requiresSMChange(SA(SA::SM_Compatible)));
// Streaming-compatible -> Streaming-compatible + LocallyStreaming
ASSERT_FALSE(SA(SA::SM_Compatible)
.requiresSMChange(SA(SA::SM_Compatible | SA::SM_Body)));
- ASSERT_EQ(*SA(SA::SM_Compatible)
- .requiresSMChange(SA(SA::SM_Compatible | SA::SM_Body),
- /*BodyOverridesInterface=*/true),
- true);
}
|
MDevereau
approved these changes
Jan 19, 2024
Contributor
MDevereau
left a comment
There was a problem hiding this comment.
This looks much clearer now, thanks.
rupprecht
pushed a commit
to rupprecht/llvm-project
that referenced
this pull request
Jan 19, 2024
…llvm#78703) Having it return a `std::optional<bool>` is unnecessarily confusing. This patch changes it to a simple 'bool'. This patch also removes the 'BodyOverridesInterface' operand because there is only a single use for this which is easily rewritten.
sdesmalen-arm
added a commit
to sdesmalen-arm/llvm-project
that referenced
this pull request
Jan 30, 2024
The issue didn't surface because the tests were not testing what they were supposed to test.
sdesmalen-arm
added a commit
that referenced
this pull request
Jan 31, 2024
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Jan 31, 2024
Calling a `__arm_locally_streaming` function from a function that is not a streaming-SVE function would lead to incorrect inlining. The issue didn't surface because the tests were not testing what they were supposed to test. (cherry picked from commit 3abf55a)
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Jan 31, 2024
Calling a `__arm_locally_streaming` function from a function that is not a streaming-SVE function would lead to incorrect inlining. The issue didn't surface because the tests were not testing what they were supposed to test. (cherry picked from commit 3abf55a)
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Feb 1, 2024
Calling a `__arm_locally_streaming` function from a function that is not a streaming-SVE function would lead to incorrect inlining. The issue didn't surface because the tests were not testing what they were supposed to test. (cherry picked from commit 3abf55a)
tstellar
pushed a commit
to tstellar/llvm-project
that referenced
this pull request
Feb 14, 2024
Calling a `__arm_locally_streaming` function from a function that is not a streaming-SVE function would lead to incorrect inlining. The issue didn't surface because the tests were not testing what they were supposed to test. (cherry picked from commit 3abf55a)
tstellar
pushed a commit
to tstellar/llvm-project
that referenced
this pull request
Feb 14, 2024
Calling a `__arm_locally_streaming` function from a function that is not a streaming-SVE function would lead to incorrect inlining. The issue didn't surface because the tests were not testing what they were supposed to test. (cherry picked from commit 3abf55a)
tstellar
pushed a commit
to tstellar/llvm-project
that referenced
this pull request
Feb 14, 2024
Calling a `__arm_locally_streaming` function from a function that is not a streaming-SVE function would lead to incorrect inlining. The issue didn't surface because the tests were not testing what they were supposed to test. (cherry picked from commit 3abf55a)
tstellar
pushed a commit
to tstellar/llvm-project
that referenced
this pull request
Feb 14, 2024
Calling a `__arm_locally_streaming` function from a function that is not a streaming-SVE function would lead to incorrect inlining. The issue didn't surface because the tests were not testing what they were supposed to test. (cherry picked from commit 3abf55a)
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Having it return a
std::optional<bool>is unnecessarily confusing. This patch changes it to a simple 'bool'.This patch also removes the 'BodyOverridesInterface' operand because there is only a single use for this which is easily rewritten.