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

[Paul Berry]

On 12 March 2013 19:29, Eric Anholt <eric at anholt.net> wrote:

> 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)
>

Fair enough--I'll fix that.


>
> >           /* 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.
>

Hmm, now I'm curious.

Amazingly enough, gcc with -O2 is actually smart enough to use a bitfield:

_Z24_mesa_varying_slot_in_fs15gl_varying_slot:
.LFB0:
    .cfi_startproc
    cmpl    $20, %edi
    ja    .L4
    movl    $1, %edx
    movl    %edi, %ecx
    xorl    %eax, %eax
    salq    %cl, %rdx
    testl    $1175552, %edx
    je    .L4
    rep
    ret
    .p2align 4,,10
    .p2align 3
.L4:
    movl    $1, %eax
    ret
    .cfi_endproc

I'm impressed.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130313/c4ff2342/attachment.html>