[PATCH] R600/SI: Fix READLANE and WRITELANE lane select for VI
Tom Stellard
tom at stellard.net
Tue Feb 17 17:11:30 PST 2015
On Sun, Feb 15, 2015 at 02:07:35PM +0100, Marek Olšák wrote:
> A new patch series is attached. Please review.
>
LGTM for all patches.
> 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
> From 145dafa608362e91082e56c143bd28fc3be8c7d3 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Marek=20Ol=C5=A1=C3=A1k?= <marek.olsak at amd.com>
> Date: Sun, 15 Feb 2015 12:40:25 +0100
> Subject: [PATCH 1/3] R600/SI: Remove explicit VOP operand checking
>
> This should be handled by the OperandType checking.
> ---
> lib/Target/R600/SIInstrInfo.cpp | 28 ----------------------------
> 1 file changed, 28 deletions(-)
>
> diff --git a/lib/Target/R600/SIInstrInfo.cpp b/lib/Target/R600/SIInstrInfo.cpp
> index 8b65d5c..cc8e4cc 100644
> --- a/lib/Target/R600/SIInstrInfo.cpp
> +++ b/lib/Target/R600/SIInstrInfo.cpp
> @@ -1230,34 +1230,6 @@ 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;
> - }
> - }
> -
> - // Verify VOP3
> - if (isVOP3(Opcode)) {
> - if (Src0Idx != -1 &&
> - isLiteralConstant(MI->getOperand(Src0Idx), getOpSize(Opcode, Src0Idx))) {
> - ErrInfo = "VOP3 src0 cannot be a literal constant.";
> - return false;
> - }
> - if (Src1Idx != -1 &&
> - isLiteralConstant(MI->getOperand(Src1Idx), getOpSize(Opcode, Src1Idx))) {
> - ErrInfo = "VOP3 src1 cannot be a literal constant.";
> - return false;
> - }
> - if (Src2Idx != -1 &&
> - isLiteralConstant(MI->getOperand(Src2Idx), getOpSize(Opcode, Src2Idx))) {
> - ErrInfo = "VOP3 src2 cannot be a literal constant.";
> - return false;
> - }
> - }
> -
> // Verify misc. restrictions on specific instructions.
> if (Desc.getOpcode() == AMDGPU::V_DIV_SCALE_F32 ||
> Desc.getOpcode() == AMDGPU::V_DIV_SCALE_F64) {
> --
> 2.1.0
>
> From f29925ac61c922cfd7c6e0525d1d03de0021e2d8 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Marek=20Ol=C5=A1=C3=A1k?= <marek.olsak at amd.com>
> Date: Sun, 15 Feb 2015 12:40:02 +0100
> Subject: [PATCH 2/3] R600/SI: Simplify verification of
> AMDGPU::OPERAND_REG_INLINE_C
>
> ---
> lib/Target/R600/SIInstrInfo.cpp | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/lib/Target/R600/SIInstrInfo.cpp b/lib/Target/R600/SIInstrInfo.cpp
> index cc8e4cc..db39311 100644
> --- a/lib/Target/R600/SIInstrInfo.cpp
> +++ b/lib/Target/R600/SIInstrInfo.cpp
> @@ -1151,6 +1151,8 @@ bool SIInstrInfo::verifyInstruction(const MachineInstr *MI,
> return false;
> }
>
> + int RegClass = Desc.OpInfo[i].RegClass;
> +
> switch (Desc.OpInfo[i].OperandType) {
> case MCOI::OPERAND_REGISTER:
> if (MI->getOperand(i).isImm()) {
> @@ -1161,13 +1163,10 @@ bool SIInstrInfo::verifyInstruction(const MachineInstr *MI,
> case AMDGPU::OPERAND_REG_IMM32:
> break;
> case AMDGPU::OPERAND_REG_INLINE_C:
> - if (MI->getOperand(i).isImm()) {
> - int RegClass = Desc.OpInfo[i].RegClass;
> - const TargetRegisterClass *RC = RI.getRegClass(RegClass);
> - if (!isInlineConstant(MI->getOperand(i), RC->getSize())) {
> - ErrInfo = "Illegal immediate value for operand.";
> - return false;
> - }
> + if (isLiteralConstant(MI->getOperand(i),
> + RI.getRegClass(RegClass)->getSize())) {
> + ErrInfo = "Illegal immediate value for operand.";
> + return false;
> }
> break;
> case MCOI::OPERAND_IMMEDIATE:
> @@ -1186,7 +1185,6 @@ bool SIInstrInfo::verifyInstruction(const MachineInstr *MI,
> if (!MI->getOperand(i).isReg())
> continue;
>
> - int RegClass = Desc.OpInfo[i].RegClass;
> if (RegClass != -1) {
> unsigned Reg = MI->getOperand(i).getReg();
> if (TargetRegisterInfo::isVirtualRegister(Reg))
> --
> 2.1.0
>
> From 338dfac5565567eb6d606d25848043cb263d5aeb Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Marek=20Ol=C5=A1=C3=A1k?= <marek.olsak at amd.com>
> Date: Sat, 14 Feb 2015 00:35:41 +0100
> Subject: [PATCH 3/3] R600/SI: Fix READLANE and WRITELANE lane select for VI
>
> VOP2 declares vsrc1, but VOP3 declares src1.
> We can't use the same "ins" if the operands have different names in VOP2
> and VOP3 encodings.
>
> 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/SIInstructions.td | 8 ++++----
> 2 files changed, 6 insertions(+), 6 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/SIInstructions.td b/lib/Target/R600/SIInstructions.td
> index fbd38c9..c22328b 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, SCSrc_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, SCSrc_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
More information about the llvm-commits
mailing list