[PATCH] D105265: [X86] AVX512FP16 instructions enabling 3/6

[Pengfei Wang via Phabricator via cfe-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:1996
       setOperationAction(ISD::SCALAR_TO_VECTOR,   MVT::v16f16, Custom);
+      setOperationAction(ISD::SINT_TO_FP,         MVT::v16i16, Legal);
+      setOperationAction(ISD::STRICT_SINT_TO_FP,  MVT::v16i16, Legal);
----------------
LuoYuanke wrote:
> How do we know it covert to v16f16? Is it possible convert to v16f32?
No. Because `v16f32` is not a legal type on X86.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:2054
+      // vcvttph2[u]dq v4f16 -> v4i32/64, v2f16 -> v2i32/64
+      setOperationAction(ISD::FP_TO_SINT,        MVT::v2f16, Custom);
+      setOperationAction(ISD::STRICT_FP_TO_SINT, MVT::v2f16, Custom);
----------------
LuoYuanke wrote:
> Why it is not v2i16?
This is used to customize vector widen, which always check the action of result type.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19998
+  SDLoc dl(Op);
+  SDValue InVec = DAG.getNode(ISD::SCALAR_TO_VECTOR, dl, MVT::v2i64, Src);
+  if (IsStrict) {
----------------
LuoYuanke wrote:
> Should this node be chained to Op.getOperand(0) for strict FP and convert operation be chained to this node?
Not, we just chain FP nodes together.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22003
+      MakeLibCallOptions CallOptions;
+      return makeLibCall(DAG, LC, VT, In, CallOptions, SDLoc(Op)).first;
+    }
----------------
LuoYuanke wrote:
> InChain for strict FP?
Good catch.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22014
+    SDValue Res = DAG.getNode(ISD::CONCAT_VECTORS, DL, MVT::v8f16, In,
+                              DAG.getUNDEF(MVT::v4f16));
+    if (IsStrict)
----------------
LuoYuanke wrote:
> Is there any case for v3f16?
No, v3f16 will be widen to v4f16 first.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31265
+      if (Src.getValueType().getVectorElementType() == MVT::i16)
+        return;
+
----------------
LuoYuanke wrote:
> Where is vXi16 handle? Is it promoted to vXi32 finally?
No. i16 and f16 has the same element size. So we don't need to replace them with custom nodes.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31280
+        unsigned Opc = IsSigned ? X86ISD::CVTSI2P : X86ISD::CVTUI2P;
+        Results.push_back(DAG.getNode(Opc, dl, MVT::v8f16, Src));
+      }
----------------
LuoYuanke wrote:
> Isn't the result type changed to v8f16? Why we don't extract sub-vector here?
Yes. The common widen code widen both src and dst elements as the same size. We are customizing to different size here so than we can always select the 128 bit instructions. Result type larger than 128 bits doesn't have this problem.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:49327
+  // UINT_TO_FP(vXi33~63) -> UINT_TO_FP(ZEXT(vXi33~63 to vXi64))
+  if (InVT.isVector() && VT.getVectorElementType() == MVT::f16) {
+    unsigned ScalarSize = InVT.getScalarSizeInBits();
----------------
LuoYuanke wrote:
> Need to check Subtarget.hasFP16() ?
No. We can't go to here without feature FP16 enabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105265