Re: [Mesa-dev] [RFC PATCH 06/12] glsl: dont allow gl_PerVertex to be redeclared as an array of arrays
On Mon, 2015-03-23 at 08:58 -0400, Ilia Mirkin wrote:
> On Sat, Mar 21, 2015 at 11:00 PM, Timothy Arceri
> wrote:
> > On Sat, 2015-03-21 at 19:34 -0400, Ilia Mirkin wrote:
> >> On Sat, Mar 21, 2015 at 5:49 AM, Timothy Arceri
> >> wrote:
> >> > Signed-off-by: Timothy Arceri
> >> > ---
> >> > src/glsl/ast_to_hir.cpp | 6 +-
> >> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> >> > index 36f3eb7..7516e60 100644
> >> > --- a/src/glsl/ast_to_hir.cpp
> >> > +++ b/src/glsl/ast_to_hir.cpp
> >> > @@ -5595,7 +5595,11 @@ ast_interface_block::hir(exec_list *instructions,
> >> >
> >> > _mesa_shader_stage_to_string(state->stage));
> >> > }
> >> > if (this->instance_name == NULL ||
> >> > - strcmp(this->instance_name, "gl_in") != 0 ||
> >> > this->array_specifier == NULL) {
> >> > + strcmp(this->instance_name, "gl_in") != 0 ||
> >> > this->array_specifier == NULL ||
> >> > + (this->array_specifier->is_unsized_array &&
> >> > + !this->array_specifier->array_dimensions.is_empty()) ||
> >>
> >> Don't know a lot about AST, but does it make sense to have
> >> ->is_unsized_array && array_dimensions? When would that be true?
> >> Shouldn't this just be !dim.empty() without the is_unsized_array
> >> check?
> >
> > ast is a pain to work with. I don't recall why I wrote it that way but
> > it should be easy to check as it was done so the piglit tests would
> > pass. I will take another look and see if I can rework it to be a little
> > cleaner. Otherwise I might just drop this patch for now.
> >
>
> Please do, but I think that you do need the modification... just not
> the ->is_unsized_array bit of it. I think you want something like
>
> || !is_empty || !is_tail
>
> Basically you don't want gl_in[5], nor gl_in[5][] nor gl_in[][5] nor
> gl_in[][]. Right?
ok, so this is from the piglit test
glsl-1.50/compiler/gs-redeclares-pervertex-in-with-array-size.geom:
// Although the example mentions "gl_in[]", it seems reasonable to
// assume that specifying an array size (e.g. "gl_in[3]") is also
// allowed (this is confirmed by tests of the NVIDIA proprietary
// driver for Linux, version 313.18).
So we don't want gl_in, nor gl_in[5][] nor gl_in[][5] nor gl_in[][] but
gl_in[5] and gl_in[] are ok.
To answer your question from your first reply
> does it make sense to have ->is_unsized_array && array_dimensions?
When would that be true?
An unsized array doesn't have add anything to array_dimensions. So it
would be true for gl_in[][5].
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC PATCH 06/12] glsl: dont allow gl_PerVertex to be redeclared as an array of arrays
On Sat, Mar 21, 2015 at 11:00 PM, Timothy Arceri wrote:
> On Sat, 2015-03-21 at 19:34 -0400, Ilia Mirkin wrote:
>> On Sat, Mar 21, 2015 at 5:49 AM, Timothy Arceri
>> wrote:
>> > Signed-off-by: Timothy Arceri
>> > ---
>> > src/glsl/ast_to_hir.cpp | 6 +-
>> > 1 file changed, 5 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>> > index 36f3eb7..7516e60 100644
>> > --- a/src/glsl/ast_to_hir.cpp
>> > +++ b/src/glsl/ast_to_hir.cpp
>> > @@ -5595,7 +5595,11 @@ ast_interface_block::hir(exec_list *instructions,
>> > _mesa_shader_stage_to_string(state->stage));
>> > }
>> > if (this->instance_name == NULL ||
>> > - strcmp(this->instance_name, "gl_in") != 0 ||
>> > this->array_specifier == NULL) {
>> > + strcmp(this->instance_name, "gl_in") != 0 ||
>> > this->array_specifier == NULL ||
>> > + (this->array_specifier->is_unsized_array &&
>> > + !this->array_specifier->array_dimensions.is_empty()) ||
>>
>> Don't know a lot about AST, but does it make sense to have
>> ->is_unsized_array && array_dimensions? When would that be true?
>> Shouldn't this just be !dim.empty() without the is_unsized_array
>> check?
>
> ast is a pain to work with. I don't recall why I wrote it that way but
> it should be easy to check as it was done so the piglit tests would
> pass. I will take another look and see if I can rework it to be a little
> cleaner. Otherwise I might just drop this patch for now.
>
Please do, but I think that you do need the modification... just not
the ->is_unsized_array bit of it. I think you want something like
|| !is_empty || !is_tail
Basically you don't want gl_in[5], nor gl_in[5][] nor gl_in[][5] nor
gl_in[][]. Right?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC PATCH 06/12] glsl: dont allow gl_PerVertex to be redeclared as an array of arrays
On Sat, 2015-03-21 at 19:34 -0400, Ilia Mirkin wrote:
> On Sat, Mar 21, 2015 at 5:49 AM, Timothy Arceri wrote:
> > Signed-off-by: Timothy Arceri
> > ---
> > src/glsl/ast_to_hir.cpp | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> > index 36f3eb7..7516e60 100644
> > --- a/src/glsl/ast_to_hir.cpp
> > +++ b/src/glsl/ast_to_hir.cpp
> > @@ -5595,7 +5595,11 @@ ast_interface_block::hir(exec_list *instructions,
> > _mesa_shader_stage_to_string(state->stage));
> > }
> > if (this->instance_name == NULL ||
> > - strcmp(this->instance_name, "gl_in") != 0 ||
> > this->array_specifier == NULL) {
> > + strcmp(this->instance_name, "gl_in") != 0 ||
> > this->array_specifier == NULL ||
> > + (this->array_specifier->is_unsized_array &&
> > + !this->array_specifier->array_dimensions.is_empty()) ||
>
> Don't know a lot about AST, but does it make sense to have
> ->is_unsized_array && array_dimensions? When would that be true?
> Shouldn't this just be !dim.empty() without the is_unsized_array
> check?
ast is a pain to work with. I don't recall why I wrote it that way but
it should be easy to check as it was done so the piglit tests would
pass. I will take another look and see if I can rework it to be a little
cleaner. Otherwise I might just drop this patch for now.
>
> > + (this->array_specifier->array_dimensions.tail_pred->prev !=
> > NULL &&
> > +
> > !this->array_specifier->array_dimensions.tail_pred->prev->is_head_sentinel()))
> > {
>
> Perhaps all this would be easier to read if above you wrote
>
> foo_type *array = this->array_specifier;
>
> Not sure. Perhaps there are some helpers to make this easier to grok?
>
> > _mesa_glsl_error(&loc, state,
> > "gl_PerVertex input must be redeclared as "
> > "gl_in[]");
> > --
> > 2.1.0
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC PATCH 06/12] glsl: dont allow gl_PerVertex to be redeclared as an array of arrays
On Sat, Mar 21, 2015 at 5:49 AM, Timothy Arceri wrote:
> Signed-off-by: Timothy Arceri
> ---
> src/glsl/ast_to_hir.cpp | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 36f3eb7..7516e60 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -5595,7 +5595,11 @@ ast_interface_block::hir(exec_list *instructions,
> _mesa_shader_stage_to_string(state->stage));
> }
> if (this->instance_name == NULL ||
> - strcmp(this->instance_name, "gl_in") != 0 ||
> this->array_specifier == NULL) {
> + strcmp(this->instance_name, "gl_in") != 0 ||
> this->array_specifier == NULL ||
> + (this->array_specifier->is_unsized_array &&
> + !this->array_specifier->array_dimensions.is_empty()) ||
Don't know a lot about AST, but does it make sense to have
->is_unsized_array && array_dimensions? When would that be true?
Shouldn't this just be !dim.empty() without the is_unsized_array
check?
> + (this->array_specifier->array_dimensions.tail_pred->prev !=
> NULL &&
> +
> !this->array_specifier->array_dimensions.tail_pred->prev->is_head_sentinel()))
> {
Perhaps all this would be easier to read if above you wrote
foo_type *array = this->array_specifier;
Not sure. Perhaps there are some helpers to make this easier to grok?
> _mesa_glsl_error(&loc, state,
> "gl_PerVertex input must be redeclared as "
> "gl_in[]");
> --
> 2.1.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH 06/12] glsl: dont allow gl_PerVertex to be redeclared as an array of arrays
Signed-off-by: Timothy Arceri
---
src/glsl/ast_to_hir.cpp | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
index 36f3eb7..7516e60 100644
--- a/src/glsl/ast_to_hir.cpp
+++ b/src/glsl/ast_to_hir.cpp
@@ -5595,7 +5595,11 @@ ast_interface_block::hir(exec_list *instructions,
_mesa_shader_stage_to_string(state->stage));
}
if (this->instance_name == NULL ||
- strcmp(this->instance_name, "gl_in") != 0 ||
this->array_specifier == NULL) {
+ strcmp(this->instance_name, "gl_in") != 0 ||
this->array_specifier == NULL ||
+ (this->array_specifier->is_unsized_array &&
+ !this->array_specifier->array_dimensions.is_empty()) ||
+ (this->array_specifier->array_dimensions.tail_pred->prev != NULL
&&
+
!this->array_specifier->array_dimensions.tail_pred->prev->is_head_sentinel())) {
_mesa_glsl_error(&loc, state,
"gl_PerVertex input must be redeclared as "
"gl_in[]");
--
2.1.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
