Skip to content

Commit

Permalink
[AArch64] NFC: Simplify discombobulating 'requiresSMChange' interface (…
Browse files Browse the repository at this point in the history 
…#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.
  • Loading branch information
@sdesmalen-arm
sdesmalen-arm authored Jan 19, 2024
1 parent 40a631f commit 5f41cef
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 80 deletions.
15 changes: 7 additions & 8 deletions llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7635,8 +7635,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);
Expand Down Expand Up @@ -7910,8 +7909,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);
}
Expand Down Expand Up @@ -8061,8 +8061,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) {
Expand Down Expand Up @@ -25463,8 +25463,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;
}
Expand Down
5 changes: 3 additions & 2 deletions llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
20 changes: 5 additions & 15 deletions llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
9 changes: 1 addition & 8 deletions llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand Down
59 changes: 12 additions & 47 deletions llvm/unittests/Target/AArch64/SMEAttributesTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

0 comments on commit 5f41cef

Please sign in to comment.