[PATCH] D105267: [X86] AVX512FP16 instructions enabling 4/6

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:1920
+      setOperationAction(ISD::STRICT_FTRUNC,      VT, Legal);
+      setOperationAction(ISD::FRINT,              VT, Legal);
+      setOperationAction(ISD::STRICT_FRINT,       VT, Legal);
----------------
Does this node means "round to int"? What's the difference to "FNEARBYINT"?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.h:290
 
+    // AVX-512-FP16 scalar reciprocal approximations
+    FRSQRTS,
----------------
Move the code to line 283, so that it is adjacent to FRSQRT and FRCP?


================
Comment at: llvm/lib/Target/X86/X86InstrAVX512.td:9279
 /// avx512_fp14_s rcp14ss, rcp14sd, rsqrt14ss, rsqrt14sd
 multiclass avx512_fp14_s<bits<8> opc, string OpcodeStr, SDNode OpNode,
+                         X86FoldableSchedWrite sched, X86VectorVTInfo _,
----------------
The name is not precise now. We now support non-fp14 node. Also update the comments.


================
Comment at: llvm/lib/Target/X86/X86InstrAVX512.td:9484
+  defm PHZ : avx512_fp28_p<opc, OpcodeStr#"ph", v32f16_info, OpNode, sched.ZMM>,
+              avx512_fp28_p_sae<opc, OpcodeStr#"ph", v32f16_info, OpNodeSAE, sched.ZMM>,
+              T_MAP6PD, EVEX_V512, EVEX_CD8<16, CD8VF>;
----------------
indent.


================
Comment at: llvm/lib/Target/X86/X86InstrAVX512.td:13476
+
+multiclass avx512_fp16_p_vl_all<bits<8> opc, string OpcodeStr, SDNode OpNode,
+                                   X86SchedWriteWidths sched> {
----------------
Why not merge this class to avx512_fp14_p_vl_all? Is it because it doesn't use MXCSR?


================
Comment at: llvm/lib/Target/X86/X86InstrAVX512.td:13477
+multiclass avx512_fp16_p_vl_all<bits<8> opc, string OpcodeStr, SDNode OpNode,
+                                   X86SchedWriteWidths sched> {
+  let Predicates = [HasFP16] in
----------------
indent.


================
Comment at: llvm/lib/Target/X86/X86InstrFoldTables.cpp:3037
   { X86::VSQRTSDr_Int,             X86::VSQRTSDm_Int,             TB_NO_REVERSE },
+  { X86::VSQRTSHZr,                X86::VSQRTSHZm,                0 },
+  { X86::VSQRTSHZr_Int,            X86::VSQRTSHZm_Int,            TB_NO_REVERSE },
----------------
Why no TB_NO_REVERSE for it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105267