[PATCH] D79073: [AMDGPU] For PAL, make sure Scratch Buffer Descriptor do not clobber GIT pointer

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:306
+        (!ST.isAmdPalOS() ||
+         !TRI->isSubRegisterEq(Reg, MFI->getGITPtrLoReg(MF)))) {
       MRI.replaceRegWith(ScratchRsrcReg, Reg);
----------------
Hoist getGITPtrLoReg out of the loop


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:343
   // FIXME: Hack to not crash in situations which emitted an error.
-  if (ScratchWaveOffsetReg == AMDGPU::NoRegister)
+  if (PreloadedScratchWaveOffsetReg == AMDGPU::NoRegister)
     return;
----------------
!PreloadedScratchWaveOffsetReg


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:388
+  // wave offset to a free SGPR.
+  Register ScratchWaveOffsetReg = AMDGPU::NoRegister;
+  if (TRI->isSubRegisterEq(ScratchRsrcReg, PreloadedScratchWaveOffsetReg)) {
----------------
Don't need = NoRegister


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:397
+          !TRI->isSubRegisterEq(ScratchRsrcReg, Reg) &&
+          (!ST.isAmdPalOS() || MFI->getGITPtrLoReg(MF) != Reg)) {
+        ScratchWaveOffsetReg = Reg;
----------------
Hoist getGITPtrLoReg out of the loop. Also this should be redundant with the isAmdPal check?


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:407
+  }
+  assert(ScratchWaveOffsetReg != AMDGPU::NoRegister);
+
----------------
Don't need the != NoRegister


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp:446
+  if (!ST.isAmdPalOS())
+    return AMDGPU::NoRegister;
+  auto GitPtrLo = AMDGPU::SGPR0; // Low GIT address passed in
----------------
return Register()


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp:447
+    return AMDGPU::NoRegister;
+  auto GitPtrLo = AMDGPU::SGPR0; // Low GIT address passed in
+  if (ST.hasMergedShaders()) {
----------------
Register, not auto


================
Comment at: llvm/test/CodeGen/AMDGPU/SRSRC-GIT-clobber-check.ll:1
+; RUN: llc -mtriple=amdgcn--amdpal -mcpu=gfx1010 -print-after=prologepilog < %s 2>&1 | FileCheck -check-prefix=CHECK %s
+
----------------
If you're only going to check MIR, should use -stop-after and directly check the output.

A MIR test that only runs PEI is also probably more stable for this


================
Comment at: llvm/test/CodeGen/AMDGPU/SRSRC-GIT-clobber-check.ll:6-8
+; CHECK-NOT: $sgpr8_sgpr9 = S_GETPC_B64
+; CHECK-NOT: $sgpr8 = S_MOV_B32 $sgpr8
+; CHECK-NOT: $sgpr8_sgpr9_sgpr10_sgpr11 = S_LOAD_DWORDX4_IMM
----------------
Negative checks are going to be really fragile here


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79073/new/

https://reviews.llvm.org/D79073