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

[LuoYuanke via Phabricator via cfe-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:1955
       setOperationAction(ISD::SCALAR_TO_VECTOR,       MVT::v32f16, Custom);
+      setOperationAction(ISD::SINT_TO_FP,             MVT::v32i16, Legal);
+      setOperationAction(ISD::STRICT_SINT_TO_FP,      MVT::v32i16, Legal);
----------------
Sorry, I'm just confused on why the type is the same for ISD::SINT_TO_FP and ISD::FP_TO_SINT? The legalization use src type for ISD::SINT_TO_FP and dst type for ISD::FP_TO_SINT? Why not unify to dst type.


================
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);
----------------
How do we know it covert to v16f16? Is it possible convert to v16f32?


================
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);
----------------
Why it is not v2i16?


================
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) {
----------------
Should this node be chained to Op.getOperand(0) for strict FP and convert operation be chained to this node?


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


================
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)
----------------
Is there any case for v3f16?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31087
+        // Now widen to 128 bits.
+        unsigned NumConcats = 128 / TmpVT.getSizeInBits();
+        MVT ConcatVT = MVT::getVectorVT(EleVT.getSimpleVT(), 8 * NumConcats);
----------------
Is it possible the type is i3/i5/i7?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31265
+      if (Src.getValueType().getVectorElementType() == MVT::i16)
+        return;
+
----------------
Where is vXi16 handle? Is it promoted to vXi32 finally?


================
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));
+      }
----------------
Isn't the result type changed to v8f16? Why we don't extract sub-vector here?


================
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();
----------------
Need to check Subtarget.hasFP16() ?


================
Comment at: llvm/lib/Target/X86/X86InstrAVX512.td:8194
+  }
+  def : InstAlias<OpcodeStr#"x\t{$src, $dst|$dst, $src}",
+                  (!cast<Instruction>(NAME # "Z128rr") VR128X:$dst,
----------------
What is the alias used for? Can't it be distinguished from operand?


================
Comment at: llvm/lib/Target/X86/X86InstrAVX512.td:8193
+    defm Z128 : avx512_vcvt_fp<opc, OpcodeStr, v8f16x_info, v2f64x_info, null_frag,
+                               null_frag, sched.XMM, "{1to2}", "{x}", f128mem,
+                               VK2WM>, EVEX_V128;
----------------
Why null_frag instead of X86vfpround? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105265