[PATCH] D105268: [X86] AVX512FP16 instructions enabling 5/6

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: clang/include/clang/Basic/BuiltinsX86.def:2010
+TARGET_BUILTIN(__builtin_ia32_vfmaddph, "V8xV8xV8xV8x", "ncV:128:", "avx512fp16,avx512vl")
+TARGET_BUILTIN(__builtin_ia32_vfmaddph256, "V16xV16xV16xV16x", "ncV:256:", "avx512fp16,avx512vl")
+
----------------
Can we arrange the vfmaddph variant together?  Move it to line 1997?
Why there is no mask version for 128 and 256?


================
Comment at: clang/include/clang/Basic/BuiltinsX86.def:2014
+
+TARGET_BUILTIN(__builtin_ia32_vfmaddsh3_mask, "V8xV8xV8xV8xUcIi", "ncV:128:", "avx512fp16")
+TARGET_BUILTIN(__builtin_ia32_vfmaddsh3_maskz, "V8xV8xV8xV8xUcIi", "ncV:128:", "avx512fp16")
----------------
What does "3" stand for?


================
Comment at: clang/lib/Headers/avx512vlfp16intrin.h:1385
+                                                                  __m128h __C) {
+  return (__m128h)__builtin_ia32_selectph_128(
+      (__mmask8)__U,
----------------
Sorry, I'm confused sometimes we use mask builtin, sometimes we use select builtin. Any guideline on it?


================
Comment at: llvm/include/llvm/IR/IntrinsicsX86.td:5709
+
+  def int_x86_avx512fp16_vfmadd_ph_512
+      : Intrinsic<[ llvm_v32f16_ty ],
----------------
I notice there is no builtin bound to this intrinsic. What is it used for?


================
Comment at: llvm/include/llvm/IR/IntrinsicsX86.td:5727
+                  [ IntrNoMem, ImmArg<ArgIndex<3>> ]>;
+  def int_x86_avx512fp16_vfmadd_f16
+      : Intrinsic<[ llvm_half_ty ],
----------------
ph?


================
Comment at: llvm/lib/Target/X86/X86InstrFMA3Info.cpp:148
+
+#define FP16_FMA3GROUP_PACKED_AVX512(Name, Suf, Attrs)                         \
+  FMA3GROUP_PACKED_AVX512_WIDTHS(Name, PH, Suf, Attrs)
----------------
Can we integrate it to FMA3GROUP_PACKED_AVX512() with PH extended?


================
Comment at: llvm/lib/Target/X86/X86InstrFMA3Info.cpp:151
+
+#define FP16_FMA3GROUP_PACKED_AVX512_ROUND(Name, Suf, Attrs)                   \
+  FMA3GROUP_MASKED(Name, PHZ##Suf, Attrs)
----------------
Ditto.


================
Comment at: llvm/lib/Target/X86/X86InstrFMA3Info.cpp:154
+
+#define FP16_FMA3GROUP_SCALAR_AVX512_ROUND(Name, Suf, Attrs)                   \
+  FMA3GROUP(Name, SHZ##Suf, Attrs)                                             \
----------------
Ditto.


================
Comment at: llvm/lib/Target/X86/X86InstrFMA3Info.cpp:158
+
+static const X86InstrFMA3Group FP16BroadcastGroups[] = {
+  FP16_FMA3GROUP_PACKED_AVX512(VFMADD, mb, 0)
----------------
Ditto.


================
Comment at: llvm/lib/Target/X86/X86InstrFMA3Info.cpp:167
+
+static const X86InstrFMA3Group FP16RoundGroups[] = {
+  FP16_FMA3GROUP_PACKED_AVX512_ROUND(VFMADD, rb, 0)
----------------
Ditto.


================
Comment at: llvm/lib/Target/X86/X86InstrFMA3Info.cpp:208
                  (BaseOpcode >= 0xB6 && BaseOpcode <= 0xBF));
+  bool IsFMA3H = (TSFlags & X86II::EncodingMask) == X86II::EVEX &&
+                 (TSFlags & X86II::OpMapMask) == X86II::T_MAP6 &&
----------------
Looks some redundant logic. Only X86II::EVEX and X86II::T_MAP6 is special for FP16?


================
Comment at: llvm/lib/Target/X86/X86InstrFMA3Info.cpp:235
+    else
+      Table = makeArrayRef(FP16Groups);
+  }
----------------
Seems we only need FP16Groups be separate table.


================
Comment at: llvm/test/CodeGen/X86/avx512fp16-fma-commute.ll:9
+
+define half @fma_123_f16(half %x, half %y, half %z) {
+; CHECK-LABEL: fma_123_f16:
----------------
The name 123 is not the same with the generated instruction (213sh). Is it expected?


================
Comment at: llvm/test/CodeGen/X86/vec-strict-128-fp16.ll:105
 
+define <8 x half> @f13(<8 x half> %a, <8 x half> %b, <8 x half> %c) #0 {
+; CHECK-LABEL: f13:
----------------
Is it necessary to test 132, 231 version?


================
Comment at: llvm/test/CodeGen/X86/vec-strict-256-fp16.ll:105
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    vfmadd213ph %ymm2, %ymm1, %ymm0
+; CHECK-NEXT:    ret{{[l|q]}}
----------------
Ditto.


================
Comment at: llvm/test/CodeGen/X86/vec-strict-512-fp16.ll:104
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    vfmadd213ph %zmm2, %zmm1, %zmm0
+; CHECK-NEXT:    ret{{[l|q]}}
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105268