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

clang-cl --target=arm64-pc-windows-msvc doesn't support __umulh needed by WinSDK 10.0.20348.0 #50472

Closed
StephanTLavavej opened this issue Jul 18, 2021 · 5 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@StephanTLavavej
Copy link
Member

Bugzilla Link 51128
Resolution FIXED
Resolved on Oct 11, 2021 20:29
Version 12.0
OS Windows NT
Blocks #51489
CC @dwblaikie,@mstorsjo,@nico,@rnk,@tstellar
Fixed by commit(s) cc3affd ddc49d0

Extended Description

Here's how I reproed this (there may be a simpler way). Install VS 2022 17.0 Preview 2. After choosing the "Desktop development with C++" workload, customize it:

  • Select the "C++ Clang tools for Windows" (containing Clang 12)
  • Select the "MSVC v142 - VS 2019 C++ ARM64 build tools (Latest)" (slightly misnamed; this is the VS 2022 compiler)
  • Unselect the default WinSDK 10.0.19041.0
  • Select the latest "Windows 10 SDK (10.0.20348.0)"

This setup (Clang 12 + latest WinSDK) works for x86, x64, and ARM (32-bit) in the microsoft/STL test suite; however, we encountered an incompatibility on ARM64 due to the new WinSDK attempting to use the __umulh intrinsic, which works for MSVC but not Clang 12. Self-contained test case, starting from a clean command prompt:

C:\Temp>"C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Auxiliary\Build\vcvarsall.bat" x64_arm64


** Visual Studio 2022 Developer Command Prompt v17.0.0-pre.2.0
** Copyright (c) 2021 Microsoft Corporation


[vcvarsall.bat] Environment initialized for: 'x64_arm64'

C:\Temp>type meow.cpp
#include <Windows.h>

C:\Temp>cl
Microsoft (R) C/C++ Optimizing Compiler Version 19.30.30401 for ARM64
Copyright (C) Microsoft Corporation. All rights reserved.

usage: cl [ option... ] filename... [ /link linkoption... ]

C:\Temp>cl /EHsc /nologo /W4 /c meow.cpp
meow.cpp

C:\Temp>clang-cl --target=arm64-pc-windows-msvc --version
clang version 12.0.0
Target: arm64-pc-windows-msvc
Thread model: posix
InstalledDir: C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\Llvm\x64\bin

C:\Temp>clang-cl --target=arm64-pc-windows-msvc /EHsc /nologo /W4 /c meow.cpp
In file included from meow.cpp:1:
In file included from C:\Program Files (x86)\Windows Kits\10\include\10.0.20348.0\um\Windows.h:171:
In file included from C:\Program Files (x86)\Windows Kits\10\include\10.0.20348.0\shared\windef.h:24:
In file included from C:\Program Files (x86)\Windows Kits\10\include\10.0.20348.0\shared\minwindef.h:182:
C:\Program Files (x86)\Windows Kits\10\include\10.0.20348.0\um\winnt.h(6370,20): error: use of undeclared identifier
'__umulh'
*HighProduct = UnsignedMultiplyHigh(Multiplier, Multiplicand);
^
C:\Program Files (x86)\Windows Kits\10\include\10.0.20348.0\um\winnt.h(6236,30): note: expanded from macro
'UnsignedMultiplyHigh'
#define UnsignedMultiplyHigh __umulh
^
1 error generated.

@mstorsjo
Copy link
Member

  • Unselect the default WinSDK 10.0.19041.0
  • Select the latest "Windows 10 SDK (10.0.20348.0)"

Thanks for the report!

The latest SDK I happen to have handy is 10.0.22000.0, and that one happens to have this block of code:

---8<---
// Most ARM64/AArch64 intrinsics available in MSVC are not currently available in other compilers.
// clang-cl defines _M_ARM64, so we need a better escape hatch to avoid using these intrinsics,
// as well as allow us to re-enable them in the event clang-cl starts supporting the msvc intrinsics.
#if !defined(ARM64_MULT_INTRINSICS_SUPPORTED)
#if defined(_MSC_VER) && !defined(clang)
#define ARM64_MULT_INTRINSICS_SUPPORTED 1
#else
#define ARM64_MULT_INTRINSICS_SUPPORTED 0
#endif
#endif
---8<---

In that version, this issue doesn't happen (as the actual use of __umulh is ifdeffed out), but if I enable the use of these intrinsics on Clang, I do get the error you're describing. I take it that the 10.0.20348.0 SDK has removed this workaround?

(Side note, I guess we should dig through the other MSVC headers to see what other intrinsics that might be worthwhile to implement, as the comment seems to imply that there's lots of ones that we're missing.)

In any case, it should be pretty straightforward to add support for these intrinsics, I've got a WIP locally that seems to work; I'll try to brush it up and add tests to make it submittable.

@mstorsjo
Copy link
Member

I posted an initial patch to implement this in https://reviews.llvm.org/D106721.

@mstorsjo
Copy link
Member

Pushed cc3affd which implements the __mulh and __umulh builtins for MSVC/arm64 mode.

The patch applies cleanly on the release/13.x branch, so if it's not seen as too risky (I think it's quite safe) I think it'd be good to pull this into the upcoming release too.

@tstellar
Copy link
Collaborator

Merged: ddc49d0

@tstellar
Copy link
Collaborator

mentioned in issue #51489

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

3 participants