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

[Nicolai Haehnle]

This was supposed to go to the mailing list as well... sorry about that *sigh*

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?

cu,
Nicolai

> ---
>  .../dri/r300/compiler/radeon_pair_schedule.c       |   39 +++++++++++++-------
>  1 files changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/r300/compiler/radeon_pair_schedule.c b/src/mesa/drivers/dri/r300/compiler/radeon_pair_schedule.c
> index a279549..0641be5 100644
> --- a/src/mesa/drivers/dri/r300/compiler/radeon_pair_schedule.c
> +++ b/src/mesa/drivers/dri/r300/compiler/radeon_pair_schedule.c
> @@ -208,21 +208,32 @@ static void emit_all_tex(struct schedule_state * s, struct rc_instruction * befo
>
>        assert(s->ReadyTEX);
>
> -       /* Don't let the ready list change under us! */
> -       readytex = s->ReadyTEX;
> -       s->ReadyTEX = 0;
> -
> -       /* Node marker for R300 */
> -       struct rc_instruction * inst_begin = rc_insert_new_instruction(s->C, before->Prev);
> -       inst_begin->U.I.Opcode = RC_OPCODE_BEGIN_TEX;
> -
> -       /* Link texture instructions back in */
> -       while(readytex) {
> -               struct schedule_instruction * tex = readytex;
> -               readytex = readytex->NextReady;
> +       if(s->ReadyTEX){
> +               /* Node marker for R300 */
> +               struct rc_instruction * inst_begin =
> +                               rc_insert_new_instruction(s->C, before->Prev);
> +               inst_begin->U.I.Opcode = RC_OPCODE_BEGIN_TEX;
> +       }
>
> -               rc_insert_instruction(before->Prev, tex->Instruction);
> -               commit_instruction(s, tex);
> +       /* The purpose of this outer loop is to emit TEX instruction that
> +        * have become ready as a result of the inner loop.  This prevents
> +        * the fragmentation of TEX blocks, which for some shaders (in Civ4
> +        * for example) was creating too many texture indirections on r300
> +        * cards.
> +        */
> +       while(s->ReadyTEX){
> +               /* Don't let the ready list change under us! */
> +               readytex = s->ReadyTEX;
> +               s->ReadyTEX = 0;
> +
> +               /* Link texture instructions back in */
> +               while(readytex) {
> +                       struct schedule_instruction * tex = readytex;
> +                       readytex = readytex->NextReady;
> +
> +                       rc_insert_instruction(before->Prev, tex->Instruction);
> +                       commit_instruction(s, tex);
> +               }
>        }
>  }
>
> --
> 1.6.4.4