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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
