[PATCH] D130817: [X86][FP16] Fix vector_shuffle and lowering without f16c feature problems

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:16083
 
-  if (NumV2Elements == 0) {
-    // Check for being able to broadcast a single element.
-    if (SDValue Broadcast = lowerShuffleAsBroadcast(DL, MVT::v8f16, V1, V2,
-                                                    Mask, Subtarget, DAG))
-      return Broadcast;
+  if (Subtarget.hasFP16()) {
+    if (NumV2Elements == 0) {
----------------
LuoYuanke wrote:
> It seems the condition can be refined to hasAVX(), or we can add `hasAVX()` check in `lowerShuffleAsBroadcast`.
> https://www.felixcloutier.com/x86/vbroadcast.html
No, we can't. We don't have a 16-bit broadcast until `AVX512BW`. We don't need extra work to leverage it since we have turn the shuffle to `v8i16`.


================
Comment at: llvm/test/CodeGen/X86/vector-half-conversions.ll:29
-; ALL:       # %bb.0:
-; ALL-NEXT:    vcvtph2ps %xmm0, %xmm0
-; ALL-NEXT:    retq
----------------
RKSimon wrote:
> I'm confused as to under what cases we can't use vcvtph2ps for half -> float given all half values are exactly representable by float?
Not sure if I understand you question correctly. `vcvtph2ps` requires `F16C`, which is independent of `AVX` and `AVX2`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130817