[Mesa-dev] [PATCH] r300/compiler: Prevent the fragmentation of TEX blocks in the pair scheduler.

[Tom Stellard]

On Wed, Jun 02, 2010 at 11:39:57AM +0200, Nicolai Haehnle wrote:
> On Wed, Jun 2, 2010 at 6:53 AM, Tom Stellard <tstellar at gmail.com> wrote:
> > On Tue, Jun 01, 2010 at 12:00:16PM +0200, Nicolai Haehnle wrote:
> >> On Tue, Jun 1, 2010 at 7:41 AM, tstellar <tstellar at gmail.com> wrote:
> >> > From: Tom Stellard <tstellar at gmail.com>
> >> >
> >> > This fixes bug:
> >> >
> >> > https://bugs.freedesktop.org/show_bug.cgi?id=25109
> >>
> >> Is this really correct? If I understand your patch correctly, what it
> >> does is that TEX instructions that depend on earlier TEX instructions
> >> will be emitted in the same TEX group on R300. So if you have
> >> dependent texture reads like this:
> >>
> >>  MOV r0, something;
> >>  TEX r1, r0, ...;
> >>  TEX r2, r1, ...;
> >>
> >> You will now emit both TEX in the same TEX indirection block, which I
> >> thought was wrong, because the second TEX instruction will actually
> >> use the contents of r1 *before* the first TEX instruction as
> >> coordinates. At least that's how I thought the TEX hardware works:
> >>
> >> 1) Fetch all coordinates for all TEX instructions in an indirection block
> >> 2) Execute all TEX instructions in parallel
> >> 3) Store all results in the respective destination registers
> >>
> >> If my understanding is correct, then I believe your change miscompiles
> >> the above shader fragment. Can you clarify this?
> >>
> >
> > It looks like I am the one who misunderstood how TEX instructions work.
> > You are right, the patch does miscompile your example.  The shader
> > I was having problems with looked like this:
> >
> > 10: TEX temp[13].xyz, temp[12].xy__, 2D[0];
> > 11: TEX temp[12].xyz, temp[11].xy__, 2D[0];
> > 12: TEX temp[11].xyz, temp[10].xy__, 2D[0];
> > 13: TEX temp[10].xyz, temp[9].xy__, 2D[0];
> > 14: TEX temp[9].xyz, temp[8].xy__, 2D[0];
> > 15: TEX temp[8].xyz, temp[7].xy__, 2D[0];
> > 16: TEX temp[7].xyz, input[4].xy__, 2D[0];
> > 17: TEX temp[6].xyz, temp[6].xy__, 2D[0];
> > 18: TEX temp[5].xyz, temp[5].xy__, 2D[0];
> > 19: TEX temp[4].xyz, temp[4].xy__, 2D[0];
> > 20: TEX temp[3].xyz, temp[3].xy__, 2D[0];
> >
> > I think the bug is actually in the pair scheduler's dataflow analysis.
> > Currently, the pair scheduler marks #10 as a dependency of #11, which
> > would be OK if these were ALU instructions, but it shouldn't be a
> > dependency for TEX instructions.
> 
> Yes, I think what the dataflow analysis sees is that #10 reads
> temp[12] while #11 overwrites temp[12], so its belief is that #10 must
> be executed before #11. That's indeed a curious problem.
> 
> So is this done after register allocation? Because it seems to me like
> the register dealiasing should rename one of the occurences of
> temp[12] in this example...
>

Currently, the pair scheduler runs before the register allocation pass.  I have
attached the compiler output for this shader to the original bug if you
want to take a look at it:
https://bugs.freedesktop.org/show_bug.cgi?id=25109

-Tom