[RISCV][InsertVSETVLI] Reverse traversal order of block in post pass [nfc]

[reames]

Summary

This unblocks a following change to be more sophisticated during post pass rewriting.

Review wise, I basically just want a second set of eyes. This change should be straight forward, but since it took me an embarrassing number of attempts to get make check to pass. Let's make sure I'm not missing yet another cornercase.

[craig.topper]

The phrase "block traversal" made me think you were changing the order we were visiting blocks. Which of course makes no sense for the local pass. Maybe "instruction traversal" would be better?

[craig.topper]

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

1244

Why is NextMI relevant to whether this instruction can be removed?

[reames]

In D139877#3989836, @craig.topper wrote:

The phrase "block traversal" made me think you were changing the order we were visiting blocks. Which of course makes no sense for the local pass. Maybe "instruction traversal" would be better?

Does the reworded title clarify?

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

1244

This is one of the generalizations planned.

The basic idea here is we need to merge the demanded fields with the differences between the two vsetvlis to figure out if we can validly rewrite the former. i.e. if tail policy is demanded, but the two differ, we can't rewrite the former, and thus can't remove the later.

The case that's a bit tricky is when the following vsetvli either a) reads VL (via the preserving form), or b) reads a GPR which is defined between the two instructions. The former should be handled by the implicit read when we visit NextMI being modeled in the Used set. The later requires some care, and the current code uses this routine to be conservative around that case.

[craig.topper]

LGTM

[nemanjai]

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

1230

This causes failures with -Werror:

RISCVInsertVSETVLI.cpp:1230:27: error: 'iterator_range' may not intend to support class template argument deduction [-Werror,-Wctad-maybe-unsupported]
  for (MachineInstr &MI : iterator_range(MBB.rbegin(), MBB.rend())) {
                          ^
/home/nemanjai/llvm/Git/trunk1/llvm-project/llvm/include/llvm/ADT/iterator_range.h:30:7: note: add a deduction guide to suppress this warning
class iterator_range {
      ^
1 error generated.

Example: https://lab.llvm.org/buildbot/#/builders/36/builds/28344

[reames]

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

1230

I have no idea what that warning means. Forming an iterator range from two iterators is pretty much the canonical use case. I'm happy to adjust code to remove the warning, but I'll need a hint on how to do so.

[craig.topper]

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

1230

I think you want make_range instead of iterator_range?

[reames]

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

1230

I absolutely do. Wow, how did that even compile, much less pass all the unit tests? Eek.

Fix upcoming shortly.

[reames]

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

1230

Jordan beat me to a fix in 4f9d069, but I went ahead and switched to make_range in 3a020527 for clarity sake.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

if (!PrevMI.getOperand(2).isImm() || !MI.getOperand(2).isImm())
     return false;
 
   auto PriorVType = PrevMI.getOperand(2).getImm();
   auto VType = MI.getOperand(2).getImm();
   return areCompatibleVTYPEs(PriorVType, VType, Used);
 }
 
 void RISCVInsertVSETVLI::doLocalPostpass(MachineBasicBlock &MBB) {
-  MachineInstr *PrevMI = nullptr;
+  MachineInstr *NextMI = nullptr;
+  // We can have arbitrary code in successors, so VL and VTYPE
+  // must be considered demanded.
   DemandedFields Used;
+  Used.VL = true;
+  Used.demandVTYPE();
   SmallVector<MachineInstr*> ToDelete;
-  for (MachineInstr &MI : MBB) {
-    // Note: Must be *before* vsetvli handling to account for config cases
-    // which only change some subfields.
-    doUnion(Used, getDemanded(MI));
+  for (MachineInstr &MI : iterator_range(MBB.rbegin(), MBB.rend())) {
 
-    if (!isVectorConfigInstr(MI))
+    if (!isVectorConfigInstr(MI)) {
+      doUnion(Used, getDemanded(MI));
       continue;
+    }
+
+    Register VRegDef = MI.getOperand(0).getReg();
+    if (VRegDef != RISCV::X0 &&
+        !(VRegDef.isVirtual() && MRI->use_nodbg_empty(VRegDef)))
+      Used.VL = true;
 
-    if (PrevMI) {
+    if (NextMI) {
       if (!Used.VL && !Used.usedVTYPE()) {
-        ToDelete.push_back(PrevMI);
-        // fallthrough
-      } else if (canMutatePriorConfig(*PrevMI, MI, Used)) {
-        PrevMI->getOperand(2).setImm(MI.getOperand(2).getImm());
         ToDelete.push_back(&MI);
-        // Leave PrevMI unchanged
+        // Leave NextMI unchanged
         continue;
+      } else if (canMutatePriorConfig(MI, *NextMI, Used)) {
+        MI.getOperand(2).setImm(NextMI->getOperand(2).getImm());
+        ToDelete.push_back(NextMI);
+        // fallthrough
       }
     }
-    PrevMI = &MI;
+    NextMI = &MI;
     Used = getDemanded(MI);
-    Register VRegDef = MI.getOperand(0).getReg();
-    if (VRegDef != RISCV::X0 &&
-        !(VRegDef.isVirtual() && MRI->use_nodbg_empty(VRegDef)))
-      Used.VL = true;
   }
 
   for (auto *MI : ToDelete)
     MI->eraseFromParent();
 }
 
 void RISCVInsertVSETVLI::insertReadVL(MachineBasicBlock &MBB) {
   for (auto I = MBB.begin(), E = MBB.end(); I != E;) {