[AArch64][compiler-rt] Add a function returning the current vector length by kmclaughlin-arm · Pull Request #92921 · llvm/llvm-project

kmclaughlin-arm · 2024-05-21T14:27:25Z

get_runtime_vl emits a cntd instruction if SVE is available at runtime,
otherwise it will return 0.

…ngth get_runtime_vl emits a cntd instruction if SVE is available at runtime, otherwise it will return 0.

github-actions · 2024-05-21T14:48:39Z

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

efriedma-quic

This function should be in the ABI document before we merge it, I think.

efriedma-quic · 2024-05-21T17:44:28Z

compiler-rt/lib/builtins/aarch64/sme-abi-init.c

+    __init_cpu_features();
+}
+
+__attribute__((target("sve"))) long emit_cntd(void) {


efriedma-quic · 2024-05-21T17:44:53Z

compiler-rt/lib/builtins/aarch64/sme-abi-init.c

+  return vl;
+}
+
+long get_runtime_vl(void) {


This probably should be prefixed with __aarch64

Hi @efriedma-quic, I've added this routine to the ABI document in ARM-software/abi-aa#263 and renamed it to __arm_get_current_vg.

efriedma-quic · 2024-05-21T17:48:22Z

compiler-rt/lib/builtins/aarch64/sme-abi-init.c

 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

+#include "../cpu_model/aarch64.c"


cpu_model/aarch64.c isn't a header; including it seem wrong.

ARM-software/abi-aa#263 - Renamed get_runtime_vl to __arm_get_current_vg - Also return VG if currently in streaming-mode - Added static to get_aarch64_cpu_features - Added aarch64.h and included in sme-abi-init.c

…me_state, to check if currently in streaming mode.

efriedma-quic · 2024-05-30T21:16:22Z

compiler-rt/lib/builtins/cpu_model/aarch64.h

+  FEAT_INIT      // Used as flag of features initialization completion
+};
+
+long long get_features(void);


This exports a function named get_features from compiler-rt... probably you want to hide it somehow (__ prefix, or make it static).

efriedma-quic · 2024-05-31T19:09:32Z

compiler-rt/lib/builtins/cpu_model/aarch64.h

+
+long long __get_aarch64_features(void);
+
+void CONSTRUCTOR_ATTRIBUTE __init_cpu_features(void);


You shouldn't need CONSTRUCTOR_ATTRIBUTE on the declaration here, I think.

efriedma-quic · 2024-05-31T19:13:14Z

compiler-rt/lib/builtins/aarch64/sme-abi-init.c

+extern struct SME_STATE __arm_sme_state(void) __arm_streaming_compatible;
+
+__attribute__((target("sve"))) long
+__arm_get_current_vg(void) __arm_streaming_compatible {


Maybe split this into a separate file, so code using the other stuff defined in this file doesn't pull in the constructor? I assume __arm_get_current_vg won't be used in most configurations.

I've split this into a separate file and replaced the use of has_sme() with __aarch64_has_sme_and_tpidr2_el0.

efriedma-quic · 2024-05-31T19:26:15Z

compiler-rt/lib/builtins/cpu_model/aarch64.h

+  FEAT_INIT      // Used as flag of features initialization completion
+};
+
+long long __get_aarch64_features(void);


Looking again... maybe we should just poke at __aarch64_cpu_features directly, instead of adding this function? __aarch64_cpu_features is already part of the ABI (clang codegen knows about it).

Thanks, I've removed __get_aarch64_features. I'd missed that clang was also using __aarch64_cpu_features.

- Removed __get_aarch64_features. - Split out changes for __arm_get_current_vg into a new file (sme-abi-vg.c) and removed from sme-abi-init.c.

efriedma-quic

LGTM

yozhu · 2024-06-07T23:46:05Z

This change results in compiler-rt build failure:

compiler-rt/lib/builtins/aarch64/sme-abi-vg.c:27:5: error: call to undeclared function '__init_cpu_features'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    27 |     __init_cpu_features();
       |     ^
compiler-rt/lib/builtins/aarch64/sme-abi-vg.c:33:60: error: use of undeclared identifier 'FEAT_SVE'
    33 |   bool HasSVE = __aarch64_cpu_features.features & (1ULL << FEAT_SVE);

The new C file, sme-abi-vg.c, doesn't have #if !defined(DISABLE_AARCH64_FMV).

…ioning is not supported (#94973) This prevents build failures when building with `DISABLE_AARCH64_FMV`, see #92921.

When llvm#92921 added the `__arm_get_current_vg` functionality, it used the FMV feature bits mechanism rather than the existing mechanism that was previously added for SME that called `getauxval` (on Linux platforms) or `__aarch64_sme_accessible` (required for baremetal libraries). It seems simpler to always use the FMV feature bits mechanism, but for baremetal targets we still need to rely on `__arm_sme_accessible`.

…features. (#119414) When #92921 added the `__arm_get_current_vg` functionality, it used the FMV feature bits mechanism rather than the mechanism that was previously added for SME which called `getauxval` on Linux platforms or `__aarch64_sme_accessible` required for baremetal libraries. It is better to always use `__aarch64_cpu_features`. For baremetal we still need to rely on `__arm_sme_accessible` to initialise the struct.

[AArch64][compiler-rt] Add a function returning the current vector le…

1038d54

…ngth get_runtime_vl emits a cntd instruction if SVE is available at runtime, otherwise it will return 0.

kmclaughlin-arm requested review from aemerson, efriedma-quic, rsandifo-arm and sdesmalen-arm May 21, 2024 14:27

llvmbot added compiler-rt compiler-rt:builtins labels May 21, 2024

- Run clang-format

2baec75

efriedma-quic reviewed May 21, 2024

View reviewed changes

kmclaughlin-arm added 2 commits May 30, 2024 14:51

- Changes based on support routine added to the ABI document, see:

75c2ae9

ARM-software/abi-aa#263 - Renamed get_runtime_vl to __arm_get_current_vg - Also return VG if currently in streaming-mode - Added static to get_aarch64_cpu_features - Added aarch64.h and included in sme-abi-init.c

- Replaced use of __arm_in_streaming_mode from arm_sme.h with __arm_s…

455bb3a

…me_state, to check if currently in streaming mode.

efriedma-quic reviewed May 30, 2024

View reviewed changes

- Renamed get_features to __get_aarch64_features in aarch64.h

5edc405

efriedma-quic requested a review from labrinea May 31, 2024 19:24

efriedma-quic reviewed May 31, 2024

View reviewed changes

- Removed CONSTRUCTOR_ATTRIBUTE from __init_cpu_features declaration.

6a0a1fc

- Removed __get_aarch64_features. - Split out changes for __arm_get_current_vg into a new file (sme-abi-vg.c) and removed from sme-abi-init.c.

kmclaughlin-arm mentioned this pull request Jun 3, 2024

[AArch64][SME] Save VG for unwind info when changing streaming-mode #83301

Merged

efriedma-quic approved these changes Jun 3, 2024

View reviewed changes

kmclaughlin-arm merged commit 043cc5a into llvm:main Jun 5, 2024

kmclaughlin-arm mentioned this pull request Jun 10, 2024

[AArch64][compiler-rt] Disable SME ABI routines if function multiversioning is not supported #94973

Merged

kmclaughlin-arm added a commit that referenced this pull request Jun 12, 2024

[AArch64][compiler-rt] Disable SME ABI routines if function multivers…

0387254

…ioning is not supported (#94973) This prevents build failures when building with `DISABLE_AARCH64_FMV`, see #92921.

kmclaughlin-arm deleted the get-rt-vl branch June 14, 2024 12:55

sdesmalen-arm mentioned this pull request Dec 10, 2024

[compiler-rt][AArch64] Rewrite SME routines to all use __aarch64_cpu_features. #119414

Merged


		long long __get_aarch64_features(void);

		void CONSTRUCTOR_ATTRIBUTE __init_cpu_features(void);

Conversation

kmclaughlin-arm commented May 21, 2024

Uh oh!

github-actions bot commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efriedma-quic 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!

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!

Choose a reason for hiding this comment

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

yozhu commented Jun 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented May 21, 2024 •

edited

Loading