[Mesa-dev] [PATCH] gallivm: fix return opcode handling in main function of a shader

[Jose Fonseca]

Looks good to me.  Please add all necessary test cases to cover all these code paths.

Good stuff guys. Very subtle code indeed.

Jose

----- Original Message -----
> From: Roland Scheidegger <sroland at vmware.com>
> 
> If we're in some conditional or loop we must not return, or the code
> after the condition is never executed.
> (v2): And, we also can't just continue as nothing happened, since the
> mask update code would later check if we actually have a mask, so we
> need to remember that there was a return in main where we didn't exit
> (to illustrate this, a ret in a if clause would cause a mask update
> which is still ok as we're in a conditional, but after the endif the
> mask update code would drop the mask hence bringing execution back to
> pixels which should have their execution mask set to zero by the ret).
> Thanks to Christoph Bumiller for figuring this out.
> 
> This fixes https://bugs.freedesktop.org/show_bug.cgi?id=62357.
> 
> Note: This is a candidate for the stable branches.
> ---
>  src/gallium/auxiliary/gallivm/lp_bld_tgsi.h     |    1 +
>  src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c |   20 +++++++++++++++++---
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi.h
> b/src/gallium/auxiliary/gallivm/lp_bld_tgsi.h
> index dac97c3..6e65e12 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi.h
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi.h
> @@ -243,6 +243,7 @@ struct lp_exec_mask {
>     struct lp_build_context *bld;
>  
>     boolean has_mask;
> +   boolean ret_in_main;
>  
>     LLVMTypeRef int_vec_type;
>  
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
> b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
> index 0dc26b5..965255a 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
> @@ -73,6 +73,7 @@ static void lp_exec_mask_init(struct lp_exec_mask *mask,
> struct lp_build_context
>  
>     mask->bld = bld;
>     mask->has_mask = FALSE;
> +   mask->ret_in_main = FALSE;
>     mask->cond_stack_size = 0;
>     mask->loop_stack_size = 0;
>     mask->call_stack_size = 0;
> @@ -108,7 +109,7 @@ static void lp_exec_mask_update(struct lp_exec_mask
> *mask)
>     } else
>        mask->exec_mask = mask->cond_mask;
>  
> -   if (mask->call_stack_size) {
> +   if (mask->call_stack_size || mask->ret_in_main) {
>        mask->exec_mask = LLVMBuildAnd(builder,
>                                       mask->exec_mask,
>                                       mask->ret_mask,
> @@ -117,7 +118,8 @@ static void lp_exec_mask_update(struct lp_exec_mask
> *mask)
>  
>     mask->has_mask = (mask->cond_stack_size > 0 ||
>                       mask->loop_stack_size > 0 ||
> -                     mask->call_stack_size > 0);
> +                     mask->call_stack_size > 0 ||
> +                     mask->ret_in_main);
>  }
>  
>  static void lp_exec_mask_cond_push(struct lp_exec_mask *mask,
> @@ -348,11 +350,23 @@ static void lp_exec_mask_ret(struct lp_exec_mask *mask,
> int *pc)
>     LLVMBuilderRef builder = mask->bld->gallivm->builder;
>     LLVMValueRef exec_mask;
>  
> -   if (mask->call_stack_size == 0) {
> +   if (mask->cond_stack_size == 0 &&
> +       mask->loop_stack_size == 0 &&
> +       mask->call_stack_size == 0) {
>        /* returning from main() */
>        *pc = -1;
>        return;
>     }
> +
> +   if (mask->call_stack_size == 0) {
> +      /*
> +       * This requires special handling since we need to ensure
> +       * we don't drop the mask even if we have no call stack
> +       * (e.g. after a ret in a if clause after the endif)
> +       */
> +      mask->ret_in_main = TRUE;
> +   }
> +
>     exec_mask = LLVMBuildNot(builder,
>                              mask->exec_mask,
>                              "ret");
> --
> 1.7.9.5
>