[Mesa-dev] [PATCH 10/12] Get rid of _mesa_vert_result_to_frag_attrib().

[Eric Anholt]

Paul Berry <stereotype441 at gmail.com> writes:

> Now that there is no difference between the enums that represent
> vertex outputs and fragment inputs, there's no need for a conversion
> function.  But we still need to be able to detect when a given vertex
> output has no corresponding fragment input.  So it is replaced by a
> new function, _mesa_varying_slot_in_fs(), which tells whether the
> given varying slot exists as an FS input or not.
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp        | 12 ++++-----
>  src/mesa/drivers/dri/i965/brw_vs_constval.c | 13 ++++------
>  src/mesa/main/mtypes.h                      | 38 +++++++++++++----------------
>  3 files changed, 27 insertions(+), 36 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 86f8cbb..ea4a56c 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -1265,7 +1265,7 @@ fs_visitor::calculate_urb_setup()
>              continue;
>  
>  	 if (c->key.vp_outputs_written & BITFIELD64_BIT(i)) {
> -	    int fp_index = _mesa_vert_result_to_frag_attrib((gl_varying_slot) i);
> +            bool exists_in_fs = _mesa_varying_slot_in_fs((gl_varying_slot) i);

I'd rather see this call moved into the single usage in the if statement
below, like has been done elsewhere (now that the function name
explicitly talks about what's being tested in the "if" anyway)

>  	    /* The back color slot is skipped when the front color is
>  	     * also written to.  In addition, some slots can be
> @@ -1273,8 +1273,8 @@ fs_visitor::calculate_urb_setup()
>  	     * fragment shader.  So the register number must always be
>  	     * incremented, mapped or not.
>  	     */
> -	    if (fp_index >= 0)
> -	       urb_setup[fp_index] = urb_next;
> +	    if (exists_in_fs)
> +	       urb_setup[i] = urb_next;
>              urb_next++;


>  /**
> - * Convert from a gl_varying_slot value for a vertex output to the
> - * corresponding gl_frag_attrib.
> - *
> - * Varying output values which have no corresponding gl_frag_attrib
> - * (VARYING_SLOT_PSIZ, VARYING_SLOT_BFC0, VARYING_SLOT_BFC1, and
> - * VARYING_SLOT_EDGE) are converted to a value of -1.
> + * Determine if the given gl_varying_slot appears in the fragment shader.
>   */
> -static inline int
> -_mesa_vert_result_to_frag_attrib(gl_varying_slot vert_result)
> +static inline GLboolean
> +_mesa_varying_slot_in_fs(gl_varying_slot slot)
>  {
> -   if (vert_result <= VARYING_SLOT_TEX7)
> -      return vert_result;
> -   else if (vert_result < VARYING_SLOT_CLIP_DIST0)
> -      return -1;
> -   else if (vert_result <= VARYING_SLOT_CLIP_DIST1)
> -      return vert_result;
> -   else if (vert_result < VARYING_SLOT_VAR0)
> -      return -1;
> -   else
> -      return vert_result;
> +   switch (slot) {
> +   case VARYING_SLOT_PSIZ:
> +   case VARYING_SLOT_BFC0:
> +   case VARYING_SLOT_BFC1:
> +   case VARYING_SLOT_EDGE:
> +   case VARYING_SLOT_CLIP_VERTEX:
> +   case VARYING_SLOT_LAYER:
> +      return GL_FALSE;
> +   default:
> +      return GL_TRUE;
> +   }
>  }

I bet the compiler does a big switch statement instead of doing what we
could do better with bitfields.  Not a blocker, just a potential
improvement.

Other than that, I'm glad to see this series happen.

Reviewed-by: Eric Anholt <eric at anholt.net>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130312/0a83cef3/attachment-0001.pgp>