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

[Eric Anholt]

Paul Berry <stereotype441 at gmail.com> writes:

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

Sweet!
-------------- 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/20130314/586d774a/attachment.pgp>