[Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

[Ilia Mirkin]

I had trouble getting these to apply, perhaps they were meant to go on
top of something else. Anyways, should be fairly easy for you to test
out with llvmpipe.

On Tue, Aug 23, 2016 at 2:31 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> Francisco Jerez <currojerez at riseup.net> writes:
>
>> Ilia Mirkin <imirkin at alum.mit.edu> writes:
>>
>>> On Tue, Aug 23, 2016 at 12:18 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>>> Ilia Mirkin <imirkin at alum.mit.edu> writes:
>>>>
>>>>> On Tue, Aug 23, 2016 at 12:05 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>>>>> Ilia Mirkin <imirkin at alum.mit.edu> writes:
>>>>>>
>>>>>>> On Mon, Aug 22, 2016 at 10:55 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>>>>>>>> Ilia Mirkin <imirkin at alum.mit.edu> writes:
>>>>>>>>
>>>>>>>>> On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>>>>>>>>>> gl_SecondaryFragColorEXT should have the same location as gl_FragColor
>>>>>>>>>> for the secondary fragment color to be replicated to all fragment
>>>>>>>>>> outputs.  The incorrect location of gl_SecondaryFragColorEXT would
>>>>>>>>>> cause the linker to mark both FRAG_RESULT_COLOR and FRAG_RESULT_DATA0
>>>>>>>>>> as being written to, which isn't allowed by the spec and would
>>>>>>>>>> ultimately lead to an assertion failure in
>>>>>>>>>> fs_visitor::emit_fb_writes() on my i965-fb-fetch branch.
>>>>>>>>>
>>>>>>>>> My recollection was that it didn't work with COLOR for "stupid"
>>>>>>>>> reasons. Can you confirm that
>>>>>>>>> bin/arb_blend_func_extended-fbo-extended-blend-pattern_gles2 -auto
>>>>>>>>> passes with this patch?
>>>>>>>>>
>>>>>>>> Yes, it does, in fact
>>>>>>>> arb_blend_func_extended-fbo-extended-blend-pattern_gles2 hits the i965
>>>>>>>> assertion failure I mentioned above unless this patch is applied.
>>>>>>>
>>>>>>> This causes the test in question to fail on nouveau... the TGSI shader
>>>>>>> generated starts with
>>>>>>>
>>>>>>> FRAG
>>>>>>> PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
>>>>>>> DCL IN[0], POSITION, LINEAR
>>>>>>> DCL OUT[0], SAMPLEMASK
>>>>>>
>>>>>> Heh, this smells a lot like the bug fixed in PATCH 1, but somewhere in
>>>>>> the mesa state tracker.  st_glsl_to_tgsi.cpp:2422 does:
>>>>>>
>>>>>> | entry = new(mem_ctx) variable_storage(var,
>>>>>> |                                       PROGRAM_OUTPUT,
>>>>>> |                                       var->data.location
>>>>>> |                                       + var->data.index);
>>>>>>
>>>>>> which is obviously bogus, e.g. for var->data.location ==
>>>>>> FRAG_RESULT_COLOR and var->data.index == 1 you get
>>>>>> FRAG_RESULT_SAMPLE_MASK which explains the sample mask declaration
>>>>>> above.
>>>>>
>>>>> Right, because having FRAG_RESULT_COLOR and index != 0 was never
>>>>> possible prior to this. That might be why Ryan stuck it into
>>>>> FRAG_RESULT_DATA0 [I may have been the one to suggest that].
>>>>
>>>> Heh, so I guess that's the "stupid" reason you were referring to,
>>>> working around this mesa state tracker bug in the GLSL front-end.
>>>
>>> Right. Or another way of looking at it, FRAG_RESULT_COLOR + index != 0
>>> is illegal,
>>
>> That would be an acceptable limitation if it were taken into account
>> consistently (which would imply among other things binding gl_FragColor
>> to FRAG_RESULT_DATA0 if gl_SecondaryFragColor is written).  Otherwise
>> you will be telling the linker and the back-end that both
>> FRAG_RESULT_COLOR and FRAG_RESULT_DATA0 are being written, which is
>> illegal.  The i965 has been misbehaving since forever because of this,
>> but we just didn't notice until I added that assertion because the only
>> symptom is increased shader recompiles.
>>
>>> (as it would, among other things, imply multi-rt support for
>>> dual-source blending)
>>
>> FRAG_RESULT_COLOR + index != 0 doesn't imply multi-rt support, it's a
>> requirement for multi-rt support, which the GLSL spec makes room for and
>> some drivers in this tree could potentially support if the GLSL IR
>> representation of dual-source blending wasn't deliberately inconsistent.
>>
>>> and the former code was making the GLSL fe not emit the illegal
>>> combination.
>>>
>> I started digging into this and found out that that's not the only way
>> the GLSL-to-TGSI pass relies on a GLSL bug: The output semantic array
>> calculation in st_translate_fragment_program() relies on there being
>> multiple locations marked as written in the shader's OutputsWritten set
>> in the dual-source blending case (IOW you're relying on the bug fixed by
>> the previous patch too).  GLSL is not TGSI, in GLSL the secondary and
>> primary colors of a dual-source-blended output have the same location,
>> so you cannot expect there will be multiple elements present in a bitset
>> of locations.
>>
>>> Whichever way you look at it, breaking st/mesa isn't a great option.
>>
>> Then you may want to take a look at the attached patches in addition.
>> They are untested and I have close to zero experience with the
>> GLSL-to-TGSI pass, so they may break st/mesa after all.
>>
>
> Oops, I attached the wrong 0001-glsl.*.patch file, these are the right
> patches.
>
>>>   -ilia
>>
>