On 01/25/2016 01:14 PM, Timothy Arceri wrote:
On Mon, 2016-01-25 at 12:47 +0200, Tapani Pälli wrote:
On 01/25/2016 12:29 PM, Timothy Arceri wrote:
On Mon, 2016-01-25 at 16:41 +1100, Timothy Arceri wrote:
On Mon, 2015-11-30 at 14:31 +0200, Tapani Pälli wrote:
Reviewed-by: Tapani Pälli <tapani.pa...@intel.com>
On 11/25/2015 11:54 AM, Timothy Arceri wrote:
From: Gregory Hainaut <gregory.hain...@gmail.com>
This fixes an issue where the addition of the FLAT qualifier
in
varying_matches::record() can break the expected varying
order.
It also avoids a future issue with the relaxing of
interpolation
qualifier matching constraints in GLSL 4.50.
V2: (by Timothy Arceri)
* reworked comment slightly
Signed-off-by: Gregory Hainaut <gregory.hain...@gmail.com>
Reviewed-by: Timothy Arceri <timothy.arc...@collabora.com>
---
src/glsl/link_varyings.cpp | 38
++++++++++++++++++++++++++++++++-
-----
1 file changed, 32 insertions(+), 6 deletions(-)
diff --git a/src/glsl/link_varyings.cpp
b/src/glsl/link_varyings.cpp
index ac2755f..71750d1 100644
--- a/src/glsl/link_varyings.cpp
+++ b/src/glsl/link_varyings.cpp
@@ -766,7 +766,7 @@ public:
gl_shader_stage consumer_stage);
~varying_matches();
void record(ir_variable *producer_var, ir_variable
*consumer_var);
- unsigned assign_locations(uint64_t reserved_slots);
+ unsigned assign_locations(uint64_t reserved_slots, bool
separate_shader);
void store_locations() const;
private:
@@ -988,11 +988,36 @@ varying_matches::record(ir_variable
*producer_var, ir_variable *consumer_var)
* passed to varying_matches::record().
*/
unsigned
-varying_matches::assign_locations(uint64_t reserved_slots)
+varying_matches::assign_locations(uint64_t reserved_slots,
bool
separate_shader)
{
- /* Sort varying matches into an order that makes them
easy to
pack. */
- qsort(this->matches, this->num_matches, sizeof(*this
->matches),
- &varying_matches::match_comparator);
+ /* We disable varying sorting for separate shader
programs
for
the
+ * following reasons:
+ *
+ * 1/ All programs must sort the code in the same order
to
guarantee the
+ * interface matching. However
varying_matches::record()
will change the
+ * interpolation qualifier of some stages.
+ *
+ * 2/ GLSL version 4.50 removes the matching constrain on
the
interpolation
+ * qualifier.
+ *
+ * From Section 4.5 (Interpolation Qualifiers) of the
GLSL
4.40
spec:
+ *
+ * "The type and presence of interpolation qualifiers
of
variables with
+ * the same name declared in all linked shaders for
the
same
cross-stage
+ * interface must match, otherwise the link command
will
fail.
+ *
+ * When comparing an output from one stage to an input
of
a
subsequent
+ * stage, the input and output don't match if their
interpolation
+ * qualifiers (or lack thereof) are not the same."
+ *
+ * "It is a link-time error if, within the same stage,
the
interpolation
+ * qualifiers of variables of the same name do not
match."
+ */
+ if (!separate_shader) {
+ /* Sort varying matches into an order that makes them
easy
to pack. */
+ qsort(this->matches, this->num_matches, sizeof(*this
->matches),
+ &varying_matches::match_comparator);
+ }
unsigned generic_location = 0;
unsigned generic_patch_location = MAX_VARYING*4;
@@ -1592,7 +1617,8 @@ assign_varying_locations(struct
gl_context
*ctx,
reserved_varying_slot(producer, ir_var_shader_out) |
reserved_varying_slot(consumer, ir_var_shader_in);
- const unsigned slots_used =
matches.assign_locations(reserved_slots);
+ const unsigned slots_used =
matches.assign_locations(reserved_slots,
+ prog
->SeparateShader);
matches.store_locations();
for (unsigned i = 0; i < num_tfeedback_decls; ++i) {
I haven't figured out why yet but this patch breaks transform
feedback
when the last stage is a SSO (which there is currently no piglit
tests
for).
OK so the problem seems to be that if we don't do a sort we don't
move
vec4's to the top of the list, this means they don't get assigned
the
first locations which means they can end up getting split.
e.g
If float a is assigned location 0 then vec4 b will be assigned
location
0 with component 3 in location 1.
This is obviously bad to begin with and it also causes the backend
to
fall over for transform feedback as its not expecting a vec4 to be
split over multiple locations.
Did you try to disable varying packing?
No because the transform feedback code depends on packing being enabled
so that would break things worse and I don't really feel like rewriting
the transform feedback code :P
Ah right
I believe it is broken for SSO
currently. IMO packing cannot be done safely before both producer and
consumer interface are known, and producer is really known only when
we
validate the pipeline for first time. We discussed this with Samuel
some
time ago related to cases where consumer interface may override
producer
qualifiers, in those cases packing does not work correctly.
Can you give an example of what you mean?
IIRC the issue is that fragment shader (and maybe some other stages
too?) can override interpolation and auxiliary qualifiers of what's
given in vertex shader. With regular shader we can just override
producer's qualifiers before packing so that their 'packing class' will
match but with SSO we will have different 'packing class' for these when
packing (as such override cannot be done because consumer is not known
yet) so interfaces will not look the same and some variables may get
scattered to different locations.
// Tapani
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
