[PATCH] D105263: [X86] AVX512FP16 instructions enabling 1/6

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: clang/lib/Headers/avx512fp16intrin.h:254
+/// Constructs a 512-bit floating-point vector of [32 x half] from a
+///    128-bit floating-point vector of [16 x half]. The lower 256 bits
+///    contain the value of the source vector. The upper 256 bits are set
----------------
256-bit


================
Comment at: clang/lib/Headers/avx512vlfp16intrin.h:74
+static __inline__ __m256h __DEFAULT_FN_ATTRS256 _mm256_abs_ph(__m256h __A) {
+  return (__m256h)_mm256_and_epi32(_mm256_set1_epi32(0x7FFF7FFF), (__m256i)__A);
+}
----------------
Why do we use _mm256_set1_epi32 instead of _mm256_set1_epi16?


================
Comment at: clang/lib/Headers/avx512vlfp16intrin.h:78
+static __inline__ __m128h __DEFAULT_FN_ATTRS128 _mm_abs_ph(__m128h __A) {
+  return (__m128h)_mm_and_epi32(_mm_set1_epi32(0x7FFF7FFF), (__m128i)__A);
+}
----------------
Same question


================
Comment at: llvm/include/llvm/IR/RuntimeLibcalls.def:290
 HANDLE_LIBCALL(FPEXT_F16_F128, "__extendhftf2")
+HANDLE_LIBCALL(FPEXT_F16_F80, "__extendhfxf2")
 HANDLE_LIBCALL(FPEXT_F32_F64, "__extendsfdf2")
----------------
Is this tested in this patch?


================
Comment at: llvm/lib/Target/X86/X86FastISel.cpp:58
   bool X86ScalarSSEf32;
+  bool X86ScalarAVXf16;
 
----------------
AVX here should maybe be AVX512, but maybe this is pointing out that this name is bad. Would X86ScalarXMMf* be better?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:13537
+    unsigned MovOpc = 0;
+    if (EltVT == MVT::f16) {
+      MovOpc = X86ISD::MOVSH;
----------------
Drop curly braces on these.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105263/new/

https://reviews.llvm.org/D105263