Paul Berry <stereotype...@gmail.com> writes:
> On 12 March 2013 19:29, Eric Anholt <e...@anholt.net> wrote:
>
>> Paul Berry <stereotype...@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!
pgp2oql_H3moI.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
