[PATCH] D127576: [RISCV] Teach vsetvli insertion to handle VLEFF/VLSEGFF.

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Three high level concerns with this patch.

First, you need to rework the patch description.  As written, it sounds like you're trying to fix a functional issue.  To the best of my knowledge, that does not appear to actually be the case.  Instead, this patch is improving precision for the case where VL is modified by a FF instruction.

Second, this change mixes at least three separate optimizations.  I've tagged everything which isn't the transfer rule, please remove them from the initial patch.

Third, we have three places which check for modifiesRegister(VL).  You only update two of them.  Make sure you include the third.



================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:327
+  // Update to the VSETVLIInfo right after MI.
+  void updateToEndStatus(const MachineInstr &MI) {
+    if (RISCV::isFaultFirstLoad(MI)) {
----------------
This should not be a member function on VSETVLIInfo.  Its part of the data flow transfer rule, not the abstract state.  


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:943
+
+      if (RISCV::isFaultFirstLoad(*DefMI)) {
+        uint64_t TSFlags = MI.getDesc().TSFlags;
----------------
This is a separate optimization, move it to its own review.  


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1080
+
+    // We need the PHI input to be the output of a VSET(I)VLI/VLEFF/VLSEGFF
+    // and match the predecessor block.
----------------
This is a separate optimization, move it to its own review.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127576