Teach PeepholeOpt to eliminate redundant copy from constant physreg (e.g VLENB on RISCV)

[reames]

Summary

The existing redundant copy elimination required a virtual register source, but the same logic works for any physreg where we don't have to worry about clobbers.

(Aside: The code can be pretty easily generalized to handle the clobber case too, but that'll be a separate patch.)

On RISCV, this helps eliminate redundant CSR reads from VLENB.

[craig.topper]

This looks ok to me, but I'd like to understand some things about the larger roadmap here.

If SelectionDAG CSEd the read_register this test case wouldn't need this patch. The peephole pass code here seems to be restricted to a single basic block, same as SelectionDAG. read_register isn't CSEd in SelectionDAG because read_register and read_volatile_register use the same code and have a chain dependency.

The main places we create PseudoVLENB are from ISD::VSCALE lowering and frame lowering.

For ISD::VSCALE lowering we go through RISCVISD::READ_VLENB. Do you plan to replace with RISCVISD::READ_VLENB with ISD::READ_REGISTER? Or make RISCVISD::READ_VLENB isel to a COPY? Or maybe use ISD::CopyFromReg? RISCVISD::READ_VLENB and ISD::CopyFromReg should CSE as long as the chain input is EntryNode and there is no Glue.

For the frame lowering case, that happens after regalloc so this patch doesn't help with that.

Do you have other cases that this patch helps that wouldn't already be covered by SelectionDAG CSE?

[reames]

In D125564#3513834, @craig.topper wrote:

This looks ok to me, but I'd like to understand some things about the larger roadmap here.

If SelectionDAG CSEd the read_register this test case wouldn't need this patch. The peephole pass code here seems to be restricted to a single basic block, same as SelectionDAG. read_register isn't CSEd in SelectionDAG because read_register and read_volatile_register use the same code and have a chain dependency.

The main places we create PseudoVLENB are from ISD::VSCALE lowering and frame lowering.

For ISD::VSCALE lowering we go through RISCVISD::READ_VLENB. Do you plan to replace with RISCVISD::READ_VLENB with ISD::READ_REGISTER? Or make RISCVISD::READ_VLENB isel to a COPY? Or maybe use ISD::CopyFromReg? RISCVISD::READ_VLENB and ISD::CopyFromReg should CSE as long as the chain input is EntryNode and there is no Glue.

For the frame lowering case, that happens after regalloc so this patch doesn't help with that.

Do you have other cases that this patch helps that wouldn't already be covered by SelectionDAG CSE?

Ok, two level response here.

First, this is a generic improvement to target independent code. We have the notion of constant physical registers. Regardless of whether there are practice benefits on the particular example constant register, having the generic infrastructure handle more cases (at near zero cost) seems worthwhile on it's own.

Your right that we should also be able to get this case in SelectionDAG. This was already on my punch list (https://github.com/preames/public-notes/blob/master/llvm-riscv-status.rst#optimizations-for-constant-physregs-vlenb-x0), and I don't see any reason to only implement it in one place.

Second, you raise the question of whether this will be useful in practice on RISCV for VLENB. The honest answer is I don't know. I want to be able to replace PsuedoReadVLENB the whole way thought, but whether that will end up being more than a very minor clean is non obvious.

What got me looking at this was the recent change to PsuedoReadVL which we've turned into something much more than just a register read. If we can use COPY as canonical form for VTYPE, I was going to prototype splitting the PsueodReadVL node into component pieces. I haven't done that yet. I hit too many independent "we should fix that"s and didn't bother until the general infrastructure for constant physical registers had been fleshed out a bit more. (To prevent confusion, VL is *not* constant.)

[craig.topper]

In D125564#3517184, @reames wrote:

In D125564#3513834, @craig.topper wrote:

This looks ok to me, but I'd like to understand some things about the larger roadmap here.

If SelectionDAG CSEd the read_register this test case wouldn't need this patch. The peephole pass code here seems to be restricted to a single basic block, same as SelectionDAG. read_register isn't CSEd in SelectionDAG because read_register and read_volatile_register use the same code and have a chain dependency.

The main places we create PseudoVLENB are from ISD::VSCALE lowering and frame lowering.

For ISD::VSCALE lowering we go through RISCVISD::READ_VLENB. Do you plan to replace with RISCVISD::READ_VLENB with ISD::READ_REGISTER? Or make RISCVISD::READ_VLENB isel to a COPY? Or maybe use ISD::CopyFromReg? RISCVISD::READ_VLENB and ISD::CopyFromReg should CSE as long as the chain input is EntryNode and there is no Glue.

For the frame lowering case, that happens after regalloc so this patch doesn't help with that.

Do you have other cases that this patch helps that wouldn't already be covered by SelectionDAG CSE?

Ok, two level response here.

First, this is a generic improvement to target independent code. We have the notion of constant physical registers. Regardless of whether there are practice benefits on the particular example constant register, having the generic infrastructure handle more cases (at near zero cost) seems worthwhile on it's own.

Yeah I'm not opposed to this. I guess part of my concern was that this becomes untested if we fix SelectionDAG CSE. Maybe we should have a directed MIR test?

llvm/lib/CodeGen/PeepholeOptimizer.cpp

private:
     /// Finds recurrence cycles, but only ones that formulated around
     /// a def operand and a use operand that are tied. If there is a use
     /// operand commutable with the tied use operand, find recurrence cycle
     /// along that operand as well.
     bool findTargetRecurrence(Register Reg,
                               const SmallSet<Register, 2> &TargetReg,
                               RecurrenceCycle &RC);
 
-    /// If copy instruction \p MI is a virtual register copy, track it in
-    /// the set \p CopyMIs. If this virtual register was previously seen as a
+    /// If copy instruction \p MI is a virtual register copy or a copy of a
+    /// constant physical register to a virtual register, track it in the
+    /// set \p CopyMIs. If this virtual register was previously seen as a
     /// copy, replace the uses of this copy with the previously seen copy's
     /// destination register.
     bool foldRedundantCopy(MachineInstr &MI,
                            DenseMap<RegSubRegPair, MachineInstr *> &CopyMIs);
 
     /// Is the register \p Reg a non-allocatable physical register?
     bool isNAPhysCopy(Register Reg);
 

 //
 // Should replace %2 uses with %1:sub1
 bool PeepholeOptimizer::foldRedundantCopy(
     MachineInstr &MI, DenseMap<RegSubRegPair, MachineInstr *> &CopyMIs) {
   assert(MI.isCopy() && "expected a COPY machine instruction");
 
   Register SrcReg = MI.getOperand(1).getReg();
   unsigned SrcSubReg = MI.getOperand(1).getSubReg();
-  if (!SrcReg.isVirtual())
+  if (!SrcReg.isVirtual() && !MRI->isConstantPhysReg(SrcReg))
     return false;
 
   Register DstReg = MI.getOperand(0).getReg();
   if (!DstReg.isVirtual())
     return false;
 
   RegSubRegPair SrcPair(SrcReg, SrcSubReg);
 
for (MachineBasicBlock &MBB : MF) {
 
     // Track when a non-allocatable physical register is copied to a virtual
     // register so that useless moves can be removed.
     //
     // $physreg is the map index; MI is the last valid `%vreg = COPY $physreg`
     // without any intervening re-definition of $physreg.
     DenseMap<Register, MachineInstr *> NAPhysToVirtMIs;
 
-    // Set of pairs of virtual registers and their subregs that are copied
-    // from.
+    // Set of copies to virtual registers keyed by source register.  Never
+    // holds any physreg which requires def tracking.
     DenseMap<RegSubRegPair, MachineInstr *> CopySrcMIs;
 
     bool IsLoopHeader = MLI->isLoopHeader(&MBB);
 
     for (MachineBasicBlock::iterator MII = MBB.begin(), MIE = MBB.end();
          MII != MIE; ) {
       MachineInstr *MI = &*MII;
       // We may be erasing MI below, increment MII now.

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

 }
 
 bool RISCVRegisterInfo::isAsmClobberable(const MachineFunction &MF,
                                          MCRegister PhysReg) const {
   return !MF.getSubtarget<RISCVSubtarget>().isRegisterReservedByUser(PhysReg);
 }
 
 bool RISCVRegisterInfo::isConstantPhysReg(MCRegister PhysReg) const {
-  return PhysReg == RISCV::X0;
+  return PhysReg == RISCV::X0 || PhysReg == RISCV::VLENB;
 }
 
 const uint32_t *RISCVRegisterInfo::getNoPreservedMask() const {
   return CSR_NoRegs_RegMask;
 }
 
 // Frame indexes representing locations of CSRs which are given a fixed location
 // by save/restore libcalls.

llvm/test/CodeGen/RISCV/vlenb.ll

entry:
   call i32 @llvm.read_register.i32(metadata !0)
   ret void
 }
 
 define i32 @simple_cse() {
 ; CHECK-LABEL: simple_cse:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    csrr a0, vlenb
-; CHECK-NEXT:    csrr a1, vlenb
-; CHECK-NEXT:    sub a0, a0, a1
+; CHECK-NEXT:    sub a0, a0, a0
 ; CHECK-NEXT:    ret
 entry:
   %v1 = call i32 @llvm.read_register.i32(metadata !0)
   %v2 = call i32 @llvm.read_register.i32(metadata !0)
   %sub = sub i32 %v1, %v2
   ret i32 %sub
 }