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

[Phoebe Wang via Phabricator via llvm-commits]

pengfei marked an inline comment as done.
pengfei 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;
----------------
LuoYuanke wrote:
> Should we check the number of operands of concat be 2?
concat has at least 2 operands. Given we have checked the dest is `v8f16` and both source come from `v4i64`, I think we don't need to check it.


================
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
----------------
LuoYuanke wrote:
> Not sure we can transform the node for strict fp operation.
The transform is strict fp compliance. There are totally two different exceptions in this case: precision and overflow. A value out of range of f16 will set both exceptions. A precision exception in i64->f32 always means the value out of range of f16. So we won't discard of introduce any exceptions.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:56719
+                           ? X86ISD::STRICT_CVTSI2P
+                           : X86ISD::STRICT_CVTUI2P;
+        Cvt0 = DAG.getNode(Opc, dl, {MVT::v8f16, MVT::Other},
----------------
LuoYuanke wrote:
> Should we make sure opc is a xint_to_fp node, otherwise return `SDValue()`?
We make sure it's xint_to_fp because we have checked the result type is f32 and input type is i64.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:56736
 
-  SDLoc dl(N);
+  if (NumElts == 1 || !isPowerOf2_32(NumElts))
+    return SDValue();
----------------
LuoYuanke wrote:
> Why move the check after line 56701?
Reuse the code in 56693 line  to check src and dst type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143872