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

[ShihPo Hung via Phabricator via llvm-commits]

arcbbb added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVVXRMRegister.cpp:58
+  bool emitWriteVXRM(MachineBasicBlock &MBB);
+  bool hasVXRMInfo(const MachineInstr &MI, unsigned &VXRMInfo) const;
+};
----------------
craig.topper wrote:
> Would it be better to return Optional<unsigned>?
Yes, thanks!


================
Comment at: llvm/lib/Target/RISCV/RISCVVXRMRegister.cpp:116
+        continue;
+      } else if (NewVXRMImm != RISCVVXRndMode::DYN) {
+        BuildMI(MBB, MI, MI.getDebugLoc(), TII->get(RISCV::WriteVXRMImm))
----------------
craig.topper wrote:
> craig.topper wrote:
> > Drop `else` after `continue`
> Does NewVXRMImm being DYN mean to use the value from the last static instruction? That seems fragile since they weren't order in IR.
DYN means to use the saved value by the first static instruction. SwapVXRMImm/SwapVCSRImm is inserted between Entry/Calls/InlineAsms and static instrucitons.

Sorry my wording is misleading.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121376