[PATCH] D158913: [RISCV] Add a cross basic block VXRM write insertion pass.

[Yueh-Ting (eop) Chen via Phabricator via llvm-commits]

eopXD added a comment.

The heuristic overall makes sense to me. Maybe add test coverage for nested loops too?



================
Comment at: llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp:21
+// Phase 2 propogates the round mode state to successor blocks,
+//         and hoists the round mode if the folowing conditions meet:
+//           1. The basic block is a loop header.
----------------
folowing -> following

if one of the following conditions meet?


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp:56
+
+  static VXRMInfo getUnknown() {
+    VXRMInfo Info;
----------------
Can we have a static singleton of `VXRMInfo` whose `State` is `Unknown`?


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp:69
+  void setVXRMImm(unsigned Imm) {
+    VXRMImm = Imm;
+    State = Static;
----------------
Add assertion?
```
assert(0 <= Imm && Imm <= 3 && "vxrm value range is [0, 3]");
```


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp:126
+  /// Implement operator<<.
+  /// @{
+  void print(raw_ostream &OS) const {
----------------
Is this extra line that was copied from elsewhere?


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp:157
+
+  // The VXRMInfo that represents the VXRM settings from all predecessor
+  // blocks. Calculated in Phase 2, and used by Phase 3.
----------------
Describing this as "intersect" of all predecessors might be more descriptive.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp:207
+  bool NeedVXRMChange = false;
+  BlockData &BBInfo = BlockInfo[MBB.getNumber()];
+  CurInfo = BBInfo.Pred;
----------------
const


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp:211
+  for (const MachineInstr &MI : MBB) {
+    int VXRMIdx = RISCVII::getVXRMOpNum(MI.getDesc());
+    if (VXRMIdx >= 0) {
----------------
const


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp:213
+    if (VXRMIdx >= 0) {
+      unsigned NewVXRMImm = MI.getOperand(VXRMIdx).getImm() & 7;
+      NeedVXRMChange = true;
----------------
const


================
Comment at: llvm/test/CodeGen/RISCV/rvv/vxrm-insert.ll:144
+; Test same vxrm with asm in between which may invalidate vxrm.
+define <vscale x 1 x i8> @test4(<vscale x 1 x i8> %0, <vscale x 1 x i8> %1, <vscale x 1 x i8> %2, iXLen %3) nounwind {
+; CHECK-LABEL: test4:
----------------
Nit: Maybe more comprehensive test function names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158913