[PATCH] D134423: [AMDGPU] Fix vgpr2sgpr copy analysis to check scalar operands of buffer instructions use scalar registers.

[krishna chaitanya sankisa via Phabricator via llvm-commits]

skc7 added a comment.



In D134423#3919858 <https://reviews.llvm.org/D134423#3919858>, @alex-t wrote:

> In D134423#3914502 <https://reviews.llvm.org/D134423#3914502>, @foad wrote:
>
>> In D134423#3912577 <https://reviews.llvm.org/D134423#3912577>, @alex-t wrote:
>>
>>> BTW, if %5 is divergent we have a bug in ISel. We now should not have any V2S copy with the divergent source.
>>
>> Look at the MIR that @skc7 quoted. %5 is divergent - it's copied from a vgpr function argument.
>
> The BUFFER_LOAD_DWORDX4_OFFEN is one of (as I remember correctly 5) the exceptional opcodes for which V2S copy is created even in case the copy source is divergent.
> There is no bug in ISel. We have the value in VGPR because it is divergent and this is correct. The V2S copy is created in InstrEmitter just because the opcode requires SGPR.
> We have yet several other such opcodes.
>
> V_WRITELANE_B32, S_BUFFER_LOAD_DWORD_IMM, BUFFER_LOAD_FORMAT_X_OFFSET, BUFFER_LOAD_FORMAT_X_IDXEN, BUFFER_LOAD_FORMAT_X_OFFEN, BUFFER_LOAD_FORMAT_X_BOTHEN, IMAGE_SAMPLE_V1_V2
>  And this is really a TODO. For each of them, we should make a design and change legalizeOperand correspondingly.

@alex-t @foad @arsenm  As was suggested, legalizeOperand needs to be updated to support the mentioned opcodes which have similar issue. We will take it up as a parallel task and try to fix it.

But as a temporary work around, shall we update HasMBUFScalarReg to true, when a "V2S copy" is found and its result is used by MUBUF/MTBUF for scalar operands. HasMBUFScalarReg flag determines copy lowering way to v_readfirstlane_b32
This fixes the specific issue we encountered with BUFFER_LOAD_DWORDX4_OFFEN. "Illegal virtual register for instruction"

  Register Reg = Inst->getOperand(0).getReg();
  if (TRI->isSGPRReg(*MRI, Reg) && !TII->isVALU(*Inst)) {
  for (auto &U : MRI->use_instructions(Reg)) {
     Users.push_back(&U);
     if (Inst->isCopy()) {
       unsigned Opc = U.getOpcode();
       if (Reg.isVirtual() &&
           (MRI->getRegClass(Reg) == &AMDGPU::SGPR_128RegClass ||
            MRI->getRegClass(Reg) == &AMDGPU::SReg_32RegClass) &&
           (TII->isMUBUF(Opc) || TII->isMTBUF(Opc))) {
         Info.HasMBUFScalarReg = true;
       }
     }
   }
  }


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

https://reviews.llvm.org/D134423