[ms] Reinstate https://reviews.llvm.org/D14748 after https://reviews.llvm.org/D20291

[hans]

lib/Headers/x86intrin.h

49

I'm worried about the conditional here. IIRC, ffmpeg uses TZCNT just as a faster encoding for BSF, but now we're putting a conditional in the way, so this will be slower actually. On the other hand, the alternative is weird too :-/

[probinson]

lib/Headers/x86intrin.h

49

I thought there was a peephole to notice a guard like this and do the right thing? In which case having the guard is fine.

[hans]

lib/Headers/x86intrin.h

49

But for non-BMI targets it won't be able to remove the guard (unless it can prove the argument is zero).

MSVC will just emit the raw 'tzcnt' instruction here, and that's what ffmpeg wants.

[andreadb]

lib/Headers/x86intrin.h

49

I understand your point. However, the semantic of tzcnt_u32 (at least for BMI) is well defined on zero input. Changing that semantic for non-BMI targets sounds a bit odd/confusing to me.

I guess ffmpeg is reliant on the fact that other compilers would either preserve the intrinsic call until instruction selection, or fold it to a meaningful value (i.e. 32 in this case) when the input is known to be zero. If we remove the zero check from tzcnt_u32 (for non BMI targets), we let the optimizer enable rules to aggressively fold calls to tzcnt_u32 to undef when the input is zero.

As a side note: I noticed that gcc provides intrinsic __bsfd in ia32intrin.h. Maybe we should do something similar?

[hans]

lib/Headers/x86intrin.h

49

Yes, ffmpeg relies on the compiler to emit the raw TZCNT instruction even for non-BMI targets and don't call it with zero inputs. They just want a faster BSF.

It's all pretty weird, but maybe Nico's original patch is the way to do this after all. At least then, the intrinsic will always do what it says in the doc.

lib/Headers/x86intrin.h

 #include <immintrin.h>
 
 #if !defined(_MSC_VER) || __has_feature(modules) || defined(__3dNOW__)
 #include <mm3dnow.h>
 #endif
 
 #if !defined(_MSC_VER) || __has_feature(modules) || defined(__BMI__)
 #include <bmiintrin.h>
+#else
+/* The TZCNT is special in that it's encoded in a backward-compatible way and
+ * behaves as BSF on non-BMI targets, hence some compilers provide it even
+ * if the target doesn't support BMI instructions.  LLVM won't emit the TZCNT
+ * instruction if BMI isn't available, but to not break code assuming
+ * unconditional availability of the tzcnt instruction, provide the intrinsic
+ * here (with a BMI-independent implementation). */
+#define _tzcnt_u16(a)     (__tzcnt_u16((a)))
+#define _tzcnt_u32(a)     (__tzcnt_u32((a)))
+static __inline__ unsigned short __attribute__((__always_inline__, __nodebug__))
+__tzcnt_u16(unsigned short __X) { return __X ? __builtin_ctzs(__X) : 16; }
+static __inline__ unsigned int __attribute__((__always_inline__, __nodebug__))
+__tzcnt_u32(unsigned int __X) { return __X ? __builtin_ctz(__X) : 32; }
+#ifdef __x86_64__
+#define _tzcnt_u64(a)     (__tzcnt_u64((a)))
+static __inline__ unsigned long long
+__attribute__((__always_inline__, __nodebug__))
+__tzcnt_u64(unsigned long long __X) { return __X ? __builtin_ctzll(__X) : 64; }
+#endif /* __x86_64__ */
 #endif
 
 #if !defined(_MSC_VER) || __has_feature(modules) || defined(__BMI2__)
 #include <bmi2intrin.h>
 #endif
 
 #if !defined(_MSC_VER) || __has_feature(modules) || defined(__LZCNT__)
 #include <lzcntintrin.h>

test/Headers/tzcnt.c

+// RUN: %clang_cc1 -triple i386-pc-win32 -target-cpu broadwell \
+// RUN:     -fms-extensions -fms-compatibility -fms-compatibility-version=17.00 \
+// RUN:     -ffreestanding -emit-obj -o /dev/null -Werror \
+// RUN:     -isystem %S/Inputs/include %s
+
+// RUN: %clang_cc1 -triple x86_64-pc-win32  \
+// RUN:     -fms-extensions -fms-compatibility -fms-compatibility-version=17.00 \
+// RUN:     -ffreestanding -emit-obj -o /dev/null -Werror \
+// RUN:     -isystem %S/Inputs/include %s
+//
+// REQUIRES: x86-registered-target
+
+typedef __SIZE_TYPE__ size_t;
+#include <intrin.h>
+
+void f() {
+  // Check that _tzcnt is available even without BMI; ffmpeg relies on this.
+  __tzcnt_u16(0);
+  __tzcnt_u32(0);
+#ifdef __x86_64__
+  __tzcnt_u64(0);
+#endif
+}