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

[Matt Arsenault]

> 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?

> +    default:
> +      if (Src1.isImm()) {
> +        ErrInfo = "VOP[2C] src1 cannot be an immediate.";
> +        return false;
> +      }
>     }
>   }
> 
> diff --git a/lib/Target/R600/SIInstructions.td b/lib/Target/R600/SIInstructions.td
> index fbd38c9..f598e30 100644
> --- a/lib/Target/R600/SIInstructions.td
> +++ b/lib/Target/R600/SIInstructions.td
> @@ -1510,16 +1510,16 @@ defm V_READLANE_B32 : VOP2SI_3VI_m <
>   vop3 <0x001, 0x289>,
>   "v_readlane_b32",
>   (outs SReg_32:$vdst),
> -  (ins VGPR_32:$src0, SSrc_32:$vsrc1),
> -  "v_readlane_b32 $vdst, $src0, $vsrc1"
> +  (ins VGPR_32:$src0, SSrc_32:$src1),
> +  "v_readlane_b32 $vdst, $src0, $src1"
>> ;
> 
> defm V_WRITELANE_B32 : VOP2SI_3VI_m <
>   vop3 <0x002, 0x28a>,
>   "v_writelane_b32",
>   (outs VGPR_32:$vdst),
> -  (ins SReg_32:$src0, SSrc_32:$vsrc1),
> -  "v_writelane_b32 $vdst, $src0, $vsrc1"
> +  (ins SReg_32:$src0, SSrc_32:$src1),
> +  "v_writelane_b32 $vdst, $src0, $src1"
>> ;
> 
> // These instructions only exist on SI and CI
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits