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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Can you precommit the test so that the deltas are visible?

General structure makes sense, minor style comments only.



================
Comment at: llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp:12
+//
+// To work with the intrinsics that have SideEffects, it checks if there are
+// any VXRM uses in the given MachineFunction.
----------------
This comment confuses me, and doesn't seem to correspond to the code.  Any chance this is stale?


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp:59
+
+  bool isValid() const { return State != Uninitialized; }
+  void setUnknown() { State = Unknown; }
----------------
Minor: isInitialized


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp:79
+      return !Other.isValid();
+    if (!Other.isValid())
+      return !isValid();
----------------
I believe this can be written as:

```
if (!isValid() || !Other.isValid())
  return isValid() == Other.isValid();
```

Same with the unknown check just below.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp:105
+    // If either is unknown, the result is unknown.
+    if (isUnknown() || Other.isUnknown())
+      return VXRMInfo::getUnknown();
----------------
You can simplify this as:

```
if (*this == Other)
      return *this;
return VXMMInfo::getUnknown();
```

In particular, you don't need to separately check for unknown.  


================
Comment at: llvm/test/CodeGen/RISCV/rvv/vxrm-insert.ll:214
+; CHECK-NEXT:  # %bb.1: # %trueblock
+; CHECK-NEXT:    vsetvli zero, a0, e8, mf8, ta, ma
+; CHECK-NEXT:    csrwi vxrm, 0
----------------
Not related to this change, but shouldn't we be hoisting these two instructions which are common on both blocks?  Or is this transform running too late for that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158913