[Mesa-dev] [PATCH] tgsi: Recognize RET in main for tgsi_transform

[Jose Fonseca]

It looks good to me.

Though I think it would be useful to track nesting of subroutines and 
other control flow separately, and throw a warning when we ignore RETs 
on the main subroutine that are not on the top-most level of the control 
flow stack.  So it's easier to spot when that problem surfaces.

Reviewed-by: Jose Fonseca <jfonseca at vmware.com>


On 13/02/18 04:10, sroland at vmware.com wrote:
> From: Roland Scheidegger <sroland at vmware.com>
> 
> Shaders coming from dx10 state trackers have a RET before the END.
> And the epilog needs to be placed before the RET (otherwise it will
> get ignored).
> Hence figure out if a RET is in main, in this case we'll place
> the epilog there rather than before the END.
> (At a closer look, there actually seem to be problems with control
> flow in general with output redirection, that would need another
> look. It's enough however to fix draw's aa line emulation in some
> internal bug - lines tend to be drawn with trivial shaders, moving
> either a constant color or a vertex color directly to the output).
> ---
>   src/gallium/auxiliary/tgsi/tgsi_transform.c | 50 ++++++++++++++++++++++++++---
>   1 file changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/tgsi/tgsi_transform.c b/src/gallium/auxiliary/tgsi/tgsi_transform.c
> index ffdad13..94d872c 100644
> --- a/src/gallium/auxiliary/tgsi/tgsi_transform.c
> +++ b/src/gallium/auxiliary/tgsi/tgsi_transform.c
> @@ -110,6 +110,8 @@ tgsi_transform_shader(const struct tgsi_token *tokens_in,
>   {
>      uint procType;
>      boolean first_instruction = TRUE;
> +   boolean epilog_emitted = FALSE;
> +   int stack_size = 0;
>   
>      /* input shader */
>      struct tgsi_parse_context parse;
> @@ -166,22 +168,60 @@ tgsi_transform_shader(const struct tgsi_token *tokens_in,
>            {
>               struct tgsi_full_instruction *fullinst
>                  = &parse.FullToken.FullInstruction;
> +            unsigned opcode = fullinst->Instruction.Opcode;
>   
>               if (first_instruction && ctx->prolog) {
>                  ctx->prolog(ctx);
>               }
>   
> -            /* XXX Note: we may also want to look for a main/top-level
> -             * TGSI_OPCODE_RET instruction in the future.
> +            /*
> +             * XXX Note: we handle the case of ret in main.
> +             * However, the output redirections done by transform
> +             * have their limits with control flow and will generally
> +             * not work correctly. e.g.
> +             * if (cond) {
> +             *    oColor = x;
> +             *    ret;
> +             * }
> +             * oColor = y;
> +             * end;
> +             * If the color output is redirected to a temp and modified
> +             * by a transform, this will not work (the oColor assignment
> +             * in the conditional will never make it to the actual output).
>                */
> -            if (fullinst->Instruction.Opcode == TGSI_OPCODE_END
> -                && ctx->epilog) {
> +            if ((opcode == TGSI_OPCODE_END ||
> +                 (opcode == TGSI_OPCODE_RET && stack_size == 0))
> +                && ctx->epilog && !epilog_emitted) {
>                  /* Emit caller's epilog */
>                  ctx->epilog(ctx);
> -               /* Emit END */
> +               epilog_emitted = TRUE;
> +               /* Emit END (or RET) */
> +               if (opcode == TGSI_OPCODE_END) {
> +                  assert(stack_size == 0);
> +               }
>                  ctx->emit_instruction(ctx, fullinst);
>               }
>               else {
> +               switch (opcode) {
> +               case TGSI_OPCODE_IF:
> +               case TGSI_OPCODE_UIF:
> +               case TGSI_OPCODE_SWITCH:
> +               case TGSI_OPCODE_BGNLOOP:
> +               case TGSI_OPCODE_CAL:
> +                  stack_size++;
> +                  break;
> +               case TGSI_OPCODE_ENDIF:
> +               case TGSI_OPCODE_ENDSWITCH:
> +               case TGSI_OPCODE_ENDLOOP:
> +               case TGSI_OPCODE_ENDSUB:
> +                  assert(stack_size > 0);
> +                  stack_size--;
> +                  break;
> +               case TGSI_OPCODE_BGNSUB:
> +               case TGSI_OPCODE_RET:
> +               default:
> +                  break;
> +               }
>                  if (ctx->transform_instruction)
>                     ctx->transform_instruction(ctx, fullinst);
>                  else
>