Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
15 changes: 7 additions & 8 deletions clang/lib/Sema/SemaARM.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -567,23 +567,22 @@ static bool checkArmStreamingBuiltin(Sema &S, CallExpr *TheCall,
// * When compiling for SVE only, the caller must be in non-streaming mode.
// * When compiling for both SVE and SME, the caller can be in either mode.
if (BuiltinType == SemaARM::VerifyRuntimeMode) {
auto DisableFeatures = [](llvm::StringMap<bool> &Map, StringRef S) {
for (StringRef K : Map.keys())
if (K.starts_with(S))
Map[K] = false;
};

llvm::StringMap<bool> CallerFeatureMapWithoutSVE;
S.Context.getFunctionFeatureMap(CallerFeatureMapWithoutSVE, FD);
DisableFeatures(CallerFeatureMapWithoutSVE, "sve");
for (StringRef Feat : {"sve", "sve2", "sve2p1", "sve2-aes", "sve2-sha3",
"sve2-sm4", "sve2-bitperm"})
CallerFeatureMapWithoutSVE[Feat] = false;

// Avoid emitting diagnostics for a function that can never compile.
if (FnType == SemaARM::ArmStreaming && !CallerFeatureMapWithoutSVE["sme"])
return false;

llvm::StringMap<bool> CallerFeatureMapWithoutSME;
S.Context.getFunctionFeatureMap(CallerFeatureMapWithoutSME, FD);
DisableFeatures(CallerFeatureMapWithoutSME, "sme");
for (StringRef Feat :
{"sme", "sme2", "sme2p1", "sme-f64f64", "sme-i16i64", "sme-b16b16",
"sme-f16f16", "sme-f8f32", "sme-f8f16"})
CallerFeatureMapWithoutSME[Feat] = false;

// We know the builtin requires either some combination of SVE flags, or
// some combination of SME flags, but we need to figure out which part
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ svfloat32_t good6(svfloat32_t a, svfloat32_t b, svfloat32_t c) __arm_streaming_c
return svclamp(a, b, c);
}

// Test that the +sve-b16b16 is not considered an SVE flag (it applies to both)
__attribute__((target("+sme2,+sve2,+sve-b16b16")))
svbfloat16_t good7(svbfloat16_t a, svbfloat16_t b, svbfloat16_t c) __arm_streaming {
return svclamp_bf16(a, b, c);
}

// Without '+sme2', the builtin is only valid in non-streaming mode.
__attribute__((target("+sve2p1,+sme")))
svfloat32_t bad1(svfloat32_t a, svfloat32_t b, svfloat32_t c) __arm_streaming {
Expand Down
76 changes: 74 additions & 2 deletions clang/utils/TableGen/SveEmitter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1770,6 +1770,58 @@ void SVEEmitter::createBuiltinZAState(raw_ostream &OS) {
OS << "#endif\n\n";
}

static StringRef parseGuardParenExpr(StringRef &S) {
unsigned N = 0;
assert(S[0] == '(' && "Expected lparen");
for (unsigned I = 0; I < S.size(); ++I) {
if (S[I] == '(')
++N;
else if (S[I] == ')')
--N;
if (N == 0) {
StringRef Expr = S.substr(1, I - 1);
S = S.drop_front(I + 1);
return Expr;
}
}
llvm_unreachable("Unmatched parenthesi");
}

static StringRef parseGuardFeature(StringRef &S) {
assert(std::isalpha(S[0]) && "expected feature name");
unsigned I;
for (I = 0; I < S.size(); ++I) {
if (S[I] == ',' || S[I] == '|' || S[I] == ')')
break;
}
StringRef Expr = S.take_front(I);
S = S.drop_front(I);
return Expr;
}

static StringRef parseGuardExpr(StringRef &S) {
if (S[0] == '(')
return parseGuardParenExpr(S);
if (std::isalpha(S[0]))
return parseGuardFeature(S);
llvm_unreachable("Unexpected token in expression");
}

// Parse the TargetGuard and verify that it satisfies at least one of the
// features from the Required list.
static bool verifyGuard(StringRef S, ArrayRef<StringRef> Required) {
if (S.empty())
return false;
StringRef LHS = parseGuardExpr(S);
if (S.empty())
return llvm::any_of(Required, [LHS](StringRef R) { return R == LHS; });
if (S[0] == '|')
return verifyGuard(LHS, Required) && verifyGuard(S.drop_front(1), Required);
if (S[0] == ',')
return verifyGuard(LHS, Required) || verifyGuard(S.drop_front(1), Required);
llvm_unreachable("Unexpected token in expression");
}

void SVEEmitter::createStreamingAttrs(raw_ostream &OS, ACLEKind Kind) {
std::vector<const Record *> RV = Records.getAllDerivedDefinitions("Inst");
SmallVector<std::unique_ptr<Intrinsic>, 128> Defs;
Expand Down Expand Up @@ -1802,9 +1854,29 @@ void SVEEmitter::createStreamingAttrs(raw_ostream &OS, ACLEKind Kind) {

if (Def->isFlagSet(IsStreamingFlag))
StreamingMap["ArmStreaming"].insert(Def->getMangledName());
else if (Def->isFlagSet(VerifyRuntimeMode))
else if (Def->isFlagSet(VerifyRuntimeMode)) {
// Verify that the target guards contain at least one feature that
// actually enables SVE or SME (explicitly, or implicitly). This is needed
// for the code in SemaARM.cpp (checkArmStreamingBuiltin) that checks
// whether the required runtime mode for an intrinsic matches with the
// given set of target features and function attributes.
//
// The feature lists below must match the disabled features in
// 'checkArmStreamingBuiltin'!
if (!Def->getSVEGuard().empty() &&
!verifyGuard(Def->getSVEGuard(),
{"sve", "sve2", "sve2p1", "sve2-aes", "sve2-sha3",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should need to have "f32mm" and "f64mm" here (and the CallerFeatureMapWithoutSVE part). This works at the moment as the only intrinsics that have these in their target-guard also redundantly include sve and are invalid in streaming mode.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing those out!

It's worth saying that we could also implement things differently and require all SVE target guards to have "sve/sve2/sve2p1" as a base, such that let SVETargetGuard = "sve2-aes" becomes let SVETargetGuard = "sve2,sve2-aes". That means we need to refactor some of the target guards in the .td files, but it means we don't have to continually add all features that imply sve/sve2/sve2p1 to this list (and the list in SemaARM.cpp). For end-users I think it doesn't matter, the only difference is in the diagnostic which prints the required features for the intrinsic. Perhaps that is the better way forward.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I very much prefer this idea.

Copy link
Contributor

@SpencerAbson SpencerAbson Sep 24, 2024

Choose a reason for hiding this comment

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

I agree, I think we would definitely benefit in the long-run if we do not have to modify this code when features are added or changed.

(For future work) is it completely crazy to suggest resolving dependency chains back to SVE or SME (or both/neither) using ExtensionDependencies, which is constructed based on the feature definitions in AArch64Features.td?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose you might not need to change arm_sve.td because you could have SVETargetGuard(x) imply sve,(x)? Likewise for SME.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I believe so.

"sve2-sm4", "sve2-bitperm"}))
llvm_unreachable(
"SVE guard must include at least one base SVE version");
if (!Def->getSMEGuard().empty() &&
!verifyGuard(Def->getSMEGuard(),
{"sme", "sme2", "sme2p1", "sme-f64f64", "sme-i16i64",
"sme-b16b16", "sme-f16f16", "sme-f8f32", "sme-f8f16"}))
llvm_unreachable(
"SME guard must include at least one base SME version");
StreamingMap["VerifyRuntimeMode"].insert(Def->getMangledName());
else if (Def->isFlagSet(IsStreamingCompatibleFlag))
} else if (Def->isFlagSet(IsStreamingCompatibleFlag))
StreamingMap["ArmStreamingCompatible"].insert(Def->getMangledName());
else
StreamingMap["ArmNonStreaming"].insert(Def->getMangledName());
Expand Down