[RISCV] simplify emitVSETVLIs handling of vsetvli xN, phi(), vtype case [NFC]

[reames]

Summary

This is possibly somewhat subjective, but having an explicitly named flag to track the property required and code structure that more closely matches phase 1/2 of the dataflow seems much easier to read.

[craig.topper]

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

1111–1112

Why is this one true? If we insert a vsetvli in the body of the above if, don't we need to not use needVSETVLIPHI in the future?

[frasercrmck]

I think the simplification is good and I'd like to see something like this. Starting off with predecessor info is logical.

I'm not sure about the name PrefixTransparent though - I think that variable needs explanation (in a comment) because, personally, the name doesn't do it on its own.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

1111–1112

I was also wondering that.

[reames]

Fix bug noted by reviewers, and add requested comment.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

1111–1112

Because that's a flat out bug in the patch. Oops!

Fixed in the new version.

[reames]

Rebase, adjust a comment to be less confusing, and ping @frasercrmck

[frasercrmck]

I was wondering if this would be better represented as Optional<VSETVLIInfo> LocalBBInfo initialized to None before traversing the block. We'd update it along with CurInfo (LocalBBInfo = CurInfo). Then we'd know that if LocalBBInfo has been set there have been block-local updates to VL/VTYPE and thus it's not safe to look through PHIs. At least I'd find that a little to grok. It's subjective, after all.

So, LGTM.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

1079

changed?

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

bool RISCVInsertVSETVLI::needVSETVLIPHI(const VSETVLIInfo &Require,
   }
 
   // If all the incoming values to the PHI checked out, we don't need
   // to insert a VSETVLI.
   return false;
 }
 
 void RISCVInsertVSETVLI::emitVSETVLIs(MachineBasicBlock &MBB) {
-  VSETVLIInfo CurInfo;
+  VSETVLIInfo CurInfo = BlockInfo[MBB.getNumber()].Pred;
+  // Track whether the prefix of the block we've scanned is transparent
+  // (meaning has not yet changed the abstract state).
+  bool PrefixTransparent = true;
   for (MachineInstr &MI : MBB) {
     // If this is an explicit VSETVLI or VSETIVLI, update our state.
     if (isVectorConfigInstr(MI)) {
       // Conservatively, mark the VL and VTYPE as live.
       assert(MI.getOperand(3).getReg() == RISCV::VL &&
              MI.getOperand(4).getReg() == RISCV::VTYPE &&
              "Unexpected operands where VL and VTYPE should be");
       MI.getOperand(3).setIsDead(false);
       MI.getOperand(4).setIsDead(false);
       CurInfo = getInfoForVSETVLI(MI);
+      PrefixTransparent = false;
       continue;
     }
 
     uint64_t TSFlags = MI.getDesc().TSFlags;
     if (RISCVII::hasSEWOp(TSFlags)) {
       VSETVLIInfo NewInfo = computeInfoForInstr(MI, TSFlags, MRI);
-      if (!CurInfo.isValid()) {
-        // We haven't found any vector instructions or VL/VTYPE changes yet,
-        // use the predecessor information.
-        CurInfo = BlockInfo[MBB.getNumber()].Pred;
-        assert(CurInfo.isValid() && "Expected a valid predecessor state.");
-        if (needVSETVLI(MI, NewInfo, CurInfo)) {
-          // If this is the first implicit state change, and the state change
-          // requested can be proven to produce the same register contents, we
-          // can skip emitting the actual state change and continue as if we
-          // had since we know the GPR result of the implicit state change
-          // wouldn't be used and VL/VTYPE registers are correct.  Note that
-          // we *do* need to model the state as if it changed as while the
-          // register contents are unchanged, the abstract model can change.
-          if (needVSETVLIPHI(NewInfo, MBB))
-            insertVSETVLI(MBB, MI, NewInfo, CurInfo);
-          CurInfo = NewInfo;
-        }
-      } else {
-        // If this instruction isn't compatible with the previous VL/VTYPE
-        // we need to insert a VSETVLI.
-        // NOTE: We can't use predecessor information for the store. We must
-        // treat it the same as the first phase so that we produce the correct
-        // vl/vtype for succesor blocks.
-        if (needVSETVLI(MI, NewInfo, CurInfo)) {
+
+      // If this instruction isn't compatible with the previous VL/VTYPE
+      // we need to update the abstract state, and possibly insert a VSETVLI.
+        if (!PrefixTransparent || needVSETVLIPHI(NewInfo, MBB))
           insertVSETVLI(MBB, MI, NewInfo, CurInfo);
-          CurInfo = NewInfo;
-        }
-      }
+        PrefixTransparent = false;
 
       if (RISCVII::hasVLOp(TSFlags)) {
         MachineOperand &VLOp = MI.getOperand(getVLOpNum(MI));
         if (VLOp.isReg()) {
           // Erase the AVL operand from the instruction.
           VLOp.setReg(RISCV::NoRegister);
           VLOp.setIsKill(false);
         }
         MI.addOperand(MachineOperand::CreateReg(RISCV::VL, /*isDef*/ false,
                                                 /*isImp*/ true));
       }
       MI.addOperand(MachineOperand::CreateReg(RISCV::VTYPE, /*isDef*/ false,
                                               /*isImp*/ true));
     }
 
     // If this is something that updates VL/VTYPE that we don't know about, set
     // the state to unknown.
     if (MI.isCall() || MI.isInlineAsm() || MI.modifiesRegister(RISCV::VL) ||
         MI.modifiesRegister(RISCV::VTYPE)) {
       CurInfo = VSETVLIInfo::getUnknown();
+      PrefixTransparent = false;
     }
   }
 
   // If we reach the end of the block and our current info doesn't match the
   // expected info, insert a vsetvli to correct.
   if (!UseStrictAsserts) {
     const VSETVLIInfo &ExitInfo = BlockInfo[MBB.getNumber()].Exit;
     if (CurInfo.isValid() && ExitInfo.isValid() && !ExitInfo.isUnknown() &&