[AMDGPU][ScheduleDAG] Check for CopyToReg PhysReg clobbers in pre-RA-sched

[jrbyrnes]

Summary

This patch includes logic in the Scheduling phase of Instruction selection to check whether or not a CopyToReg will clobber a physical register's live range. If so, the scheduler will not be able to schedule the instruction in the current cycle with the current partial schedule. Without this, the scheduler can schedule CopyToRegs even if they violate a live range, resulting in correctness bugs.

[jrbyrnes]

Ported over to phab review to land in Trunk. Addressed the requests in initial review, renamed test file to better align with naming of previous test.

[rampitec]

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp

451

The check for LiveRegDefs[*AI] is excessive. If Node is not null the last check will not succeed.

[jrbyrnes]

Broke up logic in ScheduleDAGFast CheckForLiveRegDef to remove redundancy.

[rampitec]

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp

1311

Do you really want to insert it if Node is nullptr?

[jrbyrnes]

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp

1311

Hey Stas -- thanks for comments.

The are two theoretical ways Node will be nullptr. 1. We are using default value, 2. The SDNode we passed is actually a nullptr. Neither way creates a problem for this condition.

1. All invocations of CheckForLiveRegDef prior to this patch will use the default value. Since this condition is new, in order to maintain the prior behavior, we ignore this condition for the default value.

2. I assume that this is not possible, and I believe this is a valid assumption. The SDNode we pass is the Node that defines the Src SDValue in CopyToReg. I think the Src for CopyToReg must be coming from SDNode that is not nullptr. I can express this assumption via assert.

[rampitec]

LGTM with a small nit - run instnamer on the test. There should be no unnamed variables. Or rename it manually.

[arsenm]

llvm/test/CodeGen/AMDGPU/copy-to-reg-scc-clobber.ll

3

If you're going to test the different schedulers, should use the explicit flag for each one

[jrbyrnes]

Run instnamer on testfile, explicitly use "source" (RRList) scheduler for InstSelection Scheduler in test.

[jrbyrnes]

Remove accidental files

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp

if (N->getOpcode() == ISD::CopyFromReg) {
     }
   }
   return N->getSimpleValueType(NumRes);
 }
 
 /// CheckForLiveRegDef - Return true and update live register vector if the
 /// specified register def of the specified SUnit clobbers any "live" registers.
 static bool CheckForLiveRegDef(SUnit *SU, unsigned Reg,
-                               std::vector<SUnit*> &LiveRegDefs,
+                               std::vector<SUnit *> &LiveRegDefs,
                                SmallSet<unsigned, 4> &RegAdded,
                                SmallVectorImpl<unsigned> &LRegs,
-                               const TargetRegisterInfo *TRI) {
+                               const TargetRegisterInfo *TRI,
+                               const SDNode *Node = nullptr) {
   bool Added = false;
   for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI) {
-    if (LiveRegDefs[*AI] && LiveRegDefs[*AI] != SU) {
-      if (RegAdded.insert(*AI).second) {
-        LRegs.push_back(*AI);
-        Added = true;
-      }
-    }
-  }
+    // Check if Ref is live.
+    if (!LiveRegDefs[*AI])
+      continue;
+
+    // Allow multiple uses of the same def.
+    if (LiveRegDefs[*AI] == SU)
+      continue;
+
+    // Allow multiple uses of same def
+    if (Node && LiveRegDefs[*AI]->getNode() == Node)
+      continue;
+
+    // Add Reg to the set of interfering live regs.
   return Added;
 }
 
 /// DelayForLiveRegsBottomUp - Returns true if it is necessary to delay
 /// scheduling of the given node to satisfy live physical register dependencies.
 /// If the specific node is the last one that's available to schedule, do
 /// whatever is necessary (i.e. backtracking or cloning) to make it possible.
 bool ScheduleDAGFast::DelayForLiveRegsBottomUp(SUnit *SU,
if (Node->getOpcode() == ISD::INLINEASM ||
             if (Register::isPhysicalRegister(Reg))
               CheckForLiveRegDef(SU, Reg, LiveRegDefs, RegAdded, LRegs, TRI);
           }
         } else
           i += NumVals;
       }
       continue;
     }
+
+    if (Node->getOpcode() == ISD::CopyToReg) {
+      Register Reg = cast<RegisterSDNode>(Node->getOperand(1))->getReg();
+      if (Reg.isPhysical()) {
+        SDNode *SrcNode = Node->getOperand(2).getNode();
+        CheckForLiveRegDef(SU, Reg, LiveRegDefs, RegAdded, LRegs, TRI, SrcNode);
+      }
+    }
+
     if (!Node->isMachineOpcode())
       continue;
     const MCInstrDesc &MCID = TII->get(Node->getMachineOpcode());
     if (!MCID.ImplicitDefs)
       continue;
     for (const MCPhysReg *Reg = MCID.getImplicitDefs(); *Reg; ++Reg) {
       CheckForLiveRegDef(SU, *Reg, LiveRegDefs, RegAdded, LRegs, TRI);
     }

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp

for (const MCPhysReg *ImpDef = MCID.getImplicitDefs(); *ImpDef; ++ImpDef) {
       ++NumRes;
     }
   }
   return N->getSimpleValueType(NumRes);
 }
 
 /// CheckForLiveRegDef - Return true and update live register vector if the
 /// specified register def of the specified SUnit clobbers any "live" registers.
-static void CheckForLiveRegDef(SUnit *SU, unsigned Reg,
-                               SUnit **LiveRegDefs,
+static void CheckForLiveRegDef(SUnit *SU, unsigned Reg, SUnit **LiveRegDefs,
                                SmallSet<unsigned, 4> &RegAdded,
                                SmallVectorImpl<unsigned> &LRegs,
-                               const TargetRegisterInfo *TRI) {
+                               const TargetRegisterInfo *TRI,
+                               const SDNode *Node = nullptr) {
   for (MCRegAliasIterator AliasI(Reg, TRI, true); AliasI.isValid(); ++AliasI) {
 
     // Check if Ref is live.
     if (!LiveRegDefs[*AliasI]) continue;
 
     // Allow multiple uses of the same def.
     if (LiveRegDefs[*AliasI] == SU) continue;
 
+    // Allow multiple uses of same def
+    if (Node && LiveRegDefs[*AliasI]->getNode() == Node)
+      continue;
+
     // Add Reg to the set of interfering live regs.
     if (RegAdded.insert(*AliasI).second) {
       LRegs.push_back(*AliasI);
     }
   }
 }
 
 /// CheckForLiveRegDefMasked - Check for any live physregs that are clobbered
if (Node->getOpcode() == ISD::INLINEASM ||
               CheckForLiveRegDef(SU, Reg, LiveRegDefs.get(), RegAdded, LRegs, TRI);
           }
         } else
           i += NumVals;
       }
       continue;
     }
 
+    if (Node->getOpcode() == ISD::CopyToReg) {
+      Register Reg = cast<RegisterSDNode>(Node->getOperand(1))->getReg();
+      if (Reg.isPhysical()) {
+        SDNode *SrcNode = Node->getOperand(2).getNode();
+        CheckForLiveRegDef(SU, Reg, LiveRegDefs.get(), RegAdded, LRegs, TRI,
+                           SrcNode);
+      }
+    }
+
     if (!Node->isMachineOpcode())
       continue;
     // If we're in the middle of scheduling a call, don't begin scheduling
     // another call. Also, don't allow any physical registers to be live across
     // the call.
     if (Node->getMachineOpcode() == TII->getCallFrameDestroyOpcode()) {
       // Check the special calling-sequence resource.
       unsigned CallResource = TRI->getNumRegs();

llvm/test/CodeGen/AMDGPU/copy-to-reg-scc-clobber.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -march=amdgcn -mcpu=gfx90a -O3 -pre-RA-sched=source < %s | FileCheck -check-prefix=RRLIST %s
+; RUN: llc -march=amdgcn -mcpu=gfx90a -O3 -pre-RA-sched=fast < %s | FileCheck -check-prefix=FAST %s
+
+
+define protected amdgpu_kernel void @sccClobber(ptr addrspace(1) %a, ptr addrspace(1) %b, ptr addrspace(1) %e, ptr addrspace(1) %f, ptr addrspace(1) %pout.coerce) {
+; RRLIST-LABEL: sccClobber:
+; RRLIST:       ; %bb.0: ; %entry
+; RRLIST-NEXT:    s_load_dwordx8 s[4:11], s[0:1], 0x24
+; RRLIST-NEXT:    v_mov_b32_e32 v2, 0
+; RRLIST-NEXT:    s_waitcnt lgkmcnt(0)
+; RRLIST-NEXT:    s_load_dword s16, s[8:9], 0x0
+; RRLIST-NEXT:    s_load_dwordx2 s[2:3], s[6:7], 0x0
+; RRLIST-NEXT:    s_load_dwordx2 s[12:13], s[4:5], 0x0
+; RRLIST-NEXT:    s_load_dwordx2 s[14:15], s[0:1], 0x44
+; RRLIST-NEXT:    s_load_dword s17, s[10:11], 0x0
+; RRLIST-NEXT:    s_waitcnt lgkmcnt(0)
+; RRLIST-NEXT:    s_min_i32 s4, s16, 0
+; RRLIST-NEXT:    v_pk_mov_b32 v[0:1], s[2:3], s[2:3] op_sel:[0,1]
+; RRLIST-NEXT:    v_cmp_lt_i64_e32 vcc, s[12:13], v[0:1]
+; RRLIST-NEXT:    s_and_b64 s[0:1], vcc, exec
+; RRLIST-NEXT:    s_cselect_b32 s0, s16, s17
+; RRLIST-NEXT:    s_cmp_eq_u64 s[12:13], s[2:3]
+; RRLIST-NEXT:    s_cselect_b32 s0, s4, s0
+; RRLIST-NEXT:    v_mov_b32_e32 v0, s0
+; RRLIST-NEXT:    global_store_dword v2, v0, s[14:15]
+; RRLIST-NEXT:    s_endpgm
+;
+; FAST-LABEL: sccClobber:
+; FAST:       ; %bb.0: ; %entry
+; FAST-NEXT:    s_load_dwordx8 s[4:11], s[0:1], 0x24
+; FAST-NEXT:    v_mov_b32_e32 v2, 0
+; FAST-NEXT:    s_waitcnt lgkmcnt(0)
+; FAST-NEXT:    s_load_dword s16, s[8:9], 0x0
+; FAST-NEXT:    s_load_dwordx2 s[2:3], s[6:7], 0x0
+; FAST-NEXT:    s_load_dwordx2 s[12:13], s[4:5], 0x0
+; FAST-NEXT:    s_load_dwordx2 s[14:15], s[0:1], 0x44
+; FAST-NEXT:    s_load_dword s17, s[10:11], 0x0
+; FAST-NEXT:    s_waitcnt lgkmcnt(0)
+; FAST-NEXT:    s_min_i32 s4, s16, 0
+; FAST-NEXT:    v_pk_mov_b32 v[0:1], s[2:3], s[2:3] op_sel:[0,1]
+; FAST-NEXT:    v_cmp_lt_i64_e32 vcc, s[12:13], v[0:1]
+; FAST-NEXT:    s_and_b64 s[0:1], vcc, exec
+; FAST-NEXT:    s_cselect_b32 s0, s16, s17
+; FAST-NEXT:    s_cmp_eq_u64 s[12:13], s[2:3]
+; FAST-NEXT:    s_cselect_b32 s0, s4, s0
+; FAST-NEXT:    v_mov_b32_e32 v0, s0
+; FAST-NEXT:    global_store_dword v2, v0, s[14:15]
+; FAST-NEXT:    s_endpgm
+entry:
+  %i = load i64, ptr addrspace(1) %a, align 8
+  %i.1 = load i64, ptr addrspace(1) %b, align 8
+  %i.2 = load i32, ptr addrspace(1) %e, align 4
+  %i.3 = load i32, ptr addrspace(1) %f, align 4
+  %cmp7.1 = icmp eq i64 %i, %i.1
+  %call.1 = tail call noundef i32 @llvm.smin.i32(i32 noundef 0, i32 noundef %i.2)
+  %cmp8.1 = icmp slt i64 %i, %i.1
+  %cond.1 = select i1 %cmp8.1, i32 %i.2, i32 %i.3
+  %cond14.1 = select i1 %cmp7.1, i32 %call.1, i32 %cond.1
+  store i32 %cond14.1, ptr addrspace(1) %pout.coerce, align 4
+  ret void
+}
+
+declare i32 @llvm.smin.i32(i32, i32)