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

Philip Reames via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 29 11:29:12 PDT 2023

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?

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list