[PATCH] R600/SI: Fix READLANE and WRITELANE lane select for VI

[Marek Olšák]

On Sat, Feb 14, 2015 at 6:51 PM, Matt Arsenault <arsenm2 at gmail.com> wrote:
>
>> On Feb 14, 2015, at 7:24 AM, Marek Olšák <maraeo at gmail.com> wrote:
>>
>> From: Marek Olšák <marek.olsak at amd.com>
>>
>> VOP2 declares vsrc1, but VOP3 declares src1.
>> We can't use the same "ins" if the operands have different names in SI
>> and VI encodings.
>>
>> Since the verifier only checks src1, verification of what was vsrc1 is now
>> enabled.
>>
>> This fixes a hang in geometry shaders which spill M0 on VI.
>> (BTW it doesn't look like M0 needs spilling and the spilling seems
>> duplicated 3 times)
>> ---
>> lib/Target/R600/SIInstrFormats.td |  4 ++--
>> lib/Target/R600/SIInstrInfo.cpp   | 20 +++++++++++++++++---
>> lib/Target/R600/SIInstructions.td |  8 ++++----
>> 3 files changed, 23 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/Target/R600/SIInstrFormats.td b/lib/Target/R600/SIInstrFormats.td
>> index 0798241..ca9b3d4 100644
>> --- a/lib/Target/R600/SIInstrFormats.td
>> +++ b/lib/Target/R600/SIInstrFormats.td
>> @@ -300,10 +300,10 @@ class VOP2e <bits<6> op> : Enc32 {
>>
>>   bits<8> VDST;
>>   bits<9> SRC0;
>> -  bits<8> VSRC1;
>> +  bits<8> SRC1;
>>
>>   let Inst{8-0} = SRC0;
>> -  let Inst{16-9} = VSRC1;
>> +  let Inst{16-9} = SRC1;
>>   let Inst{24-17} = VDST;
>>   let Inst{30-25} = op;
>>   let Inst{31} = 0x0; //encoding
>> diff --git a/lib/Target/R600/SIInstrInfo.cpp b/lib/Target/R600/SIInstrInfo.cpp
>> index 8b65d5c..41d4770 100644
>> --- a/lib/Target/R600/SIInstrInfo.cpp
>> +++ b/lib/Target/R600/SIInstrInfo.cpp
>> @@ -1233,9 +1233,23 @@ bool SIInstrInfo::verifyInstruction(const MachineInstr *MI,
>>   // Verify SRC1 for VOP2 and VOPC
>>   if (Src1Idx != -1 && (isVOP2(Opcode) || isVOPC(Opcode))) {
>>     const MachineOperand &Src1 = MI->getOperand(Src1Idx);
>> -    if (Src1.isImm()) {
>> -      ErrInfo = "VOP[2C] src1 cannot be an immediate.";
>> -      return false;
>> +
>> +    switch (Opcode) {
>> +    // The lane select can be an SGPR, M0, or inline constant.
>> +    // Only SI uses VOP2 encoding for these.
>> +    case AMDGPU::V_READLANE_B32_si:
>> +    case AMDGPU::V_WRITELANE_B32_si:
>> +      if (isLiteralConstant(Src1, getOpSize(Opcode, Src1Idx))) {
>> +        ErrInfo = "Lane select cannot be a literal constant.";
>> +        return false;
>> +      }
>> +      break;
>> +
> I don’t think you need to special case these instructions. Why isn’t this already handled by OPERAND_REG_INLINE_C type operands? Do you need another that includes m0?

I can't use the default statement, because Src1 CAN be an inline
constant, but is this checking for VOP[2C] really needed? This already
seems to be covered by the OperandType checking.

I don't need another type that includes M0, at least not yet.

Sorry, I don't see how your patch series would help.

Marek