[Mesa-dev] radeonsi: LLVM r187139 broke some piglit tests

Tom Stellard tom at stellard.net
Fri Aug 16 06:45:08 PDT 2013


On Fri, Aug 16, 2013 at 03:36:38PM +0200, Michel Dänzer wrote:
> On Don, 2013-08-15 at 13:50 -0700, Tom Stellard wrote:
> > On Thu, Aug 15, 2013 at 07:50:10PM +0200, Michel Dänzer wrote:
> > > On Don, 2013-08-15 at 09:16 -0700, Tom Stellard wrote:
> > > > On Thu, Aug 15, 2013 at 08:22:39AM -0700, Tom Stellard wrote:
> > > > > On Thu, Aug 15, 2013 at 11:55:36AM +0200, Michel Dänzer wrote:
> > > > > > On Fre, 2013-08-02 at 17:58 +0200, Michel Dänzer wrote:
> > > > > > > On Mit, 2013-07-31 at 08:42 -0700, Tom Stellard wrote:
> > > > > > > > On Wed, Jul 31, 2013 at 01:04:01PM +0200, Michel Dänzer wrote:
> > > > > > > > > 
> > > > > > > > > LLVM revision 187139 ('Allocate local registers in order for optimal
> > > > > > > > > coloring.') broke some derivative related piglit tests with the radeonsi
> > > > > > > > > driver. 
> > > > > > > > > 
> > > > > > > > > I'm attaching a diff between the bad and good generated code (as printed
> > > > > > > > > with RADEON_DUMP_SHADERS=1) for the glsl-derivs test. The only
> > > > > > > > > difference I can see is in which registers are used in which order.
> > > > > > > > > 
> > > > > > > > > I wonder if we might be missing S_WAITCNT after DS_READ/WRITE
> > > > > > > > > instructions in some cases, but I haven't spotted any candidates for
> > > > > > > > > that in the bad code which aren't there in the good code as well. Can
> > > > > > > > > anyone else spot something I've missed?
> > > > > > > > 
> > > > > > > > Shouldn't we be using the S_BARRIER instruction to keep the threads in sync?
> > > > > > > 
> > > > > > > Doesn't seem to help unfortunately, but thanks for the good suggestion.
> > > > > > 
> > > > > > I found one thing going wrong: DS_WRITE_B32 ends up using a VGPR
> > > > > > register number instead of the $gds operand for encoding the GDS field
> > > > > > (the asm output from llc even shows the VGPR name). If the VGPR number
> > > > > > happens to be odd (i.e. to have the least significant bit set), the
> > > > > > shader ends up writing to GDS instead of LDS.
> > > > > >
> > > > > 
> > > > > Ouch, that's a pretty bad bug.
> > > > > 
> > > > > > But I have no idea why this is happening, or how to fix it. :(
> > > > > > 
> > > > > > 
> > > > > 
> > > > > I can take a look at it.
> > > > 
> > > > The attached patch should fix the problem, can you test?
> > > 
> > > Thanks for finding my silly mistake.
> > > 
> > > However, I'd like to preserve the ability to use these instructions for
> > > GDS access, and the logic in SIInsertWaits::getHwCounts() only really
> > > makes sense for SMRD anyway.
> > > 
> > > How about this patch instead? It fixes the piglit regressions that
> > > prompted me to start this thread.
> 
> [...]
> 
> > > diff --git a/lib/Target/R600/SIInsertWaits.cpp b/lib/Target/R600/SIInsertWaits.cpp
> > > index ba202e3..200e064 100644
> > > --- a/lib/Target/R600/SIInsertWaits.cpp
> > > +++ b/lib/Target/R600/SIInsertWaits.cpp
> > > @@ -134,14 +134,20 @@ Counters SIInsertWaits::getHwCounts(MachineInstr &MI) {
> > >    // LGKM may uses larger values
> > >    if (TSFlags & SIInstrFlags::LGKM_CNT) {
> > >  
> > > -    MachineOperand &Op = MI.getOperand(0);
> > > -    if (!Op.isReg())
> > > -      Op = MI.getOperand(1);
> > > -    assert(Op.isReg() && "First LGKM operand must be a register!");
> > > -
> > > -    unsigned Reg = Op.getReg();
> > > -    unsigned Size = TRI->getMinimalPhysRegClass(Reg)->getSize();
> > > -    Result.Named.LGKM = Size > 4 ? 2 : 1;
> > > +    if (MI.getNumOperands() == 3) {
> > 
> > We should add a TSFlag for SMRD like we do for MIMG and add a helper
> > function isSMRD to SIInstrInfo and use it here.  The number of operands
> > for instructions tends to change from time to time.
> 
> Like this?
> 
> 
> -- 
> Earthling Michel Dänzer           |                   http://www.amd.com
> Libre software enthusiast         |          Debian, X and DRI developer

> From c1d6b0f3d9cfcf2255257fdc87c748a46f799935 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Michel=20D=C3=A4nzer?= <michel.daenzer at amd.com>
> Date: Thu, 15 Aug 2013 19:43:02 +0200
> Subject: [PATCH v2] R600/SI: Fix broken encoding of DS_WRITE_B32
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The logic in SIInsertWaits::getHwCounts() only really made sense for SMRD
> instructions, and trying to shoehorn it into handling DS_WRITE_B32 caused
> it to corrupt the encoding of that by clobbering the first operand with
> the second one.
> 
> Undo that damage and only apply the SMRD logic to that.
> 
> Fixes some derivates related piglit regressions with radeonsi.
> 
> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
Reviewed-by: Tom Stellard <thomas.stellard at amd.com>
> ---
> 
> v2: Use SIInstrFlags to test for SMRD instructions.
> 
>  lib/Target/R600/SIDefines.h       |  3 ++-
>  lib/Target/R600/SIInsertWaits.cpp | 21 +++++++++++++--------
>  lib/Target/R600/SIInstrFormats.td |  3 +++
>  lib/Target/R600/SIInstrInfo.cpp   |  4 ++++
>  lib/Target/R600/SIInstrInfo.h     |  1 +
>  test/CodeGen/R600/local-memory.ll |  4 ++--
>  6 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/Target/R600/SIDefines.h b/lib/Target/R600/SIDefines.h
> index 572ed6a..f5445ad 100644
> --- a/lib/Target/R600/SIDefines.h
> +++ b/lib/Target/R600/SIDefines.h
> @@ -13,7 +13,8 @@
>  
>  namespace SIInstrFlags {
>  enum {
> -  MIMG = 1 << 3
> +  MIMG = 1 << 3,
> +  SMRD = 1 << 4
>  };
>  }
>  
> diff --git a/lib/Target/R600/SIInsertWaits.cpp b/lib/Target/R600/SIInsertWaits.cpp
> index ba202e3..7e42fb7 100644
> --- a/lib/Target/R600/SIInsertWaits.cpp
> +++ b/lib/Target/R600/SIInsertWaits.cpp
> @@ -134,14 +134,19 @@ Counters SIInsertWaits::getHwCounts(MachineInstr &MI) {
>    // LGKM may uses larger values
>    if (TSFlags & SIInstrFlags::LGKM_CNT) {
>  
> -    MachineOperand &Op = MI.getOperand(0);
> -    if (!Op.isReg())
> -      Op = MI.getOperand(1);
> -    assert(Op.isReg() && "First LGKM operand must be a register!");
> -
> -    unsigned Reg = Op.getReg();
> -    unsigned Size = TRI->getMinimalPhysRegClass(Reg)->getSize();
> -    Result.Named.LGKM = Size > 4 ? 2 : 1;
> +    if (TII->isSMRD(MI.getOpcode())) {
> +
> +      MachineOperand &Op = MI.getOperand(0);
> +      assert(Op.isReg() && "First LGKM operand must be a register!");
> +
> +      unsigned Reg = Op.getReg();
> +      unsigned Size = TRI->getMinimalPhysRegClass(Reg)->getSize();
> +      Result.Named.LGKM = Size > 4 ? 2 : 1;
> +
> +    } else {
> +      // DS
> +      Result.Named.LGKM = 1;
> +    }
>  
>    } else {
>      Result.Named.LGKM = 0;
> diff --git a/lib/Target/R600/SIInstrFormats.td b/lib/Target/R600/SIInstrFormats.td
> index cd1bbcd..9576c05 100644
> --- a/lib/Target/R600/SIInstrFormats.td
> +++ b/lib/Target/R600/SIInstrFormats.td
> @@ -18,11 +18,13 @@ class InstSI <dag outs, dag ins, string asm, list<dag> pattern> :
>    field bits<1> EXP_CNT = 0;
>    field bits<1> LGKM_CNT = 0;
>    field bits<1> MIMG = 0;
> +  field bits<1> SMRD = 0;
>  
>    let TSFlags{0} = VM_CNT;
>    let TSFlags{1} = EXP_CNT;
>    let TSFlags{2} = LGKM_CNT;
>    let TSFlags{3} = MIMG;
> +  let TSFlags{4} = SMRD;
>  }
>  
>  class Enc32 <dag outs, dag ins, string asm, list<dag> pattern> :
> @@ -142,6 +144,7 @@ class SMRD <bits<5> op, bits<1> imm, dag outs, dag ins, string asm,
>    let Inst{31-27} = 0x18; //encoding
>  
>    let LGKM_CNT = 1;
> +  let SMRD = 1;
>  }
>  
>  //===----------------------------------------------------------------------===//
> diff --git a/lib/Target/R600/SIInstrInfo.cpp b/lib/Target/R600/SIInstrInfo.cpp
> index 9bb4ad9..2719ea2 100644
> --- a/lib/Target/R600/SIInstrInfo.cpp
> +++ b/lib/Target/R600/SIInstrInfo.cpp
> @@ -229,6 +229,10 @@ int SIInstrInfo::isMIMG(uint16_t Opcode) const {
>    return get(Opcode).TSFlags & SIInstrFlags::MIMG;
>  }
>  
> +int SIInstrInfo::isSMRD(uint16_t Opcode) const {
> +  return get(Opcode).TSFlags & SIInstrFlags::SMRD;
> +}
> +
>  //===----------------------------------------------------------------------===//
>  // Indirect addressing callbacks
>  //===----------------------------------------------------------------------===//
> diff --git a/lib/Target/R600/SIInstrInfo.h b/lib/Target/R600/SIInstrInfo.h
> index 8d24ab4..87b8063 100644
> --- a/lib/Target/R600/SIInstrInfo.h
> +++ b/lib/Target/R600/SIInstrInfo.h
> @@ -48,6 +48,7 @@ public:
>  
>    virtual bool isSafeToMoveRegClassDefs(const TargetRegisterClass *RC) const;
>    int isMIMG(uint16_t Opcode) const;
> +  int isSMRD(uint16_t Opcode) const;
>  
>    virtual int getIndirectIndexBegin(const MachineFunction &MF) const;
>  
> diff --git a/test/CodeGen/R600/local-memory.ll b/test/CodeGen/R600/local-memory.ll
> index 5458fb9..9ebb769 100644
> --- a/test/CodeGen/R600/local-memory.ll
> +++ b/test/CodeGen/R600/local-memory.ll
> @@ -13,7 +13,7 @@
>  ; SI-CHECK-NEXT: .long 32768
>  
>  ; EG-CHECK: LDS_WRITE
> -; SI-CHECK: DS_WRITE_B32
> +; SI-CHECK: DS_WRITE_B32 0
>  
>  ; GROUP_BARRIER must be the last instruction in a clause
>  ; EG-CHECK: GROUP_BARRIER
> @@ -21,7 +21,7 @@
>  ; SI-CHECK: S_BARRIER
>  
>  ; EG-CHECK: LDS_READ_RET
> -; SI-CHECK: DS_READ_B32
> +; SI-CHECK: DS_READ_B32 {{VGPR[0-9]+}}, 0
>  
>  define void @local_memory(i32 addrspace(1)* %out) {
>  entry:
> -- 
> 1.8.4.rc2
> 



More information about the mesa-dev mailing list