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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
