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

[Zakk Chen via Phabricator via llvm-commits]

khchen added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVVXRMRegister.cpp:1
+//===- RISCVVXRMRegister.cpp - Insert WriteVXRM instructions
+//---------------===//
----------------
Why not call it `RISCVInsertVXRM` similar with `RISCVInsertVSETVLI`? IMPO, this naming would confusing me until I see the comment.


================
Comment at: llvm/lib/Target/RISCV/RISCVVXRMRegister.cpp:108
+        assert(SavedVCSRReg == 0);
+        unsigned NewVCSRImm = NewVXRMImm << 1;
+        SavedVCSRReg = MRI->createVirtualRegister(&RISCV::GPRRegClass);
----------------
sorry, I didn't understand why we need to `<< 1`?

```
enum RoundingMode {
  RNU = 0,
  RNE = 1,
  RDN = 2,
  ROD = 3,
  DYN = 4,
};
```
if `NewVXRMImm` is `RDN` then the `CurVXRMImm` would be `DYN` after `<<1`?


================
Comment at: llvm/test/CodeGen/RISCV/rvv/vaadd-rm-rv32.ll:3
+; RUN: llc -mtriple=riscv32 -mattr=+v -verify-machineinstrs \
+; RUN:   < %s | FileCheck %s
+declare <vscale x 1 x i8> @llvm.riscv.vaadd.rm.nxv1i8.nxv1i8(
----------------
nit: maybe we could merge `vaadd-rm-rv32.ll`and `vaadd-rm-rv64.ll` into one as we usually did in other tests.
ex. https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/RISCV/rvv/vaadd.ll#L1-L5
```
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: sed 's/iXLen/i32/g' %s | llc -mtriple=riscv32 -mattr=+v \
; RUN:   -verify-machineinstrs | FileCheck %s --check-prefixes=CHECK,RV32
; RUN: sed 's/iXLen/i64/g' %s | llc -mtriple=riscv64 -mattr=+v \
; RUN:   -verify-machineinstrs | FileCheck %s --check-prefixes=CHECK,RV64
```

I think it could be in anorther NFC patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121376