[AMDGPU] Fix incorrect arch assert while setting up FlatScratchInit

[arsenm]

Needs test. Also not sure why we would have an assert for this here in the first place

[madhur13490]

In D84391#2169172, @arsenm wrote:

Needs test. Also not sure why we would have an assert for this here in the first place

In fact I like the assert. It guarantees that we're not building something insane. Moreover, should we refactor this code to something more readble? :

switch(llvm::AMDGPUSubTarget) {
case GFX:8 {}
case GFX:9: {}
...
}

Only caveat is the usage of function flatScratchIsPointer(). We can get rid of its usage in its current form but in future we may want to add some code to it to take contextual decisions.

[arsenm]

In D84391#2169292, @madhur13490 wrote:

In D84391#2169172, @arsenm wrote:

Needs test. Also not sure why we would have an assert for this here in the first place

In fact I like the assert. It guarantees that we're not building something insane. Moreover, should we refactor this code to something more readble? :

switch(llvm::AMDGPUSubTarget) {
case GFX:8 {}
case GFX:9: {}
...
}

Only caveat is the usage of function flatScratchIsPointer(). We can get rid of its usage in its current form but in future we may want to add some code to it to take contextual decisions.

I would like to purge all checks of the generation. The relevant bit is flatScratchIsPointer. The generation is redundant and potentially misleading.

[madhur13490]

In D84391#2169391, @arsenm wrote:

In D84391#2169292, @madhur13490 wrote:

In D84391#2169172, @arsenm wrote:

Needs test. Also not sure why we would have an assert for this here in the first place

In fact I like the assert. It guarantees that we're not building something insane. Moreover, should we refactor this code to something more readble? :

switch(llvm::AMDGPUSubTarget) {
case GFX:8 {}
case GFX:9: {}
...
}

Only caveat is the usage of function flatScratchIsPointer(). We can get rid of its usage in its current form but in future we may want to add some code to it to take contextual decisions.

I would like to purge all checks of the generation. The relevant bit is flatScratchIsPointer. The generation is redundant and potentially misleading.

I am not sure what does that mean. Do you agree with my way of refactoring or you're suggesting something else? With my approach there won't be any checks as I said we can get rid of flatScratchIsPointer() too.

[arsenm]

In D84391#2169473, @madhur13490 wrote:

I am not sure what does that mean. Do you agree with my way of refactoring or you're suggesting something else? With my approach there won't be any checks as I said we can get rid of flatScratchIsPointer() too.

I mean exactly the opposite. We should not check the generation anywhere. The hardware does not make changes in a nice, disciplined, sequenced way. Everything should be based on specific feature checks. I.e. there should only be flatScratchIsPointer checks, and nothing checking getGeneration

[madhur13490]

In D84391#2169493, @arsenm wrote:

In D84391#2169473, @madhur13490 wrote:

I am not sure what does that mean. Do you agree with my way of refactoring or you're suggesting something else? With my approach there won't be any checks as I said we can get rid of flatScratchIsPointer() too.

I mean exactly the opposite. We should not check the generation anywhere. The hardware does not make changes in a nice, disciplined, sequenced way. Everything should be based on specific feature checks. I.e. there should only be flatScratchIsPointer checks, and nothing checking getGeneration

I think we have to check getGeneration at some point because meaning of FlatScratchInit is different for every arch (so the codegen). I don't think flatScratchIsPointer() is enough for that. BTW I'd like to handle refactoring in a separate patch. We should let this land with a test.

[arsenm]

This doesn't fix an assert, it just changing it for documentation

[scott.linder]

Can we just add a predicate for whatever necessitates the SETREG for gfx10+? No idea on a good name, but if it were flatScratchIsHwreg() and it implied flatScratchIsPointer() this becomes:

if (ST.flatScratchIsHwreg()) {
  // pointer add then S_SETREG
} else if (ST.flatScratchIsPointer()) (
  // pointer add
} else {
  // size and offset dance
}
llvm_unreachable();

I agree that the existing assert here doesn't really mean anything, not sure why I wrote it.

[scott.linder]

In D84391#2170741, @scott.linder wrote:

Can we just add a predicate for whatever necessitates the SETREG for gfx10+? No idea on a good name, but if it were flatScratchIsHwreg() and it implied flatScratchIsPointer() this becomes:

if (ST.flatScratchIsHwreg()) {
  // pointer add then S_SETREG
} else if (ST.flatScratchIsPointer()) (
  // pointer add
} else {
  // size and offset dance
}
llvm_unreachable();

I agree that the existing assert here doesn't really mean anything, not sure why I wrote it.

Rather, the llvm_unreachable isn't needed if we consider the offset/add case as the "default", but maybe we should predicate it too? It would be nice if we could do this all in a way that was statically known to cover all the targets we have.

Could we combine the various predicate functions into one which returns an enumeration instead? Similar to Madhur's proposal, but without the direct reference to GFX#? I.e.

switch (ST.getFlatScratchSetupKind()) {
AMDGPU::PointerInHwreg:
  // ...
  break;
AMDGPU::PointerInSGPR:
  // ...
  break;
AMDGPU::SizeAndOffsetInSGPR:
  // ...
  break;
}

Then when we add a new variant the compiler points us to everywhere that cares.

[madhur13490]

rebase

[madhur13490]

In D84391#2170772, @scott.linder wrote:

In D84391#2170741, @scott.linder wrote:

Can we just add a predicate for whatever necessitates the SETREG for gfx10+? No idea on a good name, but if it were flatScratchIsHwreg() and it implied flatScratchIsPointer() this becomes:

if (ST.flatScratchIsHwreg()) {
  // pointer add then S_SETREG
} else if (ST.flatScratchIsPointer()) (
  // pointer add
} else {
  // size and offset dance
}
llvm_unreachable();

I agree that the existing assert here doesn't really mean anything, not sure why I wrote it.

Rather, the llvm_unreachable isn't needed if we consider the offset/add case as the "default", but maybe we should predicate it too? It would be nice if we could do this all in a way that was statically known to cover all the targets we have.

Could we combine the various predicate functions into one which returns an enumeration instead? Similar to Madhur's proposal, but without the direct reference to GFX#? I.e.

switch (ST.getFlatScratchSetupKind()) {
AMDGPU::PointerInHwreg:
  // ...
  break;
AMDGPU::PointerInSGPR:
  // ...
  break;
AMDGPU::SizeAndOffsetInSGPR:
  // ...
  break;
}

Then when we add a new variant the compiler points us to everywhere that cares.

Yeah, let's handle this in a separate patch.

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

if (ST.getGeneration() >= AMDGPUSubtarget::GFX10) {
                        (31 << AMDGPU::Hwreg::WIDTH_M1_SHIFT_)));
       BuildMI(MBB, I, DL, TII->get(AMDGPU::S_SETREG_B32)).
         addReg(FlatScrInitHi).
         addImm(int16_t(AMDGPU::Hwreg::ID_FLAT_SCR_HI |
                        (31 << AMDGPU::Hwreg::WIDTH_M1_SHIFT_)));
       return;
     }
 
+    // For GFX9.
     BuildMI(MBB, I, DL, TII->get(AMDGPU::S_ADD_U32), AMDGPU::FLAT_SCR_LO)
       .addReg(FlatScrInitLo)
       .addReg(ScratchWaveOffsetReg);
     BuildMI(MBB, I, DL, TII->get(AMDGPU::S_ADDC_U32), AMDGPU::FLAT_SCR_HI)
       .addReg(FlatScrInitHi)
       .addImm(0);
 
     return;
   }
 
-  assert(ST.getGeneration() < AMDGPUSubtarget::GFX10);
+  assert(ST.getGeneration() < AMDGPUSubtarget::GFX9);
 
   // Copy the size in bytes.
   BuildMI(MBB, I, DL, TII->get(AMDGPU::COPY), AMDGPU::FLAT_SCR_LO)
     .addReg(FlatScrInitHi, RegState::Kill);
 
   // Add wave offset in bytes to private base offset.
   // See comment in AMDKernelCodeT.h for enable_sgpr_flat_scratch_init.
   BuildMI(MBB, I, DL, TII->get(AMDGPU::S_ADD_U32), FlatScrInitLo)