Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SME2 builtins for pfalse and ptrue #71953

Merged
merged 4 commits into from
Dec 6, 2023

Conversation

MDevereau
Copy link
Contributor

@MDevereau MDevereau commented Nov 10, 2023

Extend pfalse and ptrue builtins with svcount_t return types to be enabled for sve2p1 and sme2

See ARM-software/acle#217

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 10, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 10, 2023

@llvm/pr-subscribers-clang

Author: Matthew Devereau (MDevereau)

Changes

Extend pfalse and ptrue builtins with svcount_t return types to be enabled for sve2p1 and sme2


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

3 Files Affected:

  • (modified) clang/include/clang/Basic/arm_sve.td (+5-2)
  • (modified) clang/lib/Sema/Sema.cpp (+2-1)
  • (added) clang/test/CodeGen/aarch64-sme2-intrinsics/acle_sme2_ptrue_pfalse_attr.c (+34)
diff --git a/clang/include/clang/Basic/arm_sve.td b/clang/include/clang/Basic/arm_sve.td
index 3d4c2129565903d..eccf87ac94abb3f 100644
--- a/clang/include/clang/Basic/arm_sve.td
+++ b/clang/include/clang/Basic/arm_sve.td
@@ -1861,8 +1861,6 @@ def SVBGRP_N : SInst<"svbgrp[_n_{d}]", "dda", "UcUsUiUl", MergeNone, "aarch64_sv
 
 let TargetGuard = "sve2p1" in {
 def SVFCLAMP   : SInst<"svclamp[_{d}]", "dddd", "hfd", MergeNone, "aarch64_sve_fclamp", [], []>;
-def SVPTRUE_COUNT  : SInst<"svptrue_{d}", "}v", "QcQsQiQl", MergeNone, "aarch64_sve_ptrue_{d}", [IsOverloadNone], []>;
-def SVPFALSE_COUNT_ALIAS : SInst<"svpfalse_c", "}v", "", MergeNone, "", [IsOverloadNone]>;
 
 def SVPEXT_SINGLE : SInst<"svpext_lane_{d}", "P}i", "QcQsQiQl", MergeNone, "aarch64_sve_pext", [], [ImmCheck<1, ImmCheck0_3>]>;
 def SVPEXT_X2     : SInst<"svpext_lane_{d}_x2", "2.P}i", "QcQsQiQl", MergeNone, "aarch64_sve_pext_x2", [], [ImmCheck<1, ImmCheck0_1>]>;
@@ -1981,6 +1979,11 @@ def SVCNTP_COUNT : SInst<"svcntp_{d}", "n}i", "QcQsQiQl", MergeNone, "aarch64_sv
 defm SVREVD : SInstZPZ<"svrevd", "csilUcUsUiUl", "aarch64_sve_revd">;
 }
 
+let TargetGuard = "sve2p1|sme2" in {
+  def SVPTRUE_COUNT  : SInst<"svptrue_{d}", "}v", "QcQsQiQl", MergeNone, "aarch64_sve_ptrue_{d}", [IsOverloadNone], []>;
+  def SVPFALSE_COUNT_ALIAS : SInst<"svpfalse_c", "}v", "", MergeNone, "", [IsOverloadNone]>;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // SME2
 
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index d7d8d2eaa37e1d6..e4f6a291a869c27 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -2084,7 +2084,8 @@ void Sema::checkTypeSupport(QualType Ty, SourceLocation Loc, ValueDecl *D) {
       llvm::StringMap<bool> CallerFeatureMap;
       Context.getFunctionFeatureMap(CallerFeatureMap, FD);
       if (!Builtin::evaluateRequiredTargetFeatures(
-          "sve", CallerFeatureMap))
+          "sve", CallerFeatureMap) && !Builtin::evaluateRequiredTargetFeatures(
+          "sme", CallerFeatureMap))
         Diag(D->getLocation(), diag::err_sve_vector_in_non_sve_target) << Ty;
     }
   };
diff --git a/clang/test/CodeGen/aarch64-sme2-intrinsics/acle_sme2_ptrue_pfalse_attr.c b/clang/test/CodeGen/aarch64-sme2-intrinsics/acle_sme2_ptrue_pfalse_attr.c
new file mode 100644
index 000000000000000..90d9434d87cf4a0
--- /dev/null
+++ b/clang/test/CodeGen/aarch64-sme2-intrinsics/acle_sme2_ptrue_pfalse_attr.c
@@ -0,0 +1,34 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// REQUIRES: aarch64-registered-target
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme2 -S -disable-O0-optnone -Werror -Wall -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme2 -S -disable-O0-optnone -Werror -Wall -emit-llvm -o - -x c++ %s | FileCheck %s -check-prefix=CPP-CHECK
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme2 -S -disable-O0-optnone -Werror -Wall -o /dev/null %s
+#include <arm_sve.h>
+
+// CHECK-LABEL: @test_svptrue_c8_attr(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[TMP0:%.*]] = call target("aarch64.svcount") @llvm.aarch64.sve.ptrue.c8()
+// CHECK-NEXT:    ret target("aarch64.svcount") [[TMP0]]
+//
+// CPP-CHECK-LABEL: @_Z20test_svptrue_c8_attrv(
+// CPP-CHECK-NEXT:  entry:
+// CPP-CHECK-NEXT:    [[TMP0:%.*]] = call target("aarch64.svcount") @llvm.aarch64.sve.ptrue.c8()
+// CPP-CHECK-NEXT:    ret target("aarch64.svcount") [[TMP0]]
+//
+svcount_t test_svptrue_c8_attr(void) __arm_streaming {
+  return svptrue_c8();
+}
+
+// CHECK-LABEL: @test_svptrue_c(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[TMP0:%.*]] = call target("aarch64.svcount") @llvm.aarch64.sve.convert.from.svbool.taarch64.svcountt(<vscale x 16 x i1> zeroinitializer)
+// CHECK-NEXT:    ret target("aarch64.svcount") [[TMP0]]
+//
+// CPP-CHECK-LABEL: @_Z14test_svptrue_cv(
+// CPP-CHECK-NEXT:  entry:
+// CPP-CHECK-NEXT:    [[TMP0:%.*]] = call target("aarch64.svcount") @llvm.aarch64.sve.convert.from.svbool.taarch64.svcountt(<vscale x 16 x i1> zeroinitializer)
+// CPP-CHECK-NEXT:    ret target("aarch64.svcount") [[TMP0]]
+//
+svcount_t test_svptrue_c(void) __arm_streaming {
+  return svpfalse_c();
+}

Copy link

github-actions bot commented Nov 10, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@@ -1981,6 +1979,11 @@ def SVCNTP_COUNT : SInst<"svcntp_{d}", "n}i", "QcQsQiQl", MergeNone, "aarch64_sv
defm SVREVD : SInstZPZ<"svrevd", "csilUcUsUiUl", "aarch64_sve_revd">;
}

let TargetGuard = "sve2p1|sme2" in {
def SVPTRUE_COUNT : SInst<"svptrue_{d}", "}v", "QcQsQiQl", MergeNone, "aarch64_sve_ptrue_{d}", [IsOverloadNone], []>;
def SVPFALSE_COUNT_ALIAS : SInst<"svpfalse_c", "}v", "", MergeNone, "", [IsOverloadNone]>;
Copy link
Contributor

Choose a reason for hiding this comment

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

The streaming mode attributes can now be added to these, I believe they are IsStreaming for ptrue and IsStreamingCompatible for pfalse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// CPP-CHECK-NEXT: [[TMP0:%.*]] = call target("aarch64.svcount") @llvm.aarch64.sve.ptrue.c8()
// CPP-CHECK-NEXT: ret target("aarch64.svcount") [[TMP0]]
//
svcount_t test_svptrue_c8_attr(void) __arm_streaming {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are already tests for these builtins in acle_sve2p1_ptrue.c & acle_sve2p1_pfalse.c, so I think you can just add more RUN lines to these for SME2.

Copy link
Contributor Author

@MDevereau MDevereau Nov 15, 2023

Choose a reason for hiding this comment

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

As discussed we might be missing an HasSVE2p1OrSME2 target guard which is currently stopping me from doing this in this patch due to the __arm_streaming and __arm_streaming_compatible function attributes which I've added to the tests

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you make the intrinsic IsStreamingCompatible, you should be able to add the RUN lines I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -1981,6 +1979,11 @@ def SVCNTP_COUNT : SInst<"svcntp_{d}", "n}i", "QcQsQiQl", MergeNone, "aarch64_sv
defm SVREVD : SInstZPZ<"svrevd", "csilUcUsUiUl", "aarch64_sve_revd">;
}

let TargetGuard = "sve2p1|sme2" in {
def SVPTRUE_COUNT : SInst<"svptrue_{d}", "}v", "QcQsQiQl", MergeNone, "aarch64_sve_ptrue_{d}", [IsOverloadNone, IsStreaming], []>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def SVPTRUE_COUNT : SInst<"svptrue_{d}", "}v", "QcQsQiQl", MergeNone, "aarch64_sve_ptrue_{d}", [IsOverloadNone, IsStreaming], []>;
def SVPTRUE_COUNT : SInst<"svptrue_{d}", "}v", "QcQsQiQl", MergeNone, "aarch64_sve_ptrue_{d}", [IsOverloadNone, IsStreamingCompatible], []>;

As pointed out here, this will need an attribute like IsStreamingOrHasSVE2p1, for which the compiler will give a diagnostic when compiling for +sme2 and the function is not in streaming mode.

To move this patch forward without that diagnostic, you could make this IsStreamingCompatible for now. Can you also add a FIXME to say we need to change this later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// CPP-CHECK-NEXT: [[TMP0:%.*]] = call target("aarch64.svcount") @llvm.aarch64.sve.ptrue.c8()
// CPP-CHECK-NEXT: ret target("aarch64.svcount") [[TMP0]]
//
svcount_t test_svptrue_c8_attr(void) __arm_streaming {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you make the intrinsic IsStreamingCompatible, you should be able to add the RUN lines I think?

Extend pfalse and ptrue builtins with svcount_t return types
to be enabled for sve2p1 and sme2
@@ -15,7 +17,7 @@
// CPP-CHECK-NEXT: [[TMP0:%.*]] = tail call target("aarch64.svcount") @llvm.aarch64.sve.ptrue.c8()
// CPP-CHECK-NEXT: ret target("aarch64.svcount") [[TMP0]]
//
svcount_t test_svptrue_c8(void) {
svcount_t test_svptrue_c8(void) __arm_streaming_compatible {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you rewrite the tests in such a way that we do test for the two different configurations:

  • For SVE2p1: no attribute
  • For SME2: __arm_streaming

You can do something with macros, so that you replace __arm_streaming_compatible here with ATTR and then do something like:

#ifndef TEST_SME
#define ATTR
#else
#define ATTR __arm_streaming
#endif

and then for the +sme2 RUN line you'd add -DTEST_SME.

That way, we're sure to test for the correct mode when we update the builtin to use a different attribute than __arm_streaming_compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've called the #if guard TEST_SME2, but done

Copy link
Collaborator

@sdesmalen-arm sdesmalen-arm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@MDevereau MDevereau merged commit 2f62037 into llvm:main Dec 6, 2023
4 checks passed
@MDevereau MDevereau deleted the builtins-ptrue-pfalse branch December 6, 2023 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants