[Clang][AArch64] Fix checkArmStreamingBuiltin for 'sve-b16b16'

[sdesmalen-arm]

The implementation made the assumption that any feature starting with "sve" meant that this was an SVE feature. This is not the case for "sve-b16b16", as this is a feature that applies to both SVE and SME.

This meant that:

  __attribute__((target("+sme2,+sve2,+sve-b16b16")))
  svbfloat16_t foo(svbfloat16_t a, svbfloat16_t b, svbfloat16_t c)
                                                      __arm_streaming {
      return svclamp_bf16(a, b, c);
  }

would result in an incorrect diagnostic saying that svclamp_bf16 could only be used in non-streaming functions.

[llvmbot]

@llvm/pr-subscribers-backend-arm
@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-clang

Author: Sander de Smalen (sdesmalen-arm)

Changes

The implementation made the assumption that any feature starting with "sve" meant that this was an SVE feature. This is not the case for "sve-b16b16", as this is a feature that applies to both SVE and SME.

This meant that:

  __attribute__((target("+sme2,+sve2,+sve-b16b16")))
  svbfloat16_t foo(svbfloat16_t a, svbfloat16_t b, svbfloat16_t c)
                                                      __arm_streaming {
      return svclamp_bf16(a, b, c);
  }

would result in an incorrect diagnostic saying that svclamp_bf16 could only be used in non-streaming functions.

---

Full diff: https://github.com/llvm/llvm-project/pull/109420.diff

2 Files Affected:

• (modified) clang/lib/Sema/SemaARM.cpp (+13-8)
• (modified) clang/test/Sema/aarch64-streaming-sme-or-nonstreaming-sve-builtins.c (+6)

diff --git a/clang/lib/Sema/SemaARM.cpp b/clang/lib/Sema/SemaARM.cpp
index efde354860de43..fba1453e5d38fc 100644
--- a/clang/lib/Sema/SemaARM.cpp
+++ b/clang/lib/Sema/SemaARM.cpp
@@ -567,15 +567,18 @@ 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");
+    CallerFeatureMapWithoutSVE["sve"] = false;
+    CallerFeatureMapWithoutSVE["sve2"] = false;
+    CallerFeatureMapWithoutSVE["sve2p1"] = false;
+    // FIXME: This list must be updated with future extensions, because when
+    // an intrinsic is enabled by (sve2p1|sme2p1), disabling just "sve" is
+    // not sufficient, as the feature dependences are not resolved.
+    // At the moment, it should be sufficient to test the 'base' architectural
+    // support for SVE and SME, which must always be provided in the
+    // target guard. e.g. TargetGuard = "sve-b16b16" without "sme" or "sve"
+    // is not sufficient.
 
     // Avoid emitting diagnostics for a function that can never compile.
     if (FnType == SemaARM::ArmStreaming && !CallerFeatureMapWithoutSVE["sme"])
@@ -583,7 +586,9 @@ static bool checkArmStreamingBuiltin(Sema &S, CallExpr *TheCall,
 
     llvm::StringMap<bool> CallerFeatureMapWithoutSME;
     S.Context.getFunctionFeatureMap(CallerFeatureMapWithoutSME, FD);
-    DisableFeatures(CallerFeatureMapWithoutSME, "sme");
+    CallerFeatureMapWithoutSME["sme"] = false;
+    CallerFeatureMapWithoutSME["sme2"] = false;
+    CallerFeatureMapWithoutSME["sme2p1"] = 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
diff --git a/clang/test/Sema/aarch64-streaming-sme-or-nonstreaming-sve-builtins.c b/clang/test/Sema/aarch64-streaming-sme-or-nonstreaming-sve-builtins.c
index 45776eb13e4fbc..792d79ee3e600d 100644
--- a/clang/test/Sema/aarch64-streaming-sme-or-nonstreaming-sve-builtins.c
+++ b/clang/test/Sema/aarch64-streaming-sme-or-nonstreaming-sve-builtins.c
@@ -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 {

[paulwalker-arm]

It would be better to add code (SVEEmitter?) to parse the target guards to ensure there is at least one of the required SVE and SME features referenced. This should link to this code as something to update if the intent really is to add a new "base" option.

[SpencerAbson]

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.

[sdesmalen-arm]

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.

[paulwalker-arm]

I very much prefer this idea.

[SpencerAbson]

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?

[paulwalker-arm]

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

[SpencerAbson]

Yes, I believe so.