Add a API to check SVE Length support on ARM CPU. #255

maajidkhann · 2024-08-06T11:07:50Z

This pull request introduces a new feature to the cpuinfo library that adds an API to return the maximum supported Scalable Vector Extension (SVE) vector length on the given ARM CPU. This enhancement will allow users to query and determine the maximum SVE vector lengths on a given ARM CPU, providing better insights and flexibility for optimizing applications that utilize SVE.

Key Features:
New API Function:

Introduces a single API function - cpuinfo_get_max_arm_sve_length() that returns the maximum SVE Vector Length supported on the given ARM CPU.

The function is designed to be easy to integrate with existing code in other projects like PyTorch (pytorch/pytorch#119571) and provides a straightforward interface for querying SVE VL.

Here's the sample output on SVE supported instance:

Query:

Output on SVE256 supported Hardware - Graviton3:

Signed-off-by: maajidkhann <[email protected]>

maajidkhann · 2024-08-06T11:10:03Z

@malfet @digantdesai Please review this PR. This will make it easy for people to use API's from CPUINFO into PyTorch to get SVE vector length info from the hardware.

malfet

Please fix formatting (see lint failure in CI)
Also, all links to the images seems to be broken
And last but not least, please explain why boolean has_sve128 are better than single get_max_sve_len API?

malfet · 2024-08-06T11:20:56Z

src/arm/linux/aarch64-isa.c

+    #define PR_SVE_GET_VL 51
+    #define PR_SVE_VL_LEN_MASK 0xffff


Don't you want to add something like #ifndef PR_SVE_GET_VL guard here? And also, please mention which system headers they should be defined in

malfet · 2024-08-06T11:23:23Z

tools/isa-info.c

+	printf("\tSVE 128-bit: %s\n", cpuinfo_support_arm_sve128() ? "Yes" : "No");
+	printf("\tSVE 256-bit: %s\n", cpuinfo_support_arm_sve256() ? "Yes" : "No");
+	printf("\tSVE 512-bit: %s\n", cpuinfo_support_arm_sve512() ? "Yes" : "No");


https://developer.arm.com/documentation/101458/2404/Compiler-options/-msve-vector-bits- seems to indicate, that SVE length can be up to 2048? If this is the case, wouldn't it be better to add cpuinfo_get_arm_sve_lengh() method rather than enum? Also, if it's always a power of two, wouldn't it be easier if it returned a bitshifts?

Agree. Done.
My intention in the first place was to introduce functions like how AVX has done for AVX2 and AVX512 which returns a bool and for SVE as only VL=128, 256 and 512 are relevant in ARM ecosystem, I added three different functions.

But it makes sense to just add a single function that returns maximum VL supported on that hardware and now the user can work out the logic required with the value returned.

malfet · 2024-08-06T11:25:01Z

include/cpuinfo.h

@@ -1670,6 +1670,7 @@ struct cpuinfo_arm_isa {
 	bool sve;
 	bool sve2;
 	bool i8mm;
+	int svelen;


Why signed type? Is -1 used to indicate unsupported?

As that was the initial thought, -1 = unsupported. Now I have changed it to unsigned int as we don't deal with negative values and svelen value always remains >=0 in our context.

malfet · 2024-08-06T11:30:02Z

src/arm/linux/aarch64-isa.c

+    #define PR_SVE_VL_LEN_MASK 0xffff
+    int ret = prctl(PR_SVE_GET_VL);
+    if (ret < 0) {
+        perror("prctl(PR_SVE_GET_VL) failed");


Suggested change

perror("prctl(PR_SVE_GET_VL) failed");

cpuinfo_log_error("prctl(PR_SVE_GET_VL) failed");

->Addresses review comments and adds a function that directly returns the max VL on ARM supported CPU's. ->Fixes Lint Issues. Signed-off-by: maajidkhann <[email protected]>

maajidkhann · 2024-08-06T18:27:32Z

Please fix formatting (see lint failure in CI) Also, all links to the images seems to be broken And last but not least, please explain why boolean has_sve128 are better than single get_max_sve_len API?

Addresses all the comments. Lint issues also have been now fixed.

malfet

Thank you for all the updates, looks good, but please consider addressing few minor remarks

malfet · 2024-08-07T08:18:01Z

include/cpuinfo.h

+static inline int cpuinfo_get_max_arm_sve_length(void) {
+#if CPUINFO_ARCH_ARM64
+	if ((cpuinfo_isa.sve || cpuinfo_isa.sve2) && (cpuinfo_isa.svelen > 0))
+		return cpuinfo_isa.svelen;


If I understood your previous diff correctly, svelen is returned in bytes, but API developers usually think about it in bitlength, therefore it must be multiplied by 8, shouldn't it?

Suggested change

return cpuinfo_isa.svelen;

return cpuinfo_isa.svelen * 8;

Yes its better to return bit length directly compared to bytes to avoid confusion to the end users.
Changes done.

malfet · 2024-08-07T08:21:00Z

include/cpuinfo.h

@@ -1670,6 +1670,7 @@ struct cpuinfo_arm_isa {
 	bool sve;
 	bool sve2;
 	bool i8mm;
+	unsigned int svelen;


Suggested change

unsigned int svelen;

uint32_t svelen;

malfet · 2024-08-07T08:23:34Z

include/cpuinfo.h

+static inline int cpuinfo_get_max_arm_sve_length(void) {
+#if CPUINFO_ARCH_ARM64
+	if ((cpuinfo_isa.sve || cpuinfo_isa.sve2) && (cpuinfo_isa.svelen > 0))
+		return cpuinfo_isa.svelen;
+	else
+		return -1; // Returns -1 on Non SVE supported ARM CPU's.
+#else
+	return -1; // Returns -1 on Non ARM CPU's.
+#endif
+}


Why not keep it simple? One does not need a check, on whether or not svelen is supported, as it should be initialized to 0, shouldn't it?

Suggested change

static inline int cpuinfo_get_max_arm_sve_length(void) {

#if CPUINFO_ARCH_ARM64

if ((cpuinfo_isa.sve || cpuinfo_isa.sve2) && (cpuinfo_isa.svelen > 0))

return cpuinfo_isa.svelen;

else

return -1; // Returns -1 on Non SVE supported ARM CPU's.

#else

return -1; // Returns -1 on Non ARM CPU's.

#endif

}

static inline uint32_t cpuinfo_get_max_arm_sve_length(void) {

#if CPUINFO_ARCH_ARM64

return cpuinfo_isa.svelen;

#else

return 0;

#endif

}

Agree, changes done.

malfet · 2024-08-07T08:25:07Z

src/arm/linux/aarch64-isa.c

+		// Mask out the SVE vector length bits
+		isa->svelen = ret & PR_SVE_VL_LEN_MASK;
+	}
+#endif


Suggested change

#endif

#else

isa->svelen = 0;

#endif

This change is not required anymore as we have removed the following check - #if defined(linux) (Not required as the code is compiled for Linux anyways).

malfet · 2024-08-07T08:27:36Z

src/arm/linux/aarch64-isa.c

@@ -151,4 +151,25 @@ void cpuinfo_arm64_linux_decode_isa_from_proc_cpuinfo(
 	if (features & CPUINFO_ARM_LINUX_FEATURE_ASIMDFHM) {
 		isa->fhm = true;
 	}
+
+#if defined(__linux__)
+#include <sys/prctl.h>


Adding #include directive in the body of the function feels very fragile, can you please move it to the top level of the file?

malfet · 2024-08-07T08:28:08Z

src/arm/linux/aarch64-isa.c

@@ -151,4 +151,25 @@ void cpuinfo_arm64_linux_decode_isa_from_proc_cpuinfo(
 	if (features & CPUINFO_ARM_LINUX_FEATURE_ASIMDFHM) {
 		isa->fhm = true;
 	}
+
+#if defined(__linux__)


Why does it needs #ifdef __linux__, as this file should only be compiled for Linux anyway, isn't it?

Agree. Removed this macro.

Signed-off-by: maajidkhann <[email protected]>

maajidkhann · 2024-08-07T09:30:36Z

Thank you for all the updates, looks good, but please consider addressing few minor remarks

All changes made as suggested.

Here's the test output, it now returns 256 on Graviton 3 EC2 instance.

maajidkhann · 2024-08-07T09:42:15Z

I was also able to call this new API from Pytorch source code and access the vector length correctly.

Regression PR : pytorch/cpuinfo#255 Signed-off-by: Nikhil Gupta <[email protected]> Change-Id: I56cec061072be11ec33ccb661114360b979fc7aa

…re (#135857) Regression PR : pytorch/cpuinfo#255 Change-Id: I56cec061072be11ec33ccb661114360b979fc7aa Pull Request resolved: #135857 Approved by: https://github.com/digantdesai, https://github.com/malfet

…re (pytorch#135857) Regression PR : pytorch/cpuinfo#255 Change-Id: I56cec061072be11ec33ccb661114360b979fc7aa Pull Request resolved: pytorch#135857 Approved by: https://github.com/digantdesai, https://github.com/malfet

Add a API to check SVE Length support on ARM CPU.

3208502

Signed-off-by: maajidkhann <[email protected]>

facebook-github-bot added the cla signed label Aug 6, 2024

malfet requested changes Aug 6, 2024

View reviewed changes

malfet reviewed Aug 6, 2024

View reviewed changes

Added functionality to directly return SVE VL.

6d852c3

->Addresses review comments and adds a function that directly returns the max VL on ARM supported CPU's. ->Fixes Lint Issues. Signed-off-by: maajidkhann <[email protected]>

malfet approved these changes Aug 7, 2024

View reviewed changes

Minor changes to remove unwanted checks.

28e1580

Signed-off-by: maajidkhann <[email protected]>

malfet merged commit 16bfc16 into pytorch:main Aug 7, 2024
11 checks passed

malfet mentioned this pull request Aug 30, 2024

Extending the Pytorch vec backend for SVE (ARM) pytorch/pytorch#119571

Closed

ng-05 mentioned this pull request Sep 11, 2024

[ARM][feat]: Add KleidiAI Backend & enable 4 bit matmul operators pytorch/pytorch#134124

Open

ng-05 mentioned this pull request Sep 12, 2024

[Fix]: Update CPUINFO submodule to fix support for NON-SVE ARM Hardware pytorch/pytorch#135857

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a API to check SVE Length support on ARM CPU. #255

Add a API to check SVE Length support on ARM CPU. #255

maajidkhann commented Aug 6, 2024 •

edited

Loading

maajidkhann commented Aug 6, 2024

malfet left a comment •

edited

Loading

malfet Aug 6, 2024

maajidkhann Aug 6, 2024

malfet Aug 6, 2024

maajidkhann Aug 6, 2024

malfet Aug 6, 2024

maajidkhann Aug 6, 2024

malfet Aug 6, 2024

maajidkhann Aug 6, 2024

maajidkhann commented Aug 6, 2024

malfet left a comment

malfet Aug 7, 2024

maajidkhann Aug 7, 2024

malfet Aug 7, 2024

maajidkhann Aug 7, 2024

malfet Aug 7, 2024

maajidkhann Aug 7, 2024

malfet Aug 7, 2024

maajidkhann Aug 7, 2024

malfet Aug 7, 2024

maajidkhann Aug 7, 2024

malfet Aug 7, 2024

maajidkhann Aug 7, 2024

maajidkhann commented Aug 7, 2024

maajidkhann commented Aug 7, 2024

	perror("prctl(PR_SVE_GET_VL) failed");
	cpuinfo_log_error("prctl(PR_SVE_GET_VL) failed");

-#endif
+#else
+        isa->svelen = 0;
+#endif

Add a API to check SVE Length support on ARM CPU. #255

Add a API to check SVE Length support on ARM CPU. #255

Conversation

maajidkhann commented Aug 6, 2024 • edited Loading

maajidkhann commented Aug 6, 2024

malfet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maajidkhann commented Aug 6, 2024

malfet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maajidkhann commented Aug 7, 2024

maajidkhann commented Aug 7, 2024

maajidkhann commented Aug 6, 2024 •

edited

Loading

malfet left a comment •

edited

Loading