[PATCH] D121376: [RISCV][RVV] Introduce roundmode operand to PseudoVAADD instruction

[ShihPo Hung via Phabricator via llvm-commits]

arcbbb added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:639
   }
+  // For Saturating binary operations with mask.
+  // The destination vector type is the same as first source vector.
----------------
frasercrmck wrote:
> Is it worth saying `with mask and rounding-mode operand` to help people differentiate between the intrinsics?
> 
> Also this one is defined after the no-roundmode version, but `RISCVSaturatingBinaryRMAAXNoMask` is defined //before// its counterpart. I find that quite confusing.
Yes, thanks for making it clear!
I'll fix it in the next update.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/roundmode-insert.mir:26
+    dead $x0 = PseudoVSETVLI %2:gprnox0, 69, implicit-def $vl, implicit-def $vtype
+    %3:vr = PseudoVAADD_VV_MF8 %0:vr, %1:vr, 2, $noreg, 3, implicit $vxrm, implicit $vl, implicit $vtype
+    $v8 = COPY %3:vr
----------------
craig.topper wrote:
> arcbbb wrote:
> > craig.topper wrote:
> > > Does it really have an implicit $vxrm use before the RISCVVXRMRegister pass? @efriedma should we only have the vxrm use for the dynamic case?
> > My observation is that `NoImplicit` is false by default in `CreateMachineInstr`, so implicit $vxrm is there since BuildMI.
> Right SelectionDAG will do it from the tablegen definition. For scalar FP I didn't set uses FRM in the td file and then I do a PostProcesseISel to add it for DYN instructions.
> 
> Not sure if it makes sense to do something similar here. The VXRMRegister pass would need to add the implicit def once it added the CSR write.
I am figuring it out and I study the approach in FRM & VSETVLI.
My understanding is that 
1. Remove the VXRM uses from the td
2. Add it for DYN instructions in PostInstrSelection
3. Add it for non-DYN instructions in VXRMRegister pass while inserting the CSR write.
Are my steps the same as yours?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121376