[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: sunshaoce.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9337
+    bool HasPolicy = RISCVII::hasVecPolicyOp(TSFlags);
+    unsigned Idx = MI.getNumExplicitOperands() - HasPolicy - 3;
+    const MachineOperand &RoundModeOp = MI.getOperand(Idx);
----------------
`- 3` because we assume SEW and VL exist? Can we assert hasSEW and hasVL?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:587
 def FRM    : RISCVReg<0, "frm">;
+def VCSR   : RISCVReg<0, "vcsr">;
 
----------------
Do we need this? There's no way to express that VXRM and VXSAT overlap with it so we just always reference the individual pieces.


================
Comment at: llvm/lib/Target/RISCV/RISCVVXRMRegister.cpp:94
+      MachineOperand &RoundModeOp = MI.getOperand(Idx.getValue());
+      unsigned NewVXRMImm = RoundModeOp.getImm() & 0x7;
+      if (NewVXRMImm == CurVXRMImm)
----------------
When would it be greater than 7? And if that can happen then what prevents values of 5-7 which aren't defined.


================
Comment at: llvm/lib/Target/RISCV/RISCVVXRMRegister.cpp:133
+
+    // Here handles Calls, Terminator, InlineAsm, and all ops
+    // which depend on saved VXRM
----------------
This isn't handling terminators is it?


================
Comment at: llvm/lib/Target/RISCV/RISCVVXRMRegister.cpp:144
+
+  // Restore VXRM to previous saved value before leaving the block
+  if (SavedVCSRReg) {
----------------
VXRM -> VCSR?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121376