Re: [Mesa-dev] [PATCH v2 6/6] glsl/linker: check for xfb_offset aliasing
On Wed, Apr 10, 2019 at 7:31 AM Andres Gomez wrote: > On Wed, 2019-04-10 at 10:40 +1000, Timothy Arceri wrote: > > On 13/2/19 8:57 am, Andres Gomez wrote: > > > @@ -1764,6 +1765,8 @@ struct gl_transform_feedback_buffer > > > > > > uint32_t NumVaryings; > > > > > > + BITSET_WORD *UsedComponents; > > > > Please don't add new members here that are used only for compile time > > validation. Can be try to come up with a different solution to this please? > > v1 was trying to do this through a nested loop to avoid adding extra > data structures. IMO, the nested loop had a near O(N^2) behavior but > given the possible real max amount of loops, it shouldn't have been too > bad. > > However, Ilia didn't like it and suggested an array-based solution. I'm > using here a bitmask and the max amount of components would make this > to not use a lot of memory. > > I think both solutions are fine but, if I have to come up with > something else, I'm running out of ideas ... > > Suggestions are welcome! I believe the comment is that this shouldn't live in "struct gl_transform_feedback_buffer" but rather in tfeedback_decl somewhere. That should be easy to do, no? (I haven't gone back and looked at the code, so perhaps not?) -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 6/6] glsl/linker: check for xfb_offset aliasing
On Wed, 2019-04-10 at 10:40 +1000, Timothy Arceri wrote:
> On 13/2/19 8:57 am, Andres Gomez wrote:
> > From page 76 (page 80 of the PDF) of the GLSL 4.60 v.5 spec:
> >
> >" No aliasing in output buffers is allowed: It is a compile-time or
> > link-time error to specify variables with overlapping transform
> > feedback offsets."
> >
> > Currently, this is expected to fail, but it succeeds:
> >
> >"
> >
> > ...
> >
> > layout (xfb_offset = 0) out vec2 a;
> > layout (xfb_offset = 0) out vec4 b;
> >
> > ...
> >
> >"
> >
> > v2: use a data structure to track the used components instead of a
> > nested loop (Ilia).
> >
> > Cc: Timothy Arceri
> > Cc: Ilia Mirkin
> > Signed-off-by: Andres Gomez
> > ---
> > src/compiler/glsl/link_varyings.cpp | 89 ++---
> > src/mesa/main/mtypes.h | 3 +
> > 2 files changed, 70 insertions(+), 22 deletions(-)
> >
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index 8c7d6c14c8f..95e9ae895d2 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -1173,6 +1173,73 @@ tfeedback_decl::store(struct gl_context *ctx, struct
> > gl_shader_program *prog,
> > unsigned location = this->location;
> > unsigned location_frac = this->location_frac;
> > unsigned num_components = this->num_components();
> > +
> > + /* From GL_EXT_transform_feedback:
> > + * A program will fail to link if:
> > + *
> > + * * the total number of components to capture is greater than
> > the
> > + * constant MAX_TRANSFORM_FEEDBACK_INTERLEAVED_COMPONENTS_EXT
> > and
> > + * the buffer mode is INTERLEAVED_ATTRIBS_EXT.
> > + *
> > + * From GL_ARB_enhanced_layouts:
> > + *
> > + * "The resulting stride (implicit or explicit) must be less than
> > or
> > + *equal to the implementation-dependent constant
> > + *gl_MaxTransformFeedbackInterleavedComponents."
> > + */
> > + if ((prog->TransformFeedback.BufferMode == GL_INTERLEAVED_ATTRIBS ||
> > + has_xfb_qualifiers) &&
> > + xfb_offset + num_components >
> > + ctx->Const.MaxTransformFeedbackInterleavedComponents) {
> > + linker_error(prog,
> > + "The MAX_TRANSFORM_FEEDBACK_INTERLEAVED_COMPONENTS "
> > + "limit has been exceeded.");
> > + return false;
> > + }
> > +
> > + {
>
> We don't need to do this in Mesa anymore. All compilers are now expected
> to support C99 i.e. variable declaration is no longer restricted to the
> start of a compound statement.
I can remove the brackets but I don't need those variables to outlive
beyond this piece of code for the validation either.
>
> > + /* From the OpenGL 4.60.5 spec, section 4.4.2. Output Layout
> > + * Qualifiers, Page 76, (Transform Feedback Layout Qualifiers):
> > + *
> > + * "No aliasing in output buffers is allowed: It is a
> > compile-time or
> > + * link-time error to specify variables with overlapping
> > transform
> > + * feedback offsets."
> > + */
> > + const unsigned max_components =
> > +ctx->Const.MaxTransformFeedbackInterleavedComponents;
> > + const unsigned first_component = xfb_offset;
> > + const unsigned last_component = xfb_offset + num_components - 1;
> > + const unsigned start_word = BITSET_BITWORD(first_component);
> > + const unsigned end_word = BITSET_BITWORD(last_component);
> > + assert(last_component < max_components);
> > +
> > + if (!info->Buffers[buffer].UsedComponents) {
> > +info->Buffers[buffer].UsedComponents =
> > + rzalloc_array(info, BITSET_WORD,
> > BITSET_WORDS(max_components));
> > + }
> > + BITSET_WORD *used = info->Buffers[buffer].UsedComponents;
> > +
> > + for (unsigned word = start_word; word <= end_word; word++) {
> > +unsigned start_range = 0;
> > +unsigned end_range = BITSET_WORDBITS - 1;
> > +
> > +if (word == start_word)
> > + start_range = first_component % BITSET_WORDBITS;
> > +
> > +if (word == end_word)
> > + end_range = last_component % BITSET_WORDBITS;
> > +
> > +if (used[word] & BITSET_RANGE(start_range, end_range)) {
> > + linker_error(prog,
> > +"variable '%s', xfb_offset (%d) "
> > +"is causing aliasing.",
> > +this->orig_name, xfb_offset * 4);
> > + return false;
> > +}
> > +used[word] |= BITSET_RANGE(start_range, end_range);
> > + }
> > + }
> > +
> > while (num_components > 0) {
> >unsigned
Re: [Mesa-dev] [PATCH v2 6/6] glsl/linker: check for xfb_offset aliasing
On 13/2/19 8:57 am, Andres Gomez wrote:
From page 76 (page 80 of the PDF) of the GLSL 4.60 v.5 spec:
" No aliasing in output buffers is allowed: It is a compile-time or
link-time error to specify variables with overlapping transform
feedback offsets."
Currently, this is expected to fail, but it succeeds:
"
...
layout (xfb_offset = 0) out vec2 a;
layout (xfb_offset = 0) out vec4 b;
...
"
v2: use a data structure to track the used components instead of a
nested loop (Ilia).
Cc: Timothy Arceri
Cc: Ilia Mirkin
Signed-off-by: Andres Gomez
---
src/compiler/glsl/link_varyings.cpp | 89 ++---
src/mesa/main/mtypes.h | 3 +
2 files changed, 70 insertions(+), 22 deletions(-)
diff --git a/src/compiler/glsl/link_varyings.cpp
b/src/compiler/glsl/link_varyings.cpp
index 8c7d6c14c8f..95e9ae895d2 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -1173,6 +1173,73 @@ tfeedback_decl::store(struct gl_context *ctx, struct
gl_shader_program *prog,
unsigned location = this->location;
unsigned location_frac = this->location_frac;
unsigned num_components = this->num_components();
+
+ /* From GL_EXT_transform_feedback:
+ * A program will fail to link if:
+ *
+ * * the total number of components to capture is greater than the
+ * constant MAX_TRANSFORM_FEEDBACK_INTERLEAVED_COMPONENTS_EXT and
+ * the buffer mode is INTERLEAVED_ATTRIBS_EXT.
+ *
+ * From GL_ARB_enhanced_layouts:
+ *
+ * "The resulting stride (implicit or explicit) must be less than or
+ *equal to the implementation-dependent constant
+ *gl_MaxTransformFeedbackInterleavedComponents."
+ */
+ if ((prog->TransformFeedback.BufferMode == GL_INTERLEAVED_ATTRIBS ||
+ has_xfb_qualifiers) &&
+ xfb_offset + num_components >
+ ctx->Const.MaxTransformFeedbackInterleavedComponents) {
+ linker_error(prog,
+ "The MAX_TRANSFORM_FEEDBACK_INTERLEAVED_COMPONENTS "
+ "limit has been exceeded.");
+ return false;
+ }
+
+ {
We don't need to do this in Mesa anymore. All compilers are now expected
to support C99 i.e. variable declaration is no longer restricted to the
start of a compound statement.
+ /* From the OpenGL 4.60.5 spec, section 4.4.2. Output Layout
+ * Qualifiers, Page 76, (Transform Feedback Layout Qualifiers):
+ *
+ * "No aliasing in output buffers is allowed: It is a compile-time or
+ * link-time error to specify variables with overlapping transform
+ * feedback offsets."
+ */
+ const unsigned max_components =
+ctx->Const.MaxTransformFeedbackInterleavedComponents;
+ const unsigned first_component = xfb_offset;
+ const unsigned last_component = xfb_offset + num_components - 1;
+ const unsigned start_word = BITSET_BITWORD(first_component);
+ const unsigned end_word = BITSET_BITWORD(last_component);
+ assert(last_component < max_components);
+
+ if (!info->Buffers[buffer].UsedComponents) {
+info->Buffers[buffer].UsedComponents =
+ rzalloc_array(info, BITSET_WORD, BITSET_WORDS(max_components));
+ }
+ BITSET_WORD *used = info->Buffers[buffer].UsedComponents;
+
+ for (unsigned word = start_word; word <= end_word; word++) {
+unsigned start_range = 0;
+unsigned end_range = BITSET_WORDBITS - 1;
+
+if (word == start_word)
+ start_range = first_component % BITSET_WORDBITS;
+
+if (word == end_word)
+ end_range = last_component % BITSET_WORDBITS;
+
+if (used[word] & BITSET_RANGE(start_range, end_range)) {
+ linker_error(prog,
+"variable '%s', xfb_offset (%d) "
+"is causing aliasing.",
+this->orig_name, xfb_offset * 4);
+ return false;
+}
+used[word] |= BITSET_RANGE(start_range, end_range);
+ }
+ }
+
while (num_components > 0) {
unsigned output_size = MIN2(num_components, 4 - location_frac);
assert((info->NumOutputs == 0 && max_outputs == 0) ||
@@ -1223,28 +1290,6 @@ tfeedback_decl::store(struct gl_context *ctx, struct
gl_shader_program *prog,
info->Buffers[buffer].Stride = xfb_offset;
}
- /* From GL_EXT_transform_feedback:
-* A program will fail to link if:
-*
-* * the total number of components to capture is greater than
-* the constant MAX_TRANSFORM_FEEDBACK_INTERLEAVED_COMPONENTS_EXT
-* and the buffer mode is INTERLEAVED_ATTRIBS_EXT.
-*
-* From GL_ARB_enhanced_layouts:
-*
-* "The resulting stride
Re: [Mesa-dev] [PATCH v2 6/6] glsl/linker: check for xfb_offset aliasing
I'll add locally to the commit log:
Fixes the following test:
KHR-GL44.enhanced_layouts.xfb_output_overlapping
On Tue, 2019-02-12 at 23:57 +0200, Andres Gomez wrote:
> From page 76 (page 80 of the PDF) of the GLSL 4.60 v.5 spec:
>
> " No aliasing in output buffers is allowed: It is a compile-time or
> link-time error to specify variables with overlapping transform
> feedback offsets."
>
> Currently, this is expected to fail, but it succeeds:
>
> "
>
> ...
>
> layout (xfb_offset = 0) out vec2 a;
> layout (xfb_offset = 0) out vec4 b;
>
> ...
>
> "
>
> v2: use a data structure to track the used components instead of a
> nested loop (Ilia).
>
> Cc: Timothy Arceri
> Cc: Ilia Mirkin
> Signed-off-by: Andres Gomez
> ---
> src/compiler/glsl/link_varyings.cpp | 89 ++---
> src/mesa/main/mtypes.h | 3 +
> 2 files changed, 70 insertions(+), 22 deletions(-)
>
> diff --git a/src/compiler/glsl/link_varyings.cpp
> b/src/compiler/glsl/link_varyings.cpp
> index 8c7d6c14c8f..95e9ae895d2 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -1173,6 +1173,73 @@ tfeedback_decl::store(struct gl_context *ctx, struct
> gl_shader_program *prog,
>unsigned location = this->location;
>unsigned location_frac = this->location_frac;
>unsigned num_components = this->num_components();
> +
> + /* From GL_EXT_transform_feedback:
> + * A program will fail to link if:
> + *
> + * * the total number of components to capture is greater than the
> + * constant MAX_TRANSFORM_FEEDBACK_INTERLEAVED_COMPONENTS_EXT and
> + * the buffer mode is INTERLEAVED_ATTRIBS_EXT.
> + *
> + * From GL_ARB_enhanced_layouts:
> + *
> + * "The resulting stride (implicit or explicit) must be less than or
> + *equal to the implementation-dependent constant
> + *gl_MaxTransformFeedbackInterleavedComponents."
> + */
> + if ((prog->TransformFeedback.BufferMode == GL_INTERLEAVED_ATTRIBS ||
> + has_xfb_qualifiers) &&
> + xfb_offset + num_components >
> + ctx->Const.MaxTransformFeedbackInterleavedComponents) {
> + linker_error(prog,
> + "The MAX_TRANSFORM_FEEDBACK_INTERLEAVED_COMPONENTS "
> + "limit has been exceeded.");
> + return false;
> + }
> +
> + {
> + /* From the OpenGL 4.60.5 spec, section 4.4.2. Output Layout
> + * Qualifiers, Page 76, (Transform Feedback Layout Qualifiers):
> + *
> + * "No aliasing in output buffers is allowed: It is a compile-time
> or
> + * link-time error to specify variables with overlapping transform
> + * feedback offsets."
> + */
> + const unsigned max_components =
> +ctx->Const.MaxTransformFeedbackInterleavedComponents;
> + const unsigned first_component = xfb_offset;
> + const unsigned last_component = xfb_offset + num_components - 1;
> + const unsigned start_word = BITSET_BITWORD(first_component);
> + const unsigned end_word = BITSET_BITWORD(last_component);
> + assert(last_component < max_components);
> +
> + if (!info->Buffers[buffer].UsedComponents) {
> +info->Buffers[buffer].UsedComponents =
> + rzalloc_array(info, BITSET_WORD,
> BITSET_WORDS(max_components));
> + }
> + BITSET_WORD *used = info->Buffers[buffer].UsedComponents;
> +
> + for (unsigned word = start_word; word <= end_word; word++) {
> +unsigned start_range = 0;
> +unsigned end_range = BITSET_WORDBITS - 1;
> +
> +if (word == start_word)
> + start_range = first_component % BITSET_WORDBITS;
> +
> +if (word == end_word)
> + end_range = last_component % BITSET_WORDBITS;
> +
> +if (used[word] & BITSET_RANGE(start_range, end_range)) {
> + linker_error(prog,
> +"variable '%s', xfb_offset (%d) "
> +"is causing aliasing.",
> +this->orig_name, xfb_offset * 4);
> + return false;
> +}
> +used[word] |= BITSET_RANGE(start_range, end_range);
> + }
> + }
> +
>while (num_components > 0) {
> unsigned output_size = MIN2(num_components, 4 - location_frac);
> assert((info->NumOutputs == 0 && max_outputs == 0) ||
> @@ -1223,28 +1290,6 @@ tfeedback_decl::store(struct gl_context *ctx, struct
> gl_shader_program *prog,
>info->Buffers[buffer].Stride = xfb_offset;
> }
>
> - /* From GL_EXT_transform_feedback:
> -* A program will fail to link if:
> -*
> -* * the total number of components to capture is greater than
> -* the constant
