<div dir="ltr">On 7 April 2013 12:01, Jordan Justen <span dir="ltr"><<a href="mailto:jljusten@gmail.com" target="_blank">jljusten@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div>On Sun, Apr 7, 2013 at 11:42 AM, Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>> wrote:<br>
> On 7 April 2013 11:12, Jordan Justen <<a href="mailto:jljusten@gmail.com" target="_blank">jljusten@gmail.com</a>> wrote:<br>
>><br>
>> On Sat, Apr 6, 2013 at 7:49 PM, Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>><br>
>> wrote:<br>
>> > +   if (consumer_var == NULL) {<br>
>> > +      /* Since there is no consumer_var, the interpolation type of this<br>
>> > +       * varying cannot possibly affect rendering.  Also, since the GL<br>
>> > spec<br>
>> > +       * only requires integer varyings to be "flat" when they are<br>
>> > fragment<br>
>> > +       * shader inputs, it is possible that this variable is non-flat<br>
>> > and is<br>
>> > +       * (or contains) an integer.<br>
>> > +       *<br>
>> > +       * lower_packed_varyings requires all integer varyings to flat,<br>
>> > +       * regardless of where they appear.  We can trivially satisfy<br>
>> > that<br>
>> > +       * requirement by changing the interpolation type to flat here.<br>
>> > +       */<br>
>> > +      producer_var->centroid = false;<br>
>> > +      producer_var->interpolation = INTERP_QUALIFIER_FLAT;<br>
>><br>
>> Should we check if producer_var's type is an integer?<br>
><br>
> Yeah, that's a good point.  It shouldn't affect correctness,<br>
<br>
</div>It isn't possible to have a non-integer scenario of this where we<br>
wouldn't want to override the interpolation type?<br></blockquote><div><br></div><div>Hmm, I can't think of any such scenario.  It seems to me that as long as consumer_var == NULL, that means that the varying isn't being consumed by the fragment shader, so it isn't getting interpolated (or if it is, the interpolation result is being ignored).  So the only reason the interpolation type should matter at this point is in its effect on lower_packed_varyings.  Of course I could still be missing something :)<br>
<br></div><div>I'll still go ahead and add the int check just to be on the safe side.<br></div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
> but it will<br>
> make the patch less invasive, and that seems worthwhile since I'm hoping<br>
> this can be cherry-picked back to 9.1.  I'll follow up with a v2 of the<br>
> series.<br>
><br>
>> Could this cause issues if the producer stage is later linked to<br>
>> another consumer?<br>
><br>
> That shouldn't be a problem, since at this stage of linking, we are<br>
> operating on a copy of the IR that is private to this particular linked<br>
> program.<br>
<br>
</div>Okay, well, based on that and adding the int check above, for the series:<br>
Reviewed-by: Jordan Justen <<a href="mailto:jordan.l.justen@intel.com" target="_blank">jordan.l.justen@intel.com</a>><br>
</blockquote></div><br></div></div>