[AArch64][SME] Add support for sme-fa64 #70809

MDevereau · 2023-10-31T14:07:49Z

No description provided.

SamTebbs33

LGTM

sdesmalen-arm · 2023-10-31T15:51:21Z

llvm/include/llvm/TargetParser/AArch64TargetParser.h

-    {"sme-i16i64", AArch64::AEK_SMEI16I64, "+sme-i16i64", "-sme-i16i64", FEAT_SME_I64, "+sme,+sme-i16i64,+bf16", 570},
+    {"sme-f64f64", AArch64::AEK_SMEF64F64, "+sme-f64f64", "-sme-f64f64", FEAT_SME_F64,  "+sme,+sme-f64f64,+bf16", 560},
+    {"sme-i16i64", AArch64::AEK_SMEI16I64, "+sme-i16i64", "-sme-i16i64", FEAT_SME_I64,  "+sme,+sme-i16i64,+bf16", 570},
+    {"sme-fa64",   AArch64::AEK_SMEFA64,   "+sme-fa64",   "-sme-fa64",   FEAT_SME_FA64, "+sve2",                  580},


What does the last number in the initializer list mean?

Apparently its the Function Multi Versioning priority? I'm not sure how it works but it seems to be tested by attr-target-version.c which does not appear to be testing the other SME flags such as F16F16

I wonder if this has something to do with the order in which it emits the runtime checks for these target features in the resolver function. I can't really gauge whether this priority is better or worse than any other though. Perhaps @ilinpv can give some guidance here?

sme-f16f16 is not supported by Function Multi Versioning. All features supported by FMV with priorities should be specified in ACLE: https://github.com/ARM-software/acle/blob/main/main/acle.md#function-multi-versioning
I am not sure if we need support for sme-fa64 in FMV, probably @DanielKristofKiss can clarify.

llvm/test/MC/AArch64/SVE2/adclb.s

llvm/lib/Target/AArch64/AArch64Subtarget.cpp

llvm/lib/Target/AArch64/AArch64.td

compiler-rt/lib/builtins/cpu_model.c

github-actions · 2023-11-01T16:19:21Z

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

You can test this locally with the following command:

git-clang-format --diff c350a1eaae2be108e44f93c2ccb331ec47872e56 0b4981dc3393997ba906275176960b1cdfd598d9 -- clang/lib/Basic/Targets/AArch64.cpp clang/lib/Basic/Targets/AArch64.h llvm/include/llvm/TargetParser/AArch64TargetParser.h llvm/lib/Target/AArch64/AArch64Subtarget.cpp llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp llvm/unittests/TargetParser/TargetParserTest.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp b/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
index ced1c7ea98..ac8492f393 100644
--- a/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
+++ b/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
@@ -3662,7 +3662,7 @@ static const struct Extension {
     {"sme-lutv2", {AArch64::FeatureSME_LUTv2}},
     {"sme-f8f16", {AArch64::FeatureSMEF8F16}},
     {"sme-f8f32", {AArch64::FeatureSMEF8F32}},
-    {"sme-fa64",  {AArch64::FeatureSMEFA64}},
+    {"sme-fa64", {AArch64::FeatureSMEFA64}},
 };
 
 static void setRequiredFeatureString(FeatureBitset FBS, std::string &Str) {

compiler-rt/lib/builtins/cpu_model.c

llvm/lib/Target/AArch64/AArch64Subtarget.cpp

dtemirbulatov

LGTM.

llvm/include/llvm/TargetParser/AArch64TargetParser.h

llvm/lib/Target/AArch64/AArch64.td

sdesmalen-arm · 2023-11-05T14:05:25Z

llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-fp-reduce-fa64.ll

+
+target triple = "aarch64-unknown-linux-gnu"
+
+define half @fadda_v4f16(half %start, <4 x half> %a) {


Could you add a similar test where a Neon instruction would be used when sme-fa64 is set? (and where it would be scalarised otherwise)

Added test llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-int-mla-neon-fa64.ll

sdesmalen-arm · 2023-11-05T14:07:46Z

llvm/include/llvm/TargetParser/AArch64TargetParser.h

-    {"sme-i16i64", AArch64::AEK_SMEI16I64, "+sme-i16i64", "-sme-i16i64", FEAT_SME_I64, "+sme,+sme-i16i64,+bf16", 570},
+    {"sme-f64f64", AArch64::AEK_SMEF64F64, "+sme-f64f64", "-sme-f64f64", FEAT_SME_F64,  "+sme,+sme-f64f64,+bf16", 560},
+    {"sme-i16i64", AArch64::AEK_SMEI16I64, "+sme-i16i64", "-sme-i16i64", FEAT_SME_I64,  "+sme,+sme-i16i64,+bf16", 570},
+    {"sme-fa64",   AArch64::AEK_SMEFA64,   "+sme-fa64",   "-sme-fa64",   FEAT_SME_FA64, "+sve2",                  580},


I wonder if this has something to do with the order in which it emits the runtime checks for these target features in the resolver function. I can't really gauge whether this priority is better or worse than any other though. Perhaps @ilinpv can give some guidance here?

sdesmalen-arm

LGTM (with nit addressed), but please check with @ilinpv on the FMV priorities before merging the patch.

sdesmalen-arm · 2023-11-06T16:04:54Z

llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-int-mla-neon-fa64.ll

+; NO-FA64-NEXT:    mad z0.b, p0/m, z1.b, z2.b
+; NO-FA64-NEXT:    // kill: def $d0 killed $d0 killed $z0
+; NO-FA64-NEXT:    ret
+	%tmp1 = mul <8 x i8> %A, %B;


nit: odd indentation?

sdesmalen-arm

LGTM with nit addressed

sdesmalen-arm · 2023-11-15T15:37:43Z

llvm/lib/Target/AArch64/AArch64.td

@@ -508,6 +508,9 @@ def FeatureSMEI16I64 : SubtargetFeature<"sme-i16i64", "HasSMEI16I64", "true",
 def FeatureSMEF16F16 : SubtargetFeature<"sme-f16f16", "HasSMEF16F16", "true",
  "Enable SME2.1 non-widening Float16 instructions (FEAT_SME_F16F16)", []>;

+def FeatureSMEFA64 : SubtargetFeature<"sme-fa64", "HasSMEFA64", "true",
+  "Enable the full A64 instruction set in streaming SVE mode (FEAT_SME_FA64)", [FeatureSME, FeatureSVE2, FeatureNEON]>;


Suggested change

"Enable the full A64 instruction set in streaming SVE mode (FEAT_SME_FA64)", [FeatureSME, FeatureSVE2, FeatureNEON]>;

"Enable the full A64 instruction set in streaming SVE mode (FEAT_SME_FA64)", [FeatureSME, FeatureSVE2]>;

nit: NEON is already implied by FeatureSVE (which is implied by FeatureSVE2)

compiler-rt/lib/builtins/cpu_model.c

ilinpv · 2023-11-15T15:54:37Z

llvm/include/llvm/TargetParser/AArch64TargetParser.h

@@ -293,6 +295,7 @@ inline constexpr ExtensionInfo Extensions[] = {
    {"sme-lutv2", AArch64::AEK_SME_LUTv2, "+sme-lutv2", "-sme-lutv2", FEAT_INIT, "", 0},
    {"sme-f8f16", AArch64::AEK_SMEF8F16, "+sme-f8f16", "-sme-f8f16", FEAT_INIT, "+sme2,+fp8", 0},
    {"sme-f8f32", AArch64::AEK_SMEF8F32, "+sme-f8f32", "-sme-f8f32", FEAT_INIT, "+sme2,+fp8", 0},
+    {"sme-fa64",  AArch64::AEK_SMEFA64,  "+sme-fa64",  "-sme-fa64",  FEAT_INIT, "+sme,+sve2,+simd", 0},


No need for "+sme,+sve2,+simd" mapping if feature is not in FMV list.

ilinpv

LGTM overall, thanks, just address one nit above.

ilinpv · 2023-11-16T10:55:47Z

llvm/include/llvm/TargetParser/AArch64TargetParser.h

@@ -293,6 +294,7 @@ inline constexpr ExtensionInfo Extensions[] = {
    {"sme-lutv2", AArch64::AEK_SME_LUTv2, "+sme-lutv2", "-sme-lutv2", FEAT_INIT, "", 0},
    {"sme-f8f16", AArch64::AEK_SMEF8F16, "+sme-f8f16", "-sme-f8f16", FEAT_INIT, "+sme2,+fp8", 0},
    {"sme-f8f32", AArch64::AEK_SMEF8F32, "+sme-f8f32", "-sme-f8f32", FEAT_INIT, "+sme2,+fp8", 0},
+    {"sme-fa64",  AArch64::AEK_SMEFA64,  "+sme-fa64",  "-sme-fa64",  FEAT_INIT, "", 0},


Small nit: remove extra space before FEAT_INIT here

MDevereau requested review from SamTebbs33, dtemirbulatov, sdesmalen-arm and kmclaughlin-arm October 31, 2023 14:07

llvmbot added clang Clang issues not falling into any other category compiler-rt backend:AArch64 mc Machine (object) code labels Oct 31, 2023

SamTebbs33 approved these changes Oct 31, 2023

View reviewed changes

sdesmalen-arm requested changes Oct 31, 2023

View reviewed changes

kmclaughlin-arm reviewed Oct 31, 2023

View reviewed changes

compiler-rt/lib/builtins/cpu_model.c Outdated Show resolved Hide resolved

MDevereau force-pushed the FA64 branch from d080e84 to 0534097 Compare November 1, 2023 16:05

sdesmalen-arm reviewed Nov 1, 2023

View reviewed changes

compiler-rt/lib/builtins/cpu_model.c Outdated Show resolved Hide resolved

llvm/lib/Target/AArch64/AArch64Subtarget.cpp Outdated Show resolved Hide resolved

llvm/lib/Target/AArch64/AArch64Subtarget.cpp Outdated Show resolved Hide resolved

llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Nov 2, 2023

dtemirbulatov approved these changes Nov 3, 2023

View reviewed changes

sdesmalen-arm reviewed Nov 5, 2023

View reviewed changes

sdesmalen-arm approved these changes Nov 6, 2023

View reviewed changes

MDevereau added 7 commits November 15, 2023 14:06

[AArch64][SME] Add support for sme-fa64

106913d

[AArch64][SME] Add support for sme-fa64

470db8a

Remove whitespace from .s tests

64ff862

Update cpu_model.c and clang AArch64 targets

114dda0

Add isNeonAvailable test

fb60cf1

Fix whitespace

9831991

imply sve2, reorder enums

b637940

MDevereau force-pushed the FA64 branch from eb6e297 to b637940 Compare November 15, 2023 15:31

sdesmalen-arm approved these changes Nov 15, 2023

View reviewed changes

ilinpv reviewed Nov 15, 2023

View reviewed changes

MDevereau added 2 commits November 15, 2023 17:11

Remove NEON implication and FMV dependencies

9f004d1

Remove remaining cpu_target FA64 enums

75de0cf

ilinpv approved these changes Nov 16, 2023

View reviewed changes

Remove whitespace character

0b4981d

MDevereau merged commit cdf6693 into llvm:main Nov 20, 2023

sr-tream pushed a commit to sr-tream/llvm-project that referenced this pull request Nov 20, 2023

[AArch64][SME] Add support for sme-fa64 (llvm#70809)

cc5648e

zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023

[AArch64][SME] Add support for sme-fa64 (llvm#70809)

9887fc5

MDevereau deleted the FA64 branch April 30, 2024 13:45


		target triple = "aarch64-unknown-linux-gnu"

		define half @fadda_v4f16(half %start, <4 x half> %a) {

	"Enable the full A64 instruction set in streaming SVE mode (FEAT_SME_FA64)", [FeatureSME, FeatureSVE2, FeatureNEON]>;
	"Enable the full A64 instruction set in streaming SVE mode (FEAT_SME_FA64)", [FeatureSME, FeatureSVE2]>;

[AArch64][SME] Add support for sme-fa64 #70809

[AArch64][SME] Add support for sme-fa64 #70809

Uh oh!

Conversation

MDevereau commented Oct 31, 2023

Uh oh!

SamTebbs33 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dtemirbulatov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdesmalen-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdesmalen-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilinpv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Nov 1, 2023 •

edited

Loading