<div dir="ltr">On 12 March 2013 19:29, Eric Anholt <span dir="ltr"><<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="im">Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> writes:<br>
<br>
> Now that there is no difference between the enums that represent<br>
> vertex outputs and fragment inputs, there's no need for a conversion<br>
> function. But we still need to be able to detect when a given vertex<br>
> output has no corresponding fragment input. So it is replaced by a<br>
> new function, _mesa_varying_slot_in_fs(), which tells whether the<br>
> given varying slot exists as an FS input or not.<br>
> ---<br>
> src/mesa/drivers/dri/i965/brw_fs.cpp | 12 ++++-----<br>
> src/mesa/drivers/dri/i965/brw_vs_constval.c | 13 ++++------<br>
> src/mesa/main/mtypes.h | 38 +++++++++++++----------------<br>
> 3 files changed, 27 insertions(+), 36 deletions(-)<br>
><br>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp<br>
> index 86f8cbb..ea4a56c 100644<br>
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp<br>
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp<br>
> @@ -1265,7 +1265,7 @@ fs_visitor::calculate_urb_setup()<br>
> continue;<br>
><br>
> if (c->key.vp_outputs_written & BITFIELD64_BIT(i)) {<br>
> - int fp_index = _mesa_vert_result_to_frag_attrib((gl_varying_slot) i);<br>
> + bool exists_in_fs = _mesa_varying_slot_in_fs((gl_varying_slot) i);<br>
<br>
</div>I'd rather see this call moved into the single usage in the if statement<br>
below, like has been done elsewhere (now that the function name<br>
explicitly talks about what's being tested in the "if" anyway)<br></blockquote><div><br></div><div>Fair enough--I'll fix that.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="im"><br>
> /* The back color slot is skipped when the front color is<br>
> * also written to. In addition, some slots can be<br>
> @@ -1273,8 +1273,8 @@ fs_visitor::calculate_urb_setup()<br>
> * fragment shader. So the register number must always be<br>
> * incremented, mapped or not.<br>
> */<br>
> - if (fp_index >= 0)<br>
> - urb_setup[fp_index] = urb_next;<br>
> + if (exists_in_fs)<br>
> + urb_setup[i] = urb_next;<br>
> urb_next++;<br>
<br>
<br>
</div><div><div class="h5">> /**<br>
> - * Convert from a gl_varying_slot value for a vertex output to the<br>
> - * corresponding gl_frag_attrib.<br>
> - *<br>
> - * Varying output values which have no corresponding gl_frag_attrib<br>
> - * (VARYING_SLOT_PSIZ, VARYING_SLOT_BFC0, VARYING_SLOT_BFC1, and<br>
> - * VARYING_SLOT_EDGE) are converted to a value of -1.<br>
> + * Determine if the given gl_varying_slot appears in the fragment shader.<br>
> */<br>
> -static inline int<br>
> -_mesa_vert_result_to_frag_attrib(gl_varying_slot vert_result)<br>
> +static inline GLboolean<br>
> +_mesa_varying_slot_in_fs(gl_varying_slot slot)<br>
> {<br>
> - if (vert_result <= VARYING_SLOT_TEX7)<br>
> - return vert_result;<br>
> - else if (vert_result < VARYING_SLOT_CLIP_DIST0)<br>
> - return -1;<br>
> - else if (vert_result <= VARYING_SLOT_CLIP_DIST1)<br>
> - return vert_result;<br>
> - else if (vert_result < VARYING_SLOT_VAR0)<br>
> - return -1;<br>
> - else<br>
> - return vert_result;<br>
> + switch (slot) {<br>
> + case VARYING_SLOT_PSIZ:<br>
> + case VARYING_SLOT_BFC0:<br>
> + case VARYING_SLOT_BFC1:<br>
> + case VARYING_SLOT_EDGE:<br>
> + case VARYING_SLOT_CLIP_VERTEX:<br>
> + case VARYING_SLOT_LAYER:<br>
> + return GL_FALSE;<br>
> + default:<br>
> + return GL_TRUE;<br>
> + }<br>
> }<br>
<br>
</div></div>I bet the compiler does a big switch statement instead of doing what we<br>
could do better with bitfields. Not a blocker, just a potential<br>
improvement.<br></blockquote><div><br></div><div>Hmm, now I'm curious.<br><br></div><div>Amazingly enough, gcc with -O2 is actually smart enough to use a bitfield:<br><br>_Z24_mesa_varying_slot_in_fs15gl_varying_slot:<br>
.LFB0:<br> .cfi_startproc<br> cmpl $20, %edi<br> ja .L4<br> movl $1, %edx<br> movl %edi, %ecx<br> xorl %eax, %eax<br> salq %cl, %rdx<br> testl $1175552, %edx<br> je .L4<br>
rep<br> ret<br> .p2align 4,,10<br> .p2align 3<br>.L4:<br> movl $1, %eax<br> ret<br> .cfi_endproc<br><br></div>I'm impressed.<br></div></div></div>