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

[Michel Dänzer]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-R600-SI-Fix-broken-encoding-of-DS_WRITE_B32.patch
Type: text/x-patch
Size: 5029 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130816/61ba0d9a/attachment.bin>