[AMDGPU] Do not reserve 16-bit registers

[foad]

Summary

There should be no need to reserve all SGPR hi16/lo16 halves, or all
AGPR hi16 halves. This should be done by marking the corresponding
register classes as not allocatable instead.

[foad]

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp

613

I assume this comment refers to a vcc liveness error in test/CodeGen/AMDGPU/frame-index-elimination.ll. I have fixed that in a different way with D147157.

[rampitec]

I am not so sure about SGPRs. I believe it is legal to use SGPR halves.

[Joe_Nash]

SGPR_HI16 is already marked as not allocatable. So L611 was partially redundant with that. But this patch does appear to remove a barrier to allocating into the hi or lo half of TMA_LO or some other non-GPR SReg.
I guess there is other code somewhere preventing allocating to those non-GPR SReg? If so, this patch can be marked NFC and the commit reworded to say it skips redundant reservations.

In D147158#4231480, @rampitec wrote:

I am not so sure about SGPRs. I believe it is legal to use SGPR halves.

It appears that this patch does not change anything about the legality of allocating registers in class SGPR_LO16.

[rampitec]

In D147158#4231755, @Joe_Nash wrote:

In D147158#4231480, @rampitec wrote:

I am not so sure about SGPRs. I believe it is legal to use SGPR halves.

It appears that this patch does not change anything about the legality of allocating registers in class SGPR_LO16.

But if we will allow SGPRs this code will be needed on older targets again.

[Joe_Nash]

In D147158#4231787, @rampitec wrote:

In D147158#4231755, @Joe_Nash wrote:

In D147158#4231480, @rampitec wrote:

I am not so sure about SGPRs. I believe it is legal to use SGPR halves.

It appears that this patch does not change anything about the legality of allocating registers in class SGPR_LO16.

But if we will allow SGPRs this code will be needed on older targets again.

I think you are right about that. Perhaps that is a a good reason not to merge this patch unless there is some immediate benefit.

[foad]

Make SReg_LO16 unallocatable

[foad]

In D147158#4231789, @Joe_Nash wrote:

In D147158#4231787, @rampitec wrote:

In D147158#4231755, @Joe_Nash wrote:

In D147158#4231480, @rampitec wrote:

I am not so sure about SGPRs. I believe it is legal to use SGPR halves.

I am not sure about this. It is "legal" in that the SGPR_LO16 is allocatable, but I don't think anything uses it - except for one handwritten MIR test in test/CodeGen/AMDGPU/fold_16bit_imm.mir.

It appears that this patch does not change anything about the legality of allocating registers in class SGPR_LO16.

But if we will allow SGPRs this code will be needed on older targets again.

I think you are right about that. Perhaps that is a a good reason not to merge this patch unless there is some immediate benefit.

I don't like guessing what will be required in future. Even with the GFX11 True16 work in progress it is unclear to me whether 16-bit SGPRs will be used.

My motivation is to clean up and simplify SIRegisterInfo::getReservedRegs based on what is required today. It seems like a lot of stuff has crept in and I am trying to understand what it is all for.

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp

615

I've tried to preserve the intent of this code by making SReg_LO16 unallocatable but leaving SGPR_LO16 allocatable.

[rampitec]

Here is the thing:

v_mov_b16 v0.h, vcc_lo.h
    0xd59c4800
    0x0000006a
v_mov_b16 v0.l, vcc_hi.l
    0x7e00386b

So this all seems legal.

[rampitec]

OK, let's merge this. I believe t16 work will need to redo it anyway at the end.

[foad]

In D147158#4234470, @rampitec wrote:

Here is the thing:

v_mov_b16 v0.h, vcc_lo.h
    0xd59c4800
    0x0000006a
v_mov_b16 v0.l, vcc_hi.l
    0x7e00386b

So this all seems legal.

Right, VALU instructions can access 16-bit SGPRs, and the assembler/disassembler has to accept that. The tricky question is whether we want to expose that to codegen. and I think it will be hard to do that because there is no S_MOV_B16 instruction and in general no way to write to a 16-bit SGPR; you can only read from them.

Thanks for the review.

[rampitec]

In D147158#4236545, @foad wrote:

In D147158#4234470, @rampitec wrote:

Here is the thing:

v_mov_b16 v0.h, vcc_lo.h
    0xd59c4800
    0x0000006a
v_mov_b16 v0.l, vcc_hi.l
    0x7e00386b

So this all seems legal.

Right, VALU instructions can access 16-bit SGPRs, and the assembler/disassembler has to accept that. The tricky question is whether we want to expose that to codegen. and I think it will be hard to do that because there is no S_MOV_B16 instruction and in general no way to write to a 16-bit SGPR; you can only read from them.

Thanks for the review.

Good point about the scalar move. It explains the problem. Now I think I agree about the codegen support.

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp

BitVector SIRegisterInfo::getReservedRegs(const MachineFunction &MF) const {
   //
   unsigned MaxNumSGPRs = ST.getMaxNumSGPRs(MF);
   unsigned TotalNumSGPRs = AMDGPU::SGPR_32RegClass.getNumRegs();
   for (unsigned i = MaxNumSGPRs; i < TotalNumSGPRs; ++i) {
     unsigned Reg = AMDGPU::SGPR_32RegClass.getRegister(i);
     reserveRegisterTuples(Reserved, Reg);
   }
 
-  for (auto Reg : AMDGPU::SReg_32RegClass) {
-    Reserved.set(getSubReg(Reg, AMDGPU::hi16));
-    Register Low = getSubReg(Reg, AMDGPU::lo16);
-    // This is to prevent BB vcc liveness errors.
-    if (!AMDGPU::SGPR_LO16RegClass.contains(Low))
-      Reserved.set(Low);
-  }
-
   Register ScratchRSrcReg = MFI->getScratchRSrcReg();
   if (ScratchRSrcReg != AMDGPU::NoRegister) {
     // Reserve 4 SGPRs for the scratch buffer resource descriptor in case we
     // need to spill.
     // TODO: May need to reserve a VGPR if doing LDS spilling.
     reserveRegisterTuples(Reserved, ScratchRSrcReg);
   }
 
BitVector SIRegisterInfo::getReservedRegs(const MachineFunction &MF) const {
   }
 
   // Reserve VGPRs/AGPRs.
   //
   unsigned MaxNumVGPRs = ST.getMaxNumVGPRs(MF);
   unsigned MaxNumAGPRs = MaxNumVGPRs;
   unsigned TotalNumVGPRs = AMDGPU::VGPR_32RegClass.getNumRegs();
 
-  for (auto Reg : AMDGPU::AGPR_32RegClass) {
-    Reserved.set(getSubReg(Reg, AMDGPU::hi16));
-  }
-
   // On GFX90A, the number of VGPRs and AGPRs need not be equal. Theoretically,
   // a wave may have up to 512 total vector registers combining together both
   // VGPRs and AGPRs. Hence, in an entry function without calls and without
   // AGPRs used within it, it is possible to use the whole vector register
   // budget for VGPRs.
   //
   // TODO: it shall be possible to estimate maximum AGPR/VGPR pressure and split
   //       register file accordingly.