[PATCH] D136169: [AMDGPU] Avoid SCC clobbering before S_CSELECT_B32

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:2216-2224
+          while (I != MBB->end()) {
+            if (*I != MI && I->readsRegister(AMDGPU::SCC)) {
+              NeedSaveSCC = true;
+              break;
+            }
+            if (*I != MI && I->definesRegister(AMDGPU::SCC))
+              break;
----------------
alex-t wrote:
> foad wrote:
> > What is this loop for? Isn't the scavenger already supposed to know whether SCC is live?
> > What is this loop for? Isn't the scavenger already supposed to know whether SCC is live?
> 
> The scavenger's knowledge depends on the accurate dead/kill flags insertion. This loop decreases the number of false-positive "live" SCC and accordingly unnecessary SCC saves/restores.
> 
> As Matt rightly pointed out, the dead/kill correct placement is out of this change scope.
> It also is going to be a complex task.
> 
> Should I add a corresponding TODO here?
Haven't dead/kill flags been automatically computed before this code runs?

If you do need this loop, shouldn't it conservatively assume that SCC is live at the end of the basic block?


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:2263
+          Register SCCCopy =
+              RS->scavengeRegister(getBoolRC(), std::prev(P), 0, !UseSGPR);
+          if (!SCCCopy)
----------------
alex-t wrote:
> foad wrote:
> > None of this code should depend on wave size. You don't want getBoolRC here - that's for VCC which might be a register pair in wave64. You can always save SCC in a single 32-bit SGPR.
> Since back SGPR to SCC copy depends on EXEC/EXEC_LO and I am using the same register for saving and restoring I need to use 64bit unless isWave32. I am not sure if I can use S_AND_B32 reg, exec_lo in wave64 mode?
You can just use "s_cmp_lg_u32 sgpr, 0" to copy back to SCC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136169