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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.
Herald added a subscriber: StephenFan.


================
Comment at: llvm/lib/Target/RISCV/RISCVVXRMRegister.cpp:13
+//
+// The pass consists of a single pass over each basici block looking for VXRM
+// usage that requires a WriteVXRM to be inserted.
----------------
basici->basic


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


================
Comment at: llvm/lib/Target/RISCV/RISCVVXRMRegister.cpp:116
+        continue;
+      } else if (NewVXRMImm != RISCVVXRndMode::DYN) {
+        BuildMI(MBB, MI, MI.getDebugLoc(), TII->get(RISCV::WriteVXRMImm))
----------------
Drop `else` after `continue`


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


================
Comment at: llvm/lib/Target/RISCV/RISCVVXRMRegister.cpp:140
+  if (SavedVXRMReg) {
+    MachineInstr &MI = MBB.back();
+    if (MI.isTerminator()) {
----------------
Machine basic blocks can have more than 1 terminator. You probably need to do this in the loop when you encounter the first terminator.


================
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
----------------
Does it really have an implicit $vxrm use before the RISCVVXRMRegister pass? @efriedma should we only have the vxrm use for the dynamic case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121376