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