[MSVC] Add ARM support to intrin.h for MSVC compatibility

[mstorsjo]

Summary

This fixes compiling with headers from the Windows SDK for ARM, where the YieldProcessor function (in winnt.h) refers to _ARM_BARRIER_ISHST.

The actual MSVC armintr.h contains a lot more definitions, but this is enough to build code that uses the Windows SDK but doesn't use ARM intrinsics directly.

An alternative would to just keep the addition to intrin.h (to include armintr.h), but not actually ship armintr.h, instead having clang's intrin.h include armintr.h from MSVC's include directory. (That one works fine with clang, at least for building code that uses the Windows SDK.)

[rengolin]

Can you add a test on test/Headers, like the others, please?

I'm not well versed on Windows. @compnerd?

lib/Headers/armintr.h

2

Maybe a hint that this is "ARM Windows Intrinsics"?

28

Maybe an ifdef Windows wrapper, to make sure not to mess up *nix environments?

[mstorsjo]

In D22774#501993, @rengolin wrote:

Can you add a test on test/Headers, like the others, please?

Sure, there seems to be a good spot for that in ms-intrin.cpp.

lib/Headers/armintr.h

2

Sure

28

I can add the same as at the top of intrin.h, i.e. this:

/* Only include this if we're compiling for the windows platform. */
#ifndef _MSC_VER
#include_next <intrin.h>
#else

[rengolin]

lib/Headers/armintr.h

28

Could be. I know you have that on the including header, but nothing stops people from including this one too by accident. Also, it makes it more clear where it should work and where it shouldn't.

[mstorsjo]

lib/Headers/armintr.h

28

No, I meant just like you want, but instead of #ifdef _WIN32 use #ifndef _MSC_VER, and use #include_next <armintr.h> for when not working in MSVC mode, i.e. behave as if our header didn't exist at all.

So I agree that it's useful to have on this individual header as well, not only rely on this guard in the intrin.h.

[rengolin]

lib/Headers/armintr.h

28

Sounds good.

[mstorsjo]

Added a test, added "windows" to the header title line, added an #ifndef _MSC_VER #include_next to the header.

[rengolin]

LGTM. Thanks!

[compnerd]

lib/Headers/armintr.h

26

Hmm, why not do __has_header instead? armv7-windows-itanium won't define _MSC_VER but you may have the header from the SDK.

[mstorsjo]

lib/Headers/armintr.h

26

Hmm, so you mean like this?

#if __has_include_next(<armintr.h>)
#include_next <armintr.h>
#else
// Normal file content
typedef enum ...

That would cause use in armv7-windows-msvc mode to use the SDK's armintr.h instead of ours - which as far as I know also is fine. (The issue I'm mainly fixing is that including <intrin.h> is expected to bring in this, but currently it doesn't.)

I'm not sure I follow your argument completely though - armv7-windows-itanium doesn't define _MSC_VER, and thus would already try to include the next armintr.h, which would be picked up from the SDK if it exists there.

[mstorsjo]

Ping - any reply from @compnerd?

In any case, feel free to formulate the ifdefs whichever way you want on commit as well (since I don't have commit access), as long as the included test passes.

[mstorsjo]

Ping @compnerd

[compnerd]

I had reversed the condition in my head. Why include_next the header if not on Windows?

[mstorsjo]

In D22774#507014, @compnerd wrote:

Why include_next the header if not on Windows?

Mostly because intrin.h uses the same logic. Normally, there's probably no such system header in other SDKs, but say if you happen to have a custom one named like that (on a non-windows platform unaware that windows happens to have a system header named like that) and clang's builtin header gets picked first, include the next one in the include directory chain instead - i.e. pretend like this one doesn't exist. (That is, either pick the one you want, or error out saying it isn't found.)

I don't mind particularly much though, I just picked this logic since intrin.h was using it as well. Just having the header be #ifdef _MSC_VER {content} #endif, or #ifndef _MSC_VER #error #else {content} #endif also would be fine for me.

[rengolin]

The include_next is to make sure that, whatever other environment that happens to have the same include name, should pick it up if the user or the environment provides one, or errors out if it doesn't. Pretty standard.

[compnerd]

Ah, okay. Sounds good.

[mstorsjo]

Can someone commit this for me?

[compnerd]

SVN r277928

lib/Headers/CMakeLists.txt

 set(files
   adxintrin.h
   altivec.h
   ammintrin.h
   arm_acle.h
+  armintr.h
   avx2intrin.h
   avx512bwintrin.h
   avx512cdintrin.h
   avx512dqintrin.h
   avx512erintrin.h
   avx512fintrin.h
   avx512ifmaintrin.h
   avx512ifmavlintrin.h

lib/Headers/armintr.h

+/*===---- armintr.h - ARM Windows intrinsics -------------------------------===
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ *===-----------------------------------------------------------------------===
+ */
+
+/* Only include this if we're compiling for the windows platform. */
+#ifndef _MSC_VER
+#include_next <armintr.h>
+#else
+
+#ifndef __ARMINTR_H
+#define __ARMINTR_H
+
+typedef enum
+{
+  _ARM_BARRIER_SY    = 0xF,
+  _ARM_BARRIER_ST    = 0xE,
+  _ARM_BARRIER_ISH   = 0xB,
+  _ARM_BARRIER_ISHST = 0xA,
+  _ARM_BARRIER_NSH   = 0x7,
+  _ARM_BARRIER_NSHST = 0x6,
+  _ARM_BARRIER_OSH   = 0x3,
+  _ARM_BARRIER_OSHST = 0x2
+} _ARMINTR_BARRIER_TYPE;
+
+#endif /* __ARMINTR_H */
+#endif /* _MSC_VER */

lib/Headers/intrin.h

 #ifndef __INTRIN_H
 #define __INTRIN_H
 
 /* First include the standard intrinsics. */
 #if defined(__i386__) || defined(__x86_64__)
 #include <x86intrin.h>
 #endif
 
+#if defined(__arm__)
+#include <armintr.h>
+#endif
+
 /* For the definition of jmp_buf. */
 #if __STDC_HOSTED__
 #include <setjmp.h>
 #endif
 
 /* Define the default attributes for the functions in this file. */
 #define __DEFAULT_FN_ATTRS __attribute__((__always_inline__, __nodebug__))
 

test/Headers/ms-intrin.cpp

#endif
   __readmsr(0);
 
   // FIXME: Call these in 64-bit too once the intrinsics have been fixed to
   // work there, PR19301
 #ifndef _M_X64
   __readcr3();
   __writecr3(0);
 #endif
+
+#ifdef _M_ARM
+  __dmb(_ARM_BARRIER_ISHST);
+#endif
 }