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

[Marek Olšák]

Ping.

On Sun, Feb 15, 2015 at 2:07 PM, Marek Olšák <maraeo at gmail.com> wrote:
> A new patch series is attached. Please review.
>
> Marek
>
> On Sun, Feb 15, 2015 at 12:21 PM, Marek Olšák <maraeo at gmail.com> wrote:
>> 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