[PATCH] D120449: [RISCV][RVV] Add strict vfcvt intrinsics that have side effects for dynamically-set rounding mode

[Zakk Chen via Phabricator via llvm-commits]

khchen added a comment.

Makes sense to me, but I'd appreciate someone else for a final LGTM.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4683
+    Ops.push_back(VL);
+    Ops.push_back(DAG.getUNDEF(XLenVT)); // Policy
+  }
----------------
arcbbb wrote:
> kito-cheng wrote:
> > Is this operand for tail policy? if so why this is `UNDEF`? I guess this should be `TAIL_AGNOSTIC` rather than `UNDEF`?
> Because unmasked pseudos doesn't have a policy operand,
> TA/TU is distinguished by checking if passthru is undef. I think it is proper to leave undef here.
nit: maybe we could add a comment for that, because it must be `UNDEF` to match unmaksed pattern successfully.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4678
+    MVT MaskVT = MVT::getVectorVT(MVT::i1, VT.getVectorElementCount());
+    SDValue Mask = DAG.getNode(RISCVISD::VMSET_VL, DL, MaskVT, VL);
+    Ops.push_back(Mask);
----------------
I think `TrueMask` maybe better.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td:619
+                                      VReg result_reg_class,
+                                      VReg op2_reg_class
+                                      > {
----------------
--> `VReg op2_reg_class> {`


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td:649
+    defvar ivti = GetIntVTypeInfo<fvti>.Vti;
+
+    defm : VPatConvertStrictSDNode_VL<vop, instruction, "V",
----------------
This is additional newline comparing to `VPatConvertI2FPSDNode_V_VL_STRICT`? or we need a newline in `VPatConvertI2FPSDNode_V_VL_STRICT`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120449