AMDGPU: Avoid overwriting saved PC

[kerbowa]

Summary

An outstanding load with same destination sgpr as call could cause PC to be
updated with junk value on return.

[arsenm]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

957

This is a potentially confusing name. It's the return address operand

959

This should never fail

[kerbowa]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

959

It fails with SI_TCRETURN/S_SETPC.

[kerbowa]

Address comments, rename variable.

[arsenm]

LGTM with test comment

llvm/test/CodeGen/AMDGPU/call-waw-waitcnt.mir

4

Could use a short description here of what this is testing, and point out the specific register names

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

if (MI.modifiesRegister(AMDGPU::EXEC, TRI)) {
           ScoreBrackets.hasPendingEvent(EXP_PARAM_ACCESS) ||
           ScoreBrackets.hasPendingEvent(EXP_POS_ACCESS) ||
           ScoreBrackets.hasPendingEvent(GDS_GPR_LOCK)) {
         Wait.ExpCnt = 0;
       }
     }
 
     if (MI.isCall() && callWaitsOnFunctionEntry(MI)) {
-      // Don't bother waiting on anything except the call address. The function
-      // is going to insert a wait on everything in its prolog. This still needs
-      // to be careful if the call target is a load (e.g. a GOT load).
+      // The function is going to insert a wait on everything in its prolog.
+      // This still needs to be careful if the call target is a load (e.g. a GOT
+      // load). We also need to check WAW depenancy with saved PC.
       Wait = AMDGPU::Waitcnt();
 
       int CallAddrOpIdx =
           AMDGPU::getNamedOperandIdx(MI.getOpcode(), AMDGPU::OpName::src0);
-      RegInterval Interval = ScoreBrackets.getRegInterval(&MI, TII, MRI, TRI,
-                                                          CallAddrOpIdx, false);
-      for (signed RegNo = Interval.first; RegNo < Interval.second; ++RegNo) {
+      RegInterval CallAddrOpInterval = ScoreBrackets.getRegInterval(
+          &MI, TII, MRI, TRI, CallAddrOpIdx, false);
+
+      for (signed RegNo = CallAddrOpInterval.first;
+           RegNo < CallAddrOpInterval.second; ++RegNo)
+        ScoreBrackets.determineWait(
+            LGKM_CNT, ScoreBrackets.getRegScore(RegNo, LGKM_CNT), Wait);
+
+      int RtnAddrOpIdx =
+            AMDGPU::getNamedOperandIdx(MI.getOpcode(), AMDGPU::OpName::dst);
+      if (RtnAddrOpIdx != -1) {
+        RegInterval RtnAddrOpInterval = ScoreBrackets.getRegInterval(
+            &MI, TII, MRI, TRI, RtnAddrOpIdx, false);
+
+        for (signed RegNo = RtnAddrOpInterval.first;
+             RegNo < RtnAddrOpInterval.second; ++RegNo)
           ScoreBrackets.determineWait(
               LGKM_CNT, ScoreBrackets.getRegScore(RegNo, LGKM_CNT), Wait);
       }
+
     } else {
       // FIXME: Should not be relying on memoperands.
       // Look at the source operands of every instruction to see if
       // any of them results from a previous memory operation that affects
       // its current usage. If so, an s_waitcnt instruction needs to be
       // emitted.
       // If the source operand was defined by a load, add the s_waitcnt
       // instruction.

llvm/test/CodeGen/AMDGPU/call-waw-waitcnt.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs -run-pass=si-insert-waitcnts %s -o - | FileCheck -check-prefix=GCN %s
+
+# $sgpr30_sgpr31 will hold the return address. We need a waitcnt before SI_CALL so
+# that the return address is not clobbered in the callee by the outstanding load.
+
+--- |
+  define amdgpu_kernel void @call_waw_waitcnt() #0 {
+    %1 = call i32 @func()
+    ret void
+  }
+
+  declare hidden i32 @func() #0
+
+  attributes #0 = { nounwind }
+...
+
+---
+name:            call_waw_waitcnt
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $sgpr4_sgpr5, $sgpr7, $sgpr0_sgpr1_sgpr2_sgpr3
+
+    ; GCN-LABEL: name: call_waw_waitcnt
+    ; GCN: liveins: $sgpr4_sgpr5, $sgpr7, $sgpr0_sgpr1_sgpr2_sgpr3
+    ; GCN: S_WAITCNT 0
+    ; GCN: $sgpr30_sgpr31 = S_LOAD_DWORDX2_IMM $sgpr4_sgpr5, 0, 0, 0
+    ; GCN: $sgpr33 = S_MOV_B32 killed $sgpr7
+    ; GCN: $flat_scr_lo = S_ADD_U32 killed $sgpr4, $sgpr33, implicit-def $scc
+    ; GCN: $flat_scr_hi = S_ADDC_U32 killed $sgpr5, 0, implicit-def $scc, implicit killed $scc
+    ; GCN: BUNDLE implicit-def $sgpr4_sgpr5, implicit-def $sgpr4, implicit-def $sgpr5, implicit-def $scc {
+    ; GCN:   $sgpr4_sgpr5 = S_GETPC_B64
+    ; GCN:   $sgpr4 = S_ADD_U32 internal $sgpr4, target-flags(amdgpu-rel32-lo) @func + 4, implicit-def $scc
+    ; GCN:   $sgpr5 = S_ADDC_U32 internal $sgpr5, target-flags(amdgpu-rel32-hi) @func + 4, implicit-def $scc, implicit internal $scc
+    ; GCN: }
+    ; GCN: $sgpr32 = S_MOV_B32 $sgpr33
+    ; GCN: S_WAITCNT 49279
+    ; GCN: dead $sgpr30_sgpr31 = SI_CALL killed renamable $sgpr4_sgpr5, @func, csr_amdgpu_highregs, implicit $sgpr0_sgpr1_sgpr2_sgpr3, implicit-def dead $vgpr0
+    ; GCN: S_ENDPGM 0
+    $sgpr30_sgpr31 = S_LOAD_DWORDX2_IMM $sgpr4_sgpr5, 0, 0, 0
+    $sgpr33 = S_MOV_B32 killed $sgpr7
+    $flat_scr_lo = S_ADD_U32 killed $sgpr4, $sgpr33, implicit-def $scc
+    $flat_scr_hi = S_ADDC_U32 killed $sgpr5, 0, implicit-def $scc, implicit killed $scc
+    BUNDLE implicit-def $sgpr4_sgpr5, implicit-def $sgpr4, implicit-def $sgpr5, implicit-def $scc {
+      $sgpr4_sgpr5 = S_GETPC_B64
+      $sgpr4 = S_ADD_U32 internal $sgpr4, target-flags(amdgpu-rel32-lo) @func + 4, implicit-def $scc
+      $sgpr5 = S_ADDC_U32 internal $sgpr5, target-flags(amdgpu-rel32-hi) @func + 4, implicit-def $scc, implicit internal $scc
+    }
+    $sgpr32 = S_MOV_B32 $sgpr33
+    dead $sgpr30_sgpr31 = SI_CALL killed renamable $sgpr4_sgpr5, @func, csr_amdgpu_highregs, implicit $sgpr0_sgpr1_sgpr2_sgpr3, implicit-def dead $vgpr0
+    S_ENDPGM 0
+...