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

Submitted by Francisco Jerez on Aug. 23, 2016, 6:31 p.m.

Details

Message ID 87h9ab9w2d.fsf@riseup.net
State New
Headers show
Series "Series without cover letter" ( rev: 3 ) in Mesa

Not browsing as part of any series.

Commit Message

Francisco Jerez Aug. 23, 2016, 6:31 p.m.
Francisco Jerez <currojerez@riseup.net> writes:

> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>
>> On Tue, Aug 23, 2016 at 12:18 AM, Francisco Jerez <currojerez@riseup.net> wrote:
>>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>>
>>>> On Tue, Aug 23, 2016 at 12:05 AM, Francisco Jerez <currojerez@riseup.net> wrote:
>>>>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>>>>
>>>>>> On Mon, Aug 22, 2016 at 10:55 PM, Francisco Jerez <currojerez@riseup.net> wrote:
>>>>>>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>>>>>>
>>>>>>>> On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez <currojerez@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
>

Patch hide | download patch | download mbox

From 4b6657e057a1e0af4dd7adec84ae31fe89e0b135 Mon Sep 17 00:00:00 2001
From: Francisco Jerez <currojerez@riseup.net>
Date: Tue, 23 Aug 2016 11:18:19 -0700
Subject: [PATCH 2/2] st/glsl_to_tgsi: Translate fragment shader secondary
 outputs to TGSI outputs.

---
 src/mesa/state_tracker/st_glsl_to_tgsi.cpp |  5 +++--
 src/mesa/state_tracker/st_program.c        | 16 ++++++++++------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
index 5a9cadc..7d8228c 100644
--- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
+++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
@@ -2419,7 +2419,8 @@  glsl_to_tgsi_visitor::visit(ir_dereference_variable *ir)
             entry = new(mem_ctx) variable_storage(var,
                                                   PROGRAM_OUTPUT,
                                                   var->data.location
-                                                  + var->data.index);
+                                                  + FRAG_RESULT_MAX *
+                                                    var->data.index);
          }
          this->variables.push_tail(entry);
          break;
@@ -5367,7 +5368,7 @@  dst_register(struct st_translate *t, gl_register_file file, unsigned index,
    case PROGRAM_OUTPUT:
       if (!array_id) {
          if (t->procType == PIPE_SHADER_FRAGMENT)
-            assert(index < FRAG_RESULT_MAX);
+            assert(index < 2 * FRAG_RESULT_MAX);
          else if (t->procType == PIPE_SHADER_TESS_CTRL ||
                   t->procType == PIPE_SHADER_TESS_EVAL)
             assert(index < VARYING_SLOT_TESS_MAX);
diff --git a/src/mesa/state_tracker/st_program.c b/src/mesa/state_tracker/st_program.c
index 03a685c..2a4edfa 100644
--- a/src/mesa/state_tracker/st_program.c
+++ b/src/mesa/state_tracker/st_program.c
@@ -586,7 +586,7 @@  bool
 st_translate_fragment_program(struct st_context *st,
                               struct st_fragment_program *stfp)
 {
-   GLuint outputMapping[FRAG_RESULT_MAX];
+   GLuint outputMapping[2 * FRAG_RESULT_MAX];
    GLuint inputMapping[VARYING_SLOT_MAX];
    GLuint inputSlotToAttr[VARYING_SLOT_MAX];
    GLuint interpMode[PIPE_MAX_SHADER_INPUTS];  /* XXX size? */
@@ -810,9 +810,13 @@  st_translate_fragment_program(struct st_context *st,
       }
 
       /* handle remaining outputs (color) */
-      for (attr = 0; attr < FRAG_RESULT_MAX; attr++) {
-         if (outputsWritten & BITFIELD64_BIT(attr)) {
-            switch (attr) {
+      for (attr = 0; attr < ARRAY_SIZE(outputMapping); attr++) {
+         const GLbitfield64 written = attr < FRAG_RESULT_MAX ? outputsWritten :
+            stfp->Base.Base.SecondaryOutputsWritten;
+         const unsigned loc = attr % FRAG_RESULT_MAX;
+
+         if (written & BITFIELD64_BIT(loc)) {
+            switch (loc) {
             case FRAG_RESULT_DEPTH:
             case FRAG_RESULT_STENCIL:
             case FRAG_RESULT_SAMPLE_MASK:
@@ -822,8 +826,8 @@  st_translate_fragment_program(struct st_context *st,
             case FRAG_RESULT_COLOR:
                write_all = GL_TRUE; /* fallthrough */
             default:
-               assert(attr == FRAG_RESULT_COLOR ||
-                      (FRAG_RESULT_DATA0 <= attr && attr < FRAG_RESULT_MAX));
+               assert(loc == FRAG_RESULT_COLOR ||
+                      (FRAG_RESULT_DATA0 <= loc && loc < FRAG_RESULT_MAX));
                fs_output_semantic_name[fs_num_outputs] = TGSI_SEMANTIC_COLOR;
                fs_output_semantic_index[fs_num_outputs] = numColors;
                outputMapping[attr] = fs_num_outputs;
-- 
2.9.0


Comments

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@riseup.net> wrote:
> Francisco Jerez <currojerez@riseup.net> writes:
>
>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>
>>> On Tue, Aug 23, 2016 at 12:18 AM, Francisco Jerez <currojerez@riseup.net> wrote:
>>>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>>>
>>>>> On Tue, Aug 23, 2016 at 12:05 AM, Francisco Jerez <currojerez@riseup.net> wrote:
>>>>>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>>>>>
>>>>>>> On Mon, Aug 22, 2016 at 10:55 PM, Francisco Jerez <currojerez@riseup.net> wrote:
>>>>>>>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>>>>>>>
>>>>>>>>> On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez <currojerez@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
>>
>
Ilia Mirkin <imirkin@alum.mit.edu> writes:

> 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.
>
It should apply cleanly on latest master now.

>[...]
On Wed, Aug 24, 2016 at 4:30 PM, Francisco Jerez <currojerez@riseup.net> wrote:
> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>
>> 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.
>>
> It should apply cleanly on latest master now.

Indeed it does. Running these 3 patches:

glsl: Calculate bitset of secondary outputs written in ir_set_program_inouts.
st/glsl_to_tgsi: Translate fragment shader secondary outputs to TGSI outputs.
glsl: Fix incorrect hard-coded location of the
gl_SecondaryFragColorEXT built-in.

I get:

state_tracker/st_program.c:824:st_translate_fragment_program:
Assertion `0' failed.
(gdb) l
819                 switch (loc) {
820                 case FRAG_RESULT_DEPTH:
821                 case FRAG_RESULT_STENCIL:
822                 case FRAG_RESULT_SAMPLE_MASK:
823                    /* handled above */
824                    assert(0);
825                    break;
826                 case FRAG_RESULT_COLOR:
827                    write_all = GL_TRUE; /* fallthrough */
828                 default:
(gdb) i locals
written = 8
loc = 3
numColors = 1
outputsWritten = 4
outputMapping = {7080032, 0, 1, 0, 14, 0, 7082880, 14, 4294951632, 32767,
  4156877824, 3574963175, 4294951316, 32767, 7015088, 0, 4294951504, 32767,
  4030068507, 32767, 3, 14, 0, 15}
inputMapping = {0, 4294967295 <repeats 61 times>}
inputSlotToAttr = {0, 4294967295 <repeats 61 times>}
interpMode = {1, 0 <repeats 79 times>}
interpLocation = {0 <repeats 24 times>, 4120177808, 32767, 0, 0, 0, 0,
  7080656, 0, 27, 0, 0, 0, 1744, 0, 1744, 0, 4120177728, 32767, 37, 32767,
  576, 0, 4030368463, 32767, 7074928, 0, 7078416, 0, 4294952288, 32767, 2,
  32767, 16, 0, 7078416, 0, 0, 2090733823, 7062160, 0, 0, 0, 7078416, 3, 3,
  2090733823, 6845752, 0, 4294952320, 2, 4294952320, 32767, 4029319846, 32767,
  4040141664, 32767}
attr = 15
inputsRead = 1
ureg = 0x4e00000000
write_all = 1 '\001'

I don't really have the time to investigate this further ATM. However
you should be able to reproduce yourself with llvmpipe.

  -ilia
Ilia Mirkin <imirkin@alum.mit.edu> writes:

> On Wed, Aug 24, 2016 at 4:30 PM, Francisco Jerez <currojerez@riseup.net> wrote:
>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>
>>> 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.
>>>
>> It should apply cleanly on latest master now.
>
> Indeed it does. Running these 3 patches:
>
> glsl: Calculate bitset of secondary outputs written in ir_set_program_inouts.
> st/glsl_to_tgsi: Translate fragment shader secondary outputs to TGSI outputs.
> glsl: Fix incorrect hard-coded location of the
> gl_SecondaryFragColorEXT built-in.
>

So you didn't have PATCH 1 applied to your tree?  I'd expect this change
to behave badly without it and possibly end up creating too many
outputs.  I guess I'll have to squash this change into the others...

> I get:
>
> state_tracker/st_program.c:824:st_translate_fragment_program:
> Assertion `0' failed.
> (gdb) l
> 819                 switch (loc) {
> 820                 case FRAG_RESULT_DEPTH:
> 821                 case FRAG_RESULT_STENCIL:
> 822                 case FRAG_RESULT_SAMPLE_MASK:
> 823                    /* handled above */
> 824                    assert(0);
> 825                    break;
> 826                 case FRAG_RESULT_COLOR:
> 827                    write_all = GL_TRUE; /* fallthrough */
> 828                 default:
> (gdb) i locals
> written = 8
> loc = 3
> numColors = 1
> outputsWritten = 4
> outputMapping = {7080032, 0, 1, 0, 14, 0, 7082880, 14, 4294951632, 32767,
>   4156877824, 3574963175, 4294951316, 32767, 7015088, 0, 4294951504, 32767,
>   4030068507, 32767, 3, 14, 0, 15}
> inputMapping = {0, 4294967295 <repeats 61 times>}
> inputSlotToAttr = {0, 4294967295 <repeats 61 times>}
> interpMode = {1, 0 <repeats 79 times>}
> interpLocation = {0 <repeats 24 times>, 4120177808, 32767, 0, 0, 0, 0,
>   7080656, 0, 27, 0, 0, 0, 1744, 0, 1744, 0, 4120177728, 32767, 37, 32767,
>   576, 0, 4030368463, 32767, 7074928, 0, 7078416, 0, 4294952288, 32767, 2,
>   32767, 16, 0, 7078416, 0, 0, 2090733823, 7062160, 0, 0, 0, 7078416, 3, 3,
>   2090733823, 6845752, 0, 4294952320, 2, 4294952320, 32767, 4029319846, 32767,
>   4040141664, 32767}
> attr = 15
> inputsRead = 1
> ureg = 0x4e00000000
> write_all = 1 '\001'
>
> I don't really have the time to investigate this further ATM. However
> you should be able to reproduce yourself with llvmpipe.
>
Heh, I have a hunch what's going on, I'll take a look and resend as a
proper patch -- if the general approach seems reasonable to you?

>   -ilia
On Thu, Aug 25, 2016 at 12:45 AM, Francisco Jerez <currojerez@riseup.net> wrote:
> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>
>> On Wed, Aug 24, 2016 at 4:30 PM, Francisco Jerez <currojerez@riseup.net> wrote:
>>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>>
>>>> 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.
>>>>
>>> It should apply cleanly on latest master now.
>>
>> Indeed it does. Running these 3 patches:
>>
>> glsl: Calculate bitset of secondary outputs written in ir_set_program_inouts.
>> st/glsl_to_tgsi: Translate fragment shader secondary outputs to TGSI outputs.
>> glsl: Fix incorrect hard-coded location of the
>> gl_SecondaryFragColorEXT built-in.
>>
>
> So you didn't have PATCH 1 applied to your tree?  I'd expect this change
> to behave badly without it and possibly end up creating too many
> outputs.  I guess I'll have to squash this change into the others...

Isn't patch 1 the last patch in that list? (I applied them in the
wrong order, but I don't think it matters)
Ilia Mirkin <imirkin@alum.mit.edu> writes:

> On Thu, Aug 25, 2016 at 12:45 AM, Francisco Jerez <currojerez@riseup.net> wrote:
>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>
>>> On Wed, Aug 24, 2016 at 4:30 PM, Francisco Jerez <currojerez@riseup.net> wrote:
>>>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>>>
>>>>> 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.
>>>>>
>>>> It should apply cleanly on latest master now.
>>>
>>> Indeed it does. Running these 3 patches:
>>>
>>> glsl: Calculate bitset of secondary outputs written in ir_set_program_inouts.
>>> st/glsl_to_tgsi: Translate fragment shader secondary outputs to TGSI outputs.
>>> glsl: Fix incorrect hard-coded location of the
>>> gl_SecondaryFragColorEXT built-in.
>>>
>>
>> So you didn't have PATCH 1 applied to your tree?  I'd expect this change
>> to behave badly without it and possibly end up creating too many
>> outputs.  I guess I'll have to squash this change into the others...
>
> Isn't patch 1 the last patch in that list? (I applied them in the
> wrong order, but I don't think it matters)

Nope, I was referring to "[PATCH 1/3] glsl: Fix
gl_program::OutputsWritten computation for dual-source blending.".
On Thu, Aug 25, 2016 at 12:57 AM, Francisco Jerez <currojerez@riseup.net> wrote:
> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>
>> On Thu, Aug 25, 2016 at 12:45 AM, Francisco Jerez <currojerez@riseup.net> wrote:
>>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>>
>>>> On Wed, Aug 24, 2016 at 4:30 PM, Francisco Jerez <currojerez@riseup.net> wrote:
>>>>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>>>>
>>>>>> 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.
>>>>>>
>>>>> It should apply cleanly on latest master now.
>>>>
>>>> Indeed it does. Running these 3 patches:
>>>>
>>>> glsl: Calculate bitset of secondary outputs written in ir_set_program_inouts.
>>>> st/glsl_to_tgsi: Translate fragment shader secondary outputs to TGSI outputs.
>>>> glsl: Fix incorrect hard-coded location of the
>>>> gl_SecondaryFragColorEXT built-in.
>>>>
>>>
>>> So you didn't have PATCH 1 applied to your tree?  I'd expect this change
>>> to behave badly without it and possibly end up creating too many
>>> outputs.  I guess I'll have to squash this change into the others...
>>
>> Isn't patch 1 the last patch in that list? (I applied them in the
>> wrong order, but I don't think it matters)
>
> Nope, I was referring to "[PATCH 1/3] glsl: Fix
> gl_program::OutputsWritten computation for dual-source blending.".

Ah yeah, I couldn't apply that one, I think due to the order of things
I did in. With all of those applied, the piglit in question passes
now, along with the other arb_blend_func_extended piglits. I haven't
actually looked at the contents of the patch.

  -ilia