Enable new win-arm64 platform by gaborkertesz-linaro · Pull Request #82 · pytorch/cpuinfo

gaborkertesz-linaro · 2022-03-11T15:02:41Z

This PR enables the new Windows on Arm (win-arm64) platform, by implementing the required APIs, reading topology information via Windows API.

gaborkertesz-linaro · 2022-03-11T15:05:32Z

_win-arm64_cmake-config.log
_win-arm64_cmake-build.log
_win-arm64_init-test.log
_win-arm64_cache-info.log
_win-arm64_cpu-info.log
_win-arm64_isa-info.log

gaborkertesz-linaro · 2022-03-11T20:44:11Z

Build on win-arm64
cmake . -G "Visual Studio 16 2019" -A ARM64
cmake --build .

src/arm/windows/init.c

src/arm/windows/init_by_logical_sys_info.c

marcpems · 2022-03-15T11:09:53Z

src/arm/windows/init_by_logical_sys_info.c

+				break;
+			}
+			/* Reset the lowest bit in affinity mask */
+			group_processors_mask &= (group_processors_mask - 1);


This resets the most significant bits, not the lowest bit.
Would it be clearer to shift right and test for 1?

I've inherited this code from x86 init: https://github.com/pytorch/cpuinfo/blob/master/src/x86/windows/init.c#L229
I mean it's absolutely not an excuse, I'll check that again.

I think it really clears all bits, that are lower than the least set bit.

ff 11111111
fe 11111110
fc 11111100
f8 11111000
f0 11110000
e0 11100000
c0 11000000
80 10000000
0 00000000

0x10
0x0

0x20
0x0

0x40
0x0

0x80
0x0

src/arm/windows/init_by_logical_sys_info.c

src/arm/windows/init.c

marcpems · 2022-03-15T11:23:44Z

src/arm/windows/init_by_logical_sys_info.c

+	}
+
+	/* 2. Read topology information via MSDN API: packages, cores and caches*/
+	nr_of_packages = read_packages_for_processors(


there are many things that can go wrong in these calls, but there is no way to chain the error and exit the function on error. That's not great for defensive code.
Consider adding throw/catch if permitted.

As far as I see, there are not throw/catch in the current codebase and I'd think that's intentional, so I'd try to cleanup the error paths and make sure it's going to be safe.

Yes C code mostly

src/arm/windows/init_by_logical_sys_info.c

marcpems

The general principals of querying CPU details and assigning to the cpuinfo relevant structures looks ok to me. There are some issues with the error handling here that might need addressing.

src/arm/windows/init.c

gaborkertesz-linaro · 2022-03-23T16:12:43Z

@malfet Could you review it? Thanks!

gaborkertesz-linaro · 2022-03-25T10:20:16Z

related PR:
pytorch/pytorch#72424

malfet

Mostly looks good to me, but please explain why ARM64 needs a special treatment in ELSEIF(NOT CPUINFO_TARGET_PROCESSOR_MATCHES
And use bool instead of BOOL whenever possible (i.e. in callback its probably fine, but otherwise should not be used)

CMakeLists.txt

cmake/DownloadGoogleBenchmark.cmake

src/arm/windows/init.c

src/arm/windows/windows_arm_init.h

src/arm/windows/init_by_logical_sys_info.c

gaborkertesz-linaro · 2022-03-30T08:40:04Z

@malfet Can I do anything else to close this and the related PR?

strega-nil-ms · 2022-04-12T16:42:59Z

any updates on this?

gaborkertesz-linaro · 2022-04-21T09:49:33Z

any updates on this?

Not on my side, but I'd happily close this!

niyas-sait · 2022-05-09T11:38:43Z

Can this be merged, please?

kalaskarsanket · 2022-06-15T15:14:25Z

Any plans on merging this PR ?

Maratyszcza

Use dash - to separate filename parts rather than underscore _ for consistency with other files in cpuinfo.

Maratyszcza · 2022-06-15T20:49:02Z

src/arm/windows/init.c

+};
+
+/* Please add new SoC/chip info here! */
+static struct woa_chip_info woa_chips[] = {


This covers literally two devices. We need a more generic detection method that doesn't require hard-coding every SoC.

I absolutely agree that this solution is not ideal, but we haven't found any better and still robust option considering the existing Windows API. Also new SoCs won't turn up frequently, so even though it's a limited solution, it can be maintained.

I had a similar concern but there doesn't seem to be any better way to do it with the help of Windows API.

Ampere SoC also runs Windows/Arm64 - Does that need to be added?

Would be nice, but I can't really test that, so I'd ask to do as a follow-up commit.

Maratyszcza · 2022-06-15T20:50:03Z

CMakeLists.txt

      LIST(APPEND CPUINFO_SRCS
        src/arm/android/properties.c)
    ENDIF()
+    ELSEIF(CMAKE_SYSTEM_NAME STREQUAL "Windows" AND CPUINFO_TARGET_PROCESSOR STREQUAL "ARM64")


Wrong indentation?

kaadam

Hi Gabor, thanks for working on this. It will help to solve our build issue in WoA Chromium. I managed to backport your PR to Chromium to check how the build works.
I left some comments about my findings. Basically lot of them are warnings reported by Clang-cl which treat as errors. Thanks.

kaadam · 2022-06-23T15:20:29Z

src/arm/windows/init.c

+static void set_cpuinfo_isa_fields();
+static bool get_system_info_from_registry(
+	struct woa_chip_info** chip_info,
+	enum cpu_info_vendor* vendor);


typo: cpu_info_vendor -> cpuinfo_vendor

kaadam · 2022-06-23T15:21:02Z

src/arm/windows/init.c

+
+struct vendor_info {
+	char vendor_name[VENDOR_NAME_MAX];
+	enum cpu_info_vendor cpu_info_vendor;


same as above

kaadam · 2022-06-23T15:22:34Z

src/arm/windows/init.c

+	PINIT_ONCE init_once, PVOID parameter, PVOID* context)
+{
+	struct woa_chip_info *chip_info = NULL;
+	enum cpu_info_vendor vendor = cpuinfo_vendor_unknown;


typo: cpu_info_vendor -> cpuinfo_vendor

kaadam · 2022-06-23T15:23:17Z

src/arm/windows/init.c

+
+static bool get_system_info_from_registry(
+	struct woa_chip_info** chip_info,
+	enum cpu_info_vendor* vendor)


kaadam · 2022-06-23T15:26:56Z

src/arm/windows/windows_arm_init.h

@@ -0,0 +1,32 @@
+#pragma once
+
+bool get_core_uarch_for_efficiency(


Please move these functions down to avoid 'forward references to enum' reported by Clang-cl.

kaadam · 2022-06-23T15:37:39Z

src/arm/windows/init.c

+	bool result = false;
+	char* textBuffer = NULL;
+	LPCTSTR cpu0_subkey =
+		"HARDWARE\\DESCRIPTION\\System\\CentralProcessor\\0";


Please mark these strings as wide-character literals by using 'L'.
L"HARDWARE\DESCRIPTION\System\CentralProcessor\0"

Interestingly the MSVC compiler throws the following error to 'L' , even though I'd expected your proposal is the correct one:

warning C4133: 'initializing': incompatible types - from 'unsigned short [47]' to 'LPCTSTR'

Ohh okay, seems using (LPCTSTR) cast works fine.

kaadam · 2022-06-24T07:10:35Z

src/arm/windows/init_by_logical_sys_info.c

+				break;
+				case CacheData:
+					processors[processor_global_index].cache.l1d = current_cache;
+				break;


same here as above

kaadam · 2022-06-24T07:11:45Z

src/arm/windows/init_by_logical_sys_info.c

+				case CacheData:
+					current_cache = l1d_base + l1d_counter;
+					l1d_counter++;
+				break;


same here as above

kaadam · 2022-06-24T07:12:09Z

src/arm/windows/init_by_logical_sys_info.c

+					break;
+					case CacheData:
+						numbers_of_caches[cpuinfo_cache_level_1d]++;
+					break;


Clang-cl complains about the missing other two enum values:
"error: enumeration values 'CacheUnified' and 'CacheTrace' not handled in switch [-Werror,-Wswitch]"
Could you add these values as empty cases as well?

kaadam · 2022-06-24T07:22:27Z

src/arm/windows/init.c

+/* Please add new SoC/chip info here! */
+static struct woa_chip_info woa_chips[] = {
+	/* Microsoft SQ1 Kryo 495 4 + 4 cores (3 GHz + 1.80 GHz) */
+	"Microsoft SQ1",


Coud you add some extra braces to each struct elements?
../../third_party/cpuinfo/src/src/arm/windows/init.c(39,2): error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] "Microsoft SQ1", ^~~~~~~~~~~~~~~~ { ../../third_party/cpuinfo/src/src/arm/windows/init.c(52,2): error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] "Microsoft SQ2", ^~~~~~~~~~~~~~~~ {

niyas-sait · 2022-06-24T08:08:33Z

(@gaborkertesz-linaro is on holiday now and planning to address the comments in couple of weeks)

At the moment the Chrome build is broken for Windows on ARM, since ruy build relies on cpuinfo third_party library which doesn't support Windows on ARM port yet. There is a pull request which implements the required functions for WoA platfrom [1]. After it is landed we can re-enable cpuinfo build again. [1] pytorch/cpuinfo#82 Bug: 1329774 Change-Id: Idb619eaa3180fac5068a939704151da0f653ee89 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3721652 Reviewed-by: Tarun Bansal <tbansal@chromium.org> Commit-Queue: Tarun Bansal <tbansal@chromium.org> Cr-Commit-Position: refs/heads/main@{#1018705}

gaborkertesz-linaro · 2022-07-05T09:00:57Z

I hope I've fixed all of your findings, please let me know if I missed any of them!
Unfortunetaly I wasn't able to setup building with LLVM by CMake, but I've built with '-Wall' by MSVC compiler.

kaadam · 2022-07-05T09:45:49Z

I hope I've fixed all of your findings, please let me know if I missed any of them! Unfortunetaly I wasn't able to setup building with LLVM by CMake, but I've built with '-Wall' by MSVC compiler.

Gabor, thanks for updating it. Checking on my side, I get back to you asap.

src/arm/windows/init-by-logical-sys-info.c

This patch implements the required APIs for the new win-arm64 platform by reading topology information via Windows API. Build config: cmake . -A ARM64

kaadam

Great, thanks. I'm not a reviewer, but I give you an informal LGTM.

gaborkertesz-linaro · 2022-07-05T15:42:15Z

@malfet What do you think?

This patch implements the required APIs for the new win-arm64 platform by reading topology information via Windows API. Build config: cmake . -A ARM64 GitOrigin-RevId: 1baac2b

facebook-github-bot added the cla signed label Mar 11, 2022

gaborkertesz-linaro force-pushed the win-arm64 branch from 5933e1a to cf12755 Compare March 11, 2022 20:46

marcpems reviewed Mar 14, 2022

View reviewed changes

src/arm/windows/init.c Show resolved Hide resolved

marcpems reviewed Mar 14, 2022

View reviewed changes

src/arm/windows/init.c Outdated Show resolved Hide resolved

marcpems reviewed Mar 14, 2022

View reviewed changes

src/arm/windows/init.c Show resolved Hide resolved