Re: [Mesa-dev] [PATCH] glsl/linker: outputs in the same location must share interpolation
On Thu, 2017-10-19 at 08:29 +0200, Iago Toral wrote:
> On Thu, 2017-10-19 at 17:14 +1100, Timothy Arceri wrote:
> >
> > On 19/10/17 16:57, Iago Toral Quiroga wrote:
> > > From ARB_enhanced_layouts:
> > >
> > > "[...]when location aliasing, the aliases sharing the location
> > > must have the same underlying numerical type (floating-point or
> > > integer) and the same auxiliary storage and
> > > interpolation qualification.[...]"
> > >
> > > Add code to the linker to validate that aliased locations do
> > > have the same interpolation.
> > >
> > > Fixes:
> > > KHR-
> > > GL45.enhanced_layouts.varying_location_aliasing_with_mixed_interp
> > > ol
> > > ation
> > > ---
> > > src/compiler/glsl/link_varyings.cpp | 35
> > > +++
> > > 1 file changed, 35 insertions(+)
> > >
> > > diff --git a/src/compiler/glsl/link_varyings.cpp
> > > b/src/compiler/glsl/link_varyings.cpp
> > > index 69c92bf53b..c888635e82 100644
> > > --- a/src/compiler/glsl/link_varyings.cpp
> > > +++ b/src/compiler/glsl/link_varyings.cpp
> > > @@ -459,6 +459,41 @@ cross_validate_outputs_to_inputs(struct
> > > gl_context *ctx,
> > >
> > > while (idx < slot_limit) {
> > > unsigned i = var->data.location_frac;
> > > +
> > > +/* If there are other outputs assigned to the same
> > > location
> > > + * they must have the same interpolation
> > > + */
> > > +unsigned comp = 0;
> > > +while (comp < i) {
> > > + ir_variable *tmp = explicit_locations[idx][comp];
> > > + if (tmp && tmp->data.interpolation != var-
> > > > data.interpolation) {
> > >
> > > + linker_error(prog,
> > > + "%s shader has multiple outputs
> > > at
> > > explicit "
> > > + "location %u with different
> > > interpolation "
> > > + "settings\n",
> > > + _mesa_shader_stage_to_string(prod
> > > uc
> > > er->Stage),
> > > + idx);
> > > + return;
> > > + }
> > > + comp++;
> > > +}
> > > +
> > > +comp = last_comp + 1;
>
> I just noticed that this should be:
>
> comp = last_comp;
>
> since that is the first component not used by this variable.
>
> > > +while (comp < 4) {
> > > + ir_variable *tmp = explicit_locations[idx][comp];
> > > + if (tmp && tmp->data.interpolation != var-
> > > > data.interpolation) {
> > >
> > > + linker_error(prog,
> > > + "%s shader has multiple outputs
> > > at
> > > explicit "
> > > + "location %u with different
> > > interpolation "
> > > + "settings\n",
> > > + _mesa_shader_stage_to_string(prod
> > > uc
> > > er->Stage),
> > > + idx);
> > > + return;
> > > + }
> > > + comp++;
> > > +}
> >
> > Can't we just put this check in the loop below? It should allow
> > doubles
> > to be handled without the duplication if my quick skim over the
> > code
> > is
> > correct.
>
> Mmm...I don't think so. The loop below checks the components used by
> the current variable to see if there is aliasing in the range used by
> the variable: [i, last_comp). We need to check if there are other
> variables assigned to any other components in the same location to
> check if there is a different interpolation for them, so that means
> that we need to check all the components in the location that are
> _not_
> used by this variable, so basically, all the components not covered
> by
> the loop below. This means the regions [0...i) and [last_comp...4].
>
> We could make the loop below go up to 4 instead of last_comp and the
> nhave ifs inside to do one thing in the range [i...last_comp) and
> another one in the range [last_comp, 4] but that would only make
> things
> more confusing IMHO, so I'd rather not do that.
However, we can do a single loop and skip the components in [1,
last_comp) for these checks so we only have one loop instead of two.
I'll send a v2 with this change. I have another patch to add also
checks for the auxiliary storage and this way we don't have to
duplicate those too.
Iago
> >
> > > +
> > > +/* Component aliasing is not allowed */
> > > while (i < last_comp) {
> > > if (explicit_locations[idx][i] != NULL) {
> > > linker_error(prog,
> > >
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl/linker: outputs in the same location must share interpolation
On Thu, 2017-10-19 at 17:14 +1100, Timothy Arceri wrote:
>
> On 19/10/17 16:57, Iago Toral Quiroga wrote:
> > From ARB_enhanced_layouts:
> >
> > "[...]when location aliasing, the aliases sharing the location
> > must have the same underlying numerical type (floating-point or
> > integer) and the same auxiliary storage and
> > interpolation qualification.[...]"
> >
> > Add code to the linker to validate that aliased locations do
> > have the same interpolation.
> >
> > Fixes:
> > KHR-
> > GL45.enhanced_layouts.varying_location_aliasing_with_mixed_interpol
> > ation
> > ---
> > src/compiler/glsl/link_varyings.cpp | 35
> > +++
> > 1 file changed, 35 insertions(+)
> >
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index 69c92bf53b..c888635e82 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -459,6 +459,41 @@ cross_validate_outputs_to_inputs(struct
> > gl_context *ctx,
> >
> > while (idx < slot_limit) {
> > unsigned i = var->data.location_frac;
> > +
> > +/* If there are other outputs assigned to the same
> > location
> > + * they must have the same interpolation
> > + */
> > +unsigned comp = 0;
> > +while (comp < i) {
> > + ir_variable *tmp = explicit_locations[idx][comp];
> > + if (tmp && tmp->data.interpolation != var-
> > >data.interpolation) {
> > + linker_error(prog,
> > + "%s shader has multiple outputs at
> > explicit "
> > + "location %u with different
> > interpolation "
> > + "settings\n",
> > + _mesa_shader_stage_to_string(produc
> > er->Stage),
> > + idx);
> > + return;
> > + }
> > + comp++;
> > +}
> > +
> > +comp = last_comp + 1;
I just noticed that this should be:
comp = last_comp;
since that is the first component not used by this variable.
> > +while (comp < 4) {
> > + ir_variable *tmp = explicit_locations[idx][comp];
> > + if (tmp && tmp->data.interpolation != var-
> > >data.interpolation) {
> > + linker_error(prog,
> > + "%s shader has multiple outputs at
> > explicit "
> > + "location %u with different
> > interpolation "
> > + "settings\n",
> > + _mesa_shader_stage_to_string(produc
> > er->Stage),
> > + idx);
> > + return;
> > + }
> > + comp++;
> > +}
>
> Can't we just put this check in the loop below? It should allow
> doubles
> to be handled without the duplication if my quick skim over the code
> is
> correct.
Mmm...I don't think so. The loop below checks the components used by
the current variable to see if there is aliasing in the range used by
the variable: [i, last_comp). We need to check if there are other
variables assigned to any other components in the same location to
check if there is a different interpolation for them, so that means
that we need to check all the components in the location that are _not_
used by this variable, so basically, all the components not covered by
the loop below. This means the regions [0...i) and [last_comp...4].
We could make the loop below go up to 4 instead of last_comp and the
nhave ifs inside to do one thing in the range [i...last_comp) and
another one in the range [last_comp, 4] but that would only make things
more confusing IMHO, so I'd rather not do that.
Iago
>
> > +
> > +/* Component aliasing is not allowed */
> > while (i < last_comp) {
> > if (explicit_locations[idx][i] != NULL) {
> > linker_error(prog,
> >
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl/linker: outputs in the same location must share interpolation
On 19/10/17 16:57, Iago Toral Quiroga wrote:
From ARB_enhanced_layouts:
"[...]when location aliasing, the aliases sharing the location
must have the same underlying numerical type (floating-point or
integer) and the same auxiliary storage and
interpolation qualification.[...]"
Add code to the linker to validate that aliased locations do
have the same interpolation.
Fixes:
KHR-GL45.enhanced_layouts.varying_location_aliasing_with_mixed_interpolation
---
src/compiler/glsl/link_varyings.cpp | 35 +++
1 file changed, 35 insertions(+)
diff --git a/src/compiler/glsl/link_varyings.cpp
b/src/compiler/glsl/link_varyings.cpp
index 69c92bf53b..c888635e82 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -459,6 +459,41 @@ cross_validate_outputs_to_inputs(struct gl_context *ctx,
while (idx < slot_limit) {
unsigned i = var->data.location_frac;
+
+/* If there are other outputs assigned to the same location
+ * they must have the same interpolation
+ */
+unsigned comp = 0;
+while (comp < i) {
+ ir_variable *tmp = explicit_locations[idx][comp];
+ if (tmp && tmp->data.interpolation != var->data.interpolation) {
+ linker_error(prog,
+ "%s shader has multiple outputs at explicit "
+ "location %u with different interpolation "
+ "settings\n",
+ _mesa_shader_stage_to_string(producer->Stage),
+ idx);
+ return;
+ }
+ comp++;
+}
+
+comp = last_comp + 1;
+while (comp < 4) {
+ ir_variable *tmp = explicit_locations[idx][comp];
+ if (tmp && tmp->data.interpolation != var->data.interpolation) {
+ linker_error(prog,
+ "%s shader has multiple outputs at explicit "
+ "location %u with different interpolation "
+ "settings\n",
+ _mesa_shader_stage_to_string(producer->Stage),
+ idx);
+ return;
+ }
+ comp++;
+}
Can't we just put this check in the loop below? It should allow doubles
to be handled without the duplication if my quick skim over the code is
correct.
+
+/* Component aliasing is not allowed */
while (i < last_comp) {
if (explicit_locations[idx][i] != NULL) {
linker_error(prog,
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] glsl/linker: outputs in the same location must share interpolation
From ARB_enhanced_layouts:
"[...]when location aliasing, the aliases sharing the location
must have the same underlying numerical type (floating-point or
integer) and the same auxiliary storage and
interpolation qualification.[...]"
Add code to the linker to validate that aliased locations do
have the same interpolation.
Fixes:
KHR-GL45.enhanced_layouts.varying_location_aliasing_with_mixed_interpolation
---
src/compiler/glsl/link_varyings.cpp | 35 +++
1 file changed, 35 insertions(+)
diff --git a/src/compiler/glsl/link_varyings.cpp
b/src/compiler/glsl/link_varyings.cpp
index 69c92bf53b..c888635e82 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -459,6 +459,41 @@ cross_validate_outputs_to_inputs(struct gl_context *ctx,
while (idx < slot_limit) {
unsigned i = var->data.location_frac;
+
+/* If there are other outputs assigned to the same location
+ * they must have the same interpolation
+ */
+unsigned comp = 0;
+while (comp < i) {
+ ir_variable *tmp = explicit_locations[idx][comp];
+ if (tmp && tmp->data.interpolation != var->data.interpolation) {
+ linker_error(prog,
+ "%s shader has multiple outputs at explicit "
+ "location %u with different interpolation "
+ "settings\n",
+ _mesa_shader_stage_to_string(producer->Stage),
+ idx);
+ return;
+ }
+ comp++;
+}
+
+comp = last_comp + 1;
+while (comp < 4) {
+ ir_variable *tmp = explicit_locations[idx][comp];
+ if (tmp && tmp->data.interpolation != var->data.interpolation) {
+ linker_error(prog,
+ "%s shader has multiple outputs at explicit "
+ "location %u with different interpolation "
+ "settings\n",
+ _mesa_shader_stage_to_string(producer->Stage),
+ idx);
+ return;
+ }
+ comp++;
+}
+
+/* Component aliasing is not allowed */
while (i < last_comp) {
if (explicit_locations[idx][i] != NULL) {
linker_error(prog,
--
2.11.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
