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

[Marek Olšák]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-R600-SI-Remove-explicit-VOP-operand-checking.patch
Type: text/x-patch
Size: 1772 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150215/056e37a9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-R600-SI-Simplify-verification-of-AMDGPU-OPERAND_REG_.patch
Type: text/x-patch
Size: 1906 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150215/056e37a9/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-R600-SI-Fix-READLANE-and-WRITELANE-lane-select-for-V.patch
Type: text/x-patch
Size: 1998 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150215/056e37a9/attachment-0002.bin>