<div dir="ltr">On 29 October 2013 11:20, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> writes:<br>
<br>
> On 27 October 2013 12:58, Marek Olšák <<a href="mailto:maraeo@gmail.com">maraeo@gmail.com</a>> wrote:<br>
><br>
>> On Sun, Oct 27, 2013 at 4:19 PM, Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br>
>> wrote:<br>
>> > On 20 October 2013 09:20, Marek Olšák <<a href="mailto:maraeo@gmail.com">maraeo@gmail.com</a>> wrote:<br>
>> >><br>
>> >> From: Marek Olšák <<a href="mailto:marek.olsak@amd.com">marek.olsak@amd.com</a>><br>
>> >><br>
>> >> This avoids a defect in lower_output_reads.<br>
>> >><br>
>> >> The problem is lower_output_reads treats the gl_FragData array as a<br>
>> single<br>
>> >> variable. It first redirects all output writes to a temporary variable<br>
>> >> (array)<br>
>> >> and then writes the whole temporary variable to the output, generating<br>
>> >> assignments to all elements of gl_FragData.<br>
>> ><br>
>> ><br>
>> > Can you go into more detail about why this is a problem?  At first<br>
>> glance it<br>
>> > seems like it should be fine, because failing to assign to an element of<br>
>> > gl_FragData is supposed to result in undefined data going to that<br>
>> fragment<br>
>> > output.  So lowering to a shader that does assign to that element of<br>
>> > gl_FragData seems like it should be harmless.  What am I missing here?<br>
>><br>
>> Thanks for the review. The problem is drivers cannot eliminate useless<br>
>> writes to gl_FragData, and each enabled gl_FragData output decreases<br>
>> performance. The GLSL compiler cannot eliminate the writes either,<br>
>> because gl_FragData is an array.<br>
>><br>
>> Maybe the GLSL compiler should resize arrays based on which elements<br>
>> are written, so that unused elements are not declared, but this is not<br>
>> enough for gl_FragData, where the i-th output can be written, but<br>
>> (i-1)-th output doesn't have to be.<br>
>><br>
><br>
> Ah, ok.  When I saw the word "defect", I misunderstood you to be fixing a<br>
> correctness-of-rendering bug.  As a performance optimization, I get it now.<br>
><br>
> For driver back-ends that don't need lower_output_reads (such as i915 and<br>
> i965), this optimization isn't needed.  Would you mind adding a flag to<br>
> ShaderCompilerOptions so that tgsi-based drivers can opt in to this new<br>
> optimization?  I want to make sure that the code generation of i965 and<br>
> i915 isn't affected.<br>
<br>
</div></div>Actually, if Marek has identified that applications not setting all<br>
their gl_FragData[] is a performance issue, I want to see this applied<br>
to i965, too.  This optimization would remove entire FB_WRITE opcodes<br>
for us, which could be pretty significant if applications hit this case.<br>
</blockquote></div><br></div><div class="gmail_extra">Yes, but Marek's patch won't benefit i965 even in that case.  The only effect of Marek's patch is to prevent lower_output_reads (which i965 doesn't use) from converting a shader that writes to only some elements of gl_FragData into a shader that writes to all elements of gl_FragData.<br>
<br></div><div class="gmail_extra">To optimize i965 for shaders that don't write to all elements of gl_FragData we need to do back-end work that's unrelated to Marek's patch.<br></div></div>