[PATCH] D143872: [X86][FP16] Combine two steps conversions into direct conversion

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:56704
+    // into (v8f16 vector_shuffle(v8f16 (CVTXI2P v4i64), ..))
+    if (NumElts == 8 && Src.getOpcode() == ISD::CONCAT_VECTORS) {
+      SDValue Cvt0, Cvt1;
----------------
Should we check the number of operands of concat be 2?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:56717
+        assert(IsOp0Strict && "Op0 must be strict node");
+        unsigned Opc = Op0.getOpcode() == ISD::STRICT_SINT_TO_FP
+                           ? X86ISD::STRICT_CVTSI2P
----------------
Not sure we can transform the node for strict fp operation.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:56719
+                           ? X86ISD::STRICT_CVTSI2P
+                           : X86ISD::STRICT_CVTUI2P;
+        Cvt0 = DAG.getNode(Opc, dl, {MVT::v8f16, MVT::Other},
----------------
Should we make sure opc is a xint_to_fp node, otherwise return `SDValue()`?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:56728
+      unsigned Opc = Op0.getOpcode() == ISD::SINT_TO_FP ? X86ISD::CVTSI2P
+                                                        : X86ISD::CVTUI2P;
+      Cvt0 = DAG.getNode(Opc, dl, MVT::v8f16, Op0.getOperand(0));
----------------
Ditto.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:56736
 
-  SDLoc dl(N);
+  if (NumElts == 1 || !isPowerOf2_32(NumElts))
+    return SDValue();
----------------
Why move the check after line 56701?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143872