[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:51
+
+static __inline__ __m256h __DEFAULT_FN_ATTRS256 _mm256_undefined_ph() {
+  return (__m256h)__builtin_ia32_undef256();
----------------
I think this should be `_mm256_undefined_ph(void)`



================
Comment at: clang/lib/Headers/avx512fp16intrin.h:61
+
+static __inline__ __m128h __DEFAULT_FN_ATTRS128 _mm_undefined_ph() {
+  return (__m128h)__builtin_ia32_undef128();
----------------
I think this should be `_mm_undefined_ph(void)`


================
Comment at: clang/lib/Headers/avx512fp16intrin.h:65
+
+static __inline__ __m512h __DEFAULT_FN_ATTRS512 _mm512_undefined_ph() {
+  return (__m512h)__builtin_ia32_undef512();
----------------
I think this should be `_mm512_undefined_ph(void)`


================
Comment at: clang/test/CodeGen/X86/avx512fp16-complex.c:1
+// RUN: %clang_cc1 %s -O0 -fno-experimental-new-pass-manager -emit-llvm -triple x86_64-unknown-unknown -target-feature +avx512fp16 -o - | FileCheck %s --check-prefix=X86
+
----------------
Can we split _Complex out of this patch? This affects other targets that have _Float16 right? So probably needs a different set of reviewers.


================
Comment at: clang/test/Sema/Float16.c:13
 #ifdef HAVE
-// FIXME: Should this be valid?
-_Complex _Float16 a; // expected-error {{'_Complex _Float16' is invalid}}
+// FIXME: Should this be invalid?
+_Complex _Float16 a;
----------------
It's odd to change behavior and then have a FIXME asking if the old behavior was correct.


================
Comment at: llvm/lib/Support/X86TargetParser.cpp:204
 constexpr FeatureBitset FeaturesSapphireRapids =
-    FeaturesICLServer | FeatureAMX_TILE | FeatureAMX_INT8 | FeatureAMX_BF16 |
-    FeatureAVX512BF16 | FeatureAVX512VP2INTERSECT | FeatureCLDEMOTE |
-    FeatureENQCMD | FeatureMOVDIR64B | FeatureMOVDIRI | FeaturePTWRITE |
-    FeatureSERIALIZE | FeatureSHSTK | FeatureTSXLDTRK | FeatureUINTR |
-    FeatureWAITPKG | FeatureAVXVNNI;
+    FeatureAVX512FP16 | FeaturesICLServer | FeatureAMX_TILE | FeatureAMX_INT8 |
+    FeatureAMX_BF16 | FeatureAVX512BF16 | FeatureAVX512VP2INTERSECT |
----------------
I think FeaturesICLServer should still be at the beginning of the list. FeatureAVX512FP16 should be alphabetized with the other AVX512 features. Looks like FeatureAVXNNI was already incorrectly alphabetized.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:18961
 
     // SHUFPS the element to the lowest double word, then movss.
+    SmallVector<int, 8> Mask(VecVT.getVectorNumElements(), -1);
----------------
I think this comment should mention movsh now.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19110
 
   // This will be just movd/movq/movss/movsd.
   if (IdxVal == 0 && ISD::isBuildVectorAllZeros(N0.getNode())) {
----------------
movsh


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:23042
          Op0.getSimpleValueType().is512BitVector())) {
-      assert(VT.getVectorNumElements() <= 16);
+      assert(VT.getVectorNumElements() <= 16 || Subtarget.hasFP16());
       Opc = IsStrict ? X86ISD::STRICT_CMPM : X86ISD::CMPM;
----------------
This should probably include EltVT==MVT::f16 for the FP16 override?


================
Comment at: llvm/lib/Target/X86/X86InstrFragmentsSIMD.td:410
 def X86Movsldup : SDNode<"X86ISD::MOVSLDUP", SDTShuff1Op>;
+def X86Movsh : SDNode<"X86ISD::MOVSH",
+                      SDTypeProfile<1, 2, [SDTCisVT<0, v8f16>,
----------------
Add a blank line above this to match the original formatting


================
Comment at: llvm/lib/Target/X86/X86InstrFragmentsSIMD.td:996
 
+def fp16imm0 : PatLeaf<(f16 fpimm), [{
+  return N->isExactlyValue(+0.0);
----------------
This should be with fp32imm0 and friends.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105263