Re: [Mesa-dev] [PATCH 1.2/8] spirv: Generate spirv_info.c

[Dylan Baker]

Quoting Jason Ekstrand (2017-07-17 10:42:05)
> Generally available things tend to be painful in Python because you have to 
> set
> the python path if you ever want to import anything that isn't in your
> directory.  That doesn't mean we shouldn't do it, just that the pain may be 
> too
> high.  Also, having a copyright block at the top of the file as suggested 
> means
> that the python file still has a copyright block.

I agree about the pain of PYTHONPATH, but putting an assignment before imports
is a really, really, really, really bad idea. In python import is an overloaded
keyword, it does two related things. It does the familiar "give me module  in
my namespace", but it also can change the semantics of the language. That's what
"from __future__ import " does, and one of the things you can change that way
is whether a string is a unicode or a bytearray in python2 (they default to
bytearrays in python2, but unicode in python3), which is something we'll want to
toggle if hybridize for python2 and 3. What would happen in this file now if we
added "from __future__ import unicode_literals" is that the COPYRIGHT would
visually look like a unicode object, but would actually be a bytearray, while
the rest of the naked strings in the file would be unicode.

What would actually be much easier, is putting the function in a mako file and
loading that, mako has filesystem based loader for such things, and injecting
mako into mako templates is pretty easy.  I'll send a patch for that.


signature.asc
Description: signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1.2/8] spirv: Generate spirv_info.c

[Ian Romanick]

On 07/17/2017 10:54 AM, Emil Velikov wrote:
> On 15 July 2017 at 02:39, Ian Romanick  wrote:
>> From: Ian Romanick 
>>
>> The old table based spirv_*_to_string functions would return NULL for
>> any values "inside" the table that didn't have entries.  The tables also
>> needed to be updated by hand each time a new spirv.h was imported.
>> Generate the file instead.
>>
>> Signed-off-by: Ian Romanick 
>> Suggested-by: Jason Ekstrand 
>> ---
>> I am pretty sure the scons changes are bogus... halp, plz?
>>
> From a quick look the following should do it. Can resend as inline,
> etc - let me know what you prefer.
> 
> https://github.com/evelikov/Mesa/commit/d8d808db279e8edba441c3416c87324a87a3bff5.patch
> 
>>  src/compiler/Makefile.am   |   2 +
>>  src/compiler/Makefile.nir.am   |   1 +
>>  src/compiler/Makefile.sources  |   4 +-
>>  src/compiler/Makefile.spirv.am |  31 
>>  src/compiler/SConscript|   1 +
>>  src/compiler/SConscript.spirv  |  32 
>>  src/compiler/spirv/spirv_info.c| 156 
>> -
>>  src/compiler/spirv/spirv_info_c.py |  90 +
>>  8 files changed, 160 insertions(+), 157 deletions(-)
>>  create mode 100644 src/compiler/Makefile.spirv.am
>>  create mode 100644 src/compiler/SConscript.spirv
>>  delete mode 100644 src/compiler/spirv/spirv_info.c
>>  create mode 100644 src/compiler/spirv/spirv_info_c.py
>>
> I think you missed "git add spirvjson"

That's in the previous patch... which I guess should also add that file
to EXTRA_DIST, right?

> About keeping the JSON + header file in sync one could do try the following:
>  - Add simple bash/other script which parses the revision (other
> unique field) from the files and error out on mismatch.
>  - Wire said script to `make check'
>  - ...
>  - Profit :-)
> 
> -Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1.2/8] spirv: Generate spirv_info.c

[Emil Velikov]

On 17 July 2017 at 18:54, Emil Velikov  wrote:
> I think you missed "git add spirvjson"

Nope, it's there as PATCH 1.1/8. Pardon for the noise.

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1.2/8] spirv: Generate spirv_info.c

[Emil Velikov]

On 15 July 2017 at 02:39, Ian Romanick  wrote:
> From: Ian Romanick 
>
> The old table based spirv_*_to_string functions would return NULL for
> any values "inside" the table that didn't have entries.  The tables also
> needed to be updated by hand each time a new spirv.h was imported.
> Generate the file instead.
>
> Signed-off-by: Ian Romanick 
> Suggested-by: Jason Ekstrand 
> ---
> I am pretty sure the scons changes are bogus... halp, plz?
>
From a quick look the following should do it. Can resend as inline,
etc - let me know what you prefer.

https://github.com/evelikov/Mesa/commit/d8d808db279e8edba441c3416c87324a87a3bff5.patch

>  src/compiler/Makefile.am   |   2 +
>  src/compiler/Makefile.nir.am   |   1 +
>  src/compiler/Makefile.sources  |   4 +-
>  src/compiler/Makefile.spirv.am |  31 
>  src/compiler/SConscript|   1 +
>  src/compiler/SConscript.spirv  |  32 
>  src/compiler/spirv/spirv_info.c| 156 
> -
>  src/compiler/spirv/spirv_info_c.py |  90 +
>  8 files changed, 160 insertions(+), 157 deletions(-)
>  create mode 100644 src/compiler/Makefile.spirv.am
>  create mode 100644 src/compiler/SConscript.spirv
>  delete mode 100644 src/compiler/spirv/spirv_info.c
>  create mode 100644 src/compiler/spirv/spirv_info_c.py
>
I think you missed "git add spirvjson"

About keeping the JSON + header file in sync one could do try the following:
 - Add simple bash/other script which parses the revision (other
unique field) from the files and error out on mismatch.
 - Wire said script to `make check'
 - ...
 - Profit :-)

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1.2/8] spirv: Generate spirv_info.c

[Ian Romanick]

On 07/17/2017 10:09 AM, Ian Romanick wrote:
> On 07/14/2017 09:22 PM, Jason Ekstrand wrote:
>> What you have is fine here but I've been really liking the style that's
>> used in mesa/main/format_fallbacks.py and the anv_extensions.py that I
>> sent out which does
>>
>> COPYRIGHT="""
>> \* The usual C copyright
>>  */
>> """
> 
> I've done this in some other places where the same copyright is used
> with multiple templates.  In the GLX protocol scripts, I actually had a
> function that generated the copyright boilerplate parameterized with a
> list of copyright holders.  Maybe to reduce the copy-and-paste we should
> make something like that generally available?

Now that I've looked at format_fallback.py, I see what you mean.  I like
it.  Ignore my previous comment.

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1.2/8] spirv: Generate spirv_info.c

[Jason Ekstrand]

On Mon, Jul 17, 2017 at 10:09 AM, Ian Romanick  wrote:

> On 07/14/2017 09:22 PM, Jason Ekstrand wrote:
> > On Fri, Jul 14, 2017 at 6:39 PM, Ian Romanick  > > wrote:
> >
> > From: Ian Romanick  > >
> >
> > The old table based spirv_*_to_string functions would return NULL for
> > any values "inside" the table that didn't have entries.  The tables
> also
> > needed to be updated by hand each time a new spirv.h was imported.
> > Generate the file instead.
> >
> >
> > Thanks for working on this!  This is way better than having a
> > hand-rolled table that requires updating.
>
> The one unfortunate thing is that spirv.h and spirv.core.grammar.json
> need to be updated together.
>
> > Signed-off-by: Ian Romanick  > >
> > Suggested-by: Jason Ekstrand  > >
> > ---
> > I am pretty sure the scons changes are bogus... halp, plz?
> >
> >
> > I'm going to let Emil review your autotools and write you some
> Android.mk.
> >
> >
> >  src/compiler/Makefile.am   |   2 +
> >  src/compiler/Makefile.nir.am    |   1 +
> >  src/compiler/Makefile.sources  |   4 +-
> >  src/compiler/Makefile.spirv.am  |  31
> > 
> >  src/compiler/SConscript|   1 +
> >  src/compiler/SConscript.spirv  |  32 
> >  src/compiler/spirv/spirv_info.c| 156
> > -
> >  src/compiler/spirv/spirv_info_c.py |  90 +
> >  8 files changed, 160 insertions(+), 157 deletions(-)
> >  create mode 100644 src/compiler/Makefile.spirv.am
> > 
> >  create mode 100644 src/compiler/SConscript.spirv
> >  delete mode 100644 src/compiler/spirv/spirv_info.c
> >  create mode 100644 src/compiler/spirv/spirv_info_c.py
> >
> > diff --git a/src/compiler/Makefile.am b/src/compiler/Makefile.am
> > index 4c83365..b8f6697 100644
> > --- a/src/compiler/Makefile.am
> > +++ b/src/compiler/Makefile.am
> > @@ -63,3 +63,5 @@ PYTHON_GEN = $(AM_V_GEN)$(PYTHON2) $(PYTHON_FLAGS)
> >  include Makefile.glsl.am 
> >
> >  include Makefile.nir.am 
> > +
> > +include Makefile.spirv.am 
> > diff --git a/src/compiler/Makefile.nir.am 
> > b/src/compiler/Makefile.nir.am 
> > index 13f02a7..d10f173 100644
> > --- a/src/compiler/Makefile.nir.am 
> > +++ b/src/compiler/Makefile.nir.am 
> > @@ -29,6 +29,7 @@ nir_libnir_la_LIBADD = \
> >  nir_libnir_la_SOURCES =\
> > $(NIR_FILES)\
> > $(SPIRV_FILES)  \
> > +   $(SPIRV_GENERATED_FILES)\
> > $(NIR_GENERATED_FILES)
> >
> >  nir/nir_builder_opcodes.h: nir/nir_opcodes.py
> > nir/nir_builder_opcodes_h.py
> > diff --git a/src/compiler/Makefile.sources
> > b/src/compiler/Makefile.sources
> > index d3447fb..785782b 100644
> > --- a/src/compiler/Makefile.sources
> > +++ b/src/compiler/Makefile.sources
> > @@ -277,12 +277,14 @@ NIR_FILES = \
> > nir/nir_worklist.c \
> > nir/nir_worklist.h
> >
> > +SPIRV_GENERATED_FILES = \
> > +   spirv/spirv_info.c
> > +
> >  SPIRV_FILES = \
> > spirv/GLSL.std.450.h \
> > spirv/nir_spirv.h \
> > spirv/spirv.h \
> > spirv/spirv_info.h \
> > -   spirv/spirv_info.c \
> > spirv/spirv_to_nir.c \
> > spirv/vtn_alu.c \
> > spirv/vtn_cfg.c \
> > diff --git a/src/compiler/Makefile.spirv.am
> >  b/src/compiler/Makefile.spirv.am
> > 
> > new file mode 100644
> > index 000..6d5dfc0
> > --- /dev/null
> > +++ b/src/compiler/Makefile.spirv.am 
> > @@ -0,0 +1,31 @@
> > +# Copyright (C) 2017 Intel Corporation
> > +#
> > +# Permission is hereby granted, free of charge, to any person
> > obtaining a
> > +# copy of this software and associated documentation files (the
> > "Software"),
> > +# to deal in the Software without restriction, including without
> > limitation
> > +# the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > +# and/or sell copies of the Software, and to permit persons to whom
> the
> > +# Software is furnished to do so, subject to the following
> 

Re: [Mesa-dev] [PATCH 1.2/8] spirv: Generate spirv_info.c

[Ian Romanick]

On 07/14/2017 09:22 PM, Jason Ekstrand wrote:
> On Fri, Jul 14, 2017 at 6:39 PM, Ian Romanick  > wrote:
> 
> From: Ian Romanick  >
> 
> The old table based spirv_*_to_string functions would return NULL for
> any values "inside" the table that didn't have entries.  The tables also
> needed to be updated by hand each time a new spirv.h was imported.
> Generate the file instead.
> 
> 
> Thanks for working on this!  This is way better than having a
> hand-rolled table that requires updating.

The one unfortunate thing is that spirv.h and spirv.core.grammar.json
need to be updated together.

> Signed-off-by: Ian Romanick  >
> Suggested-by: Jason Ekstrand  >
> ---
> I am pretty sure the scons changes are bogus... halp, plz?
> 
> 
> I'm going to let Emil review your autotools and write you some Android.mk.
>  
> 
>  src/compiler/Makefile.am   |   2 +
>  src/compiler/Makefile.nir.am    |   1 +
>  src/compiler/Makefile.sources  |   4 +-
>  src/compiler/Makefile.spirv.am  |  31
> 
>  src/compiler/SConscript|   1 +
>  src/compiler/SConscript.spirv  |  32 
>  src/compiler/spirv/spirv_info.c| 156
> -
>  src/compiler/spirv/spirv_info_c.py |  90 +
>  8 files changed, 160 insertions(+), 157 deletions(-)
>  create mode 100644 src/compiler/Makefile.spirv.am
> 
>  create mode 100644 src/compiler/SConscript.spirv
>  delete mode 100644 src/compiler/spirv/spirv_info.c
>  create mode 100644 src/compiler/spirv/spirv_info_c.py
> 
> diff --git a/src/compiler/Makefile.am b/src/compiler/Makefile.am
> index 4c83365..b8f6697 100644
> --- a/src/compiler/Makefile.am
> +++ b/src/compiler/Makefile.am
> @@ -63,3 +63,5 @@ PYTHON_GEN = $(AM_V_GEN)$(PYTHON2) $(PYTHON_FLAGS)
>  include Makefile.glsl.am 
> 
>  include Makefile.nir.am 
> +
> +include Makefile.spirv.am 
> diff --git a/src/compiler/Makefile.nir.am 
> b/src/compiler/Makefile.nir.am 
> index 13f02a7..d10f173 100644
> --- a/src/compiler/Makefile.nir.am 
> +++ b/src/compiler/Makefile.nir.am 
> @@ -29,6 +29,7 @@ nir_libnir_la_LIBADD = \
>  nir_libnir_la_SOURCES =\
> $(NIR_FILES)\
> $(SPIRV_FILES)  \
> +   $(SPIRV_GENERATED_FILES)\
> $(NIR_GENERATED_FILES)
> 
>  nir/nir_builder_opcodes.h: nir/nir_opcodes.py
> nir/nir_builder_opcodes_h.py
> diff --git a/src/compiler/Makefile.sources
> b/src/compiler/Makefile.sources
> index d3447fb..785782b 100644
> --- a/src/compiler/Makefile.sources
> +++ b/src/compiler/Makefile.sources
> @@ -277,12 +277,14 @@ NIR_FILES = \
> nir/nir_worklist.c \
> nir/nir_worklist.h
> 
> +SPIRV_GENERATED_FILES = \
> +   spirv/spirv_info.c
> +
>  SPIRV_FILES = \
> spirv/GLSL.std.450.h \
> spirv/nir_spirv.h \
> spirv/spirv.h \
> spirv/spirv_info.h \
> -   spirv/spirv_info.c \
> spirv/spirv_to_nir.c \
> spirv/vtn_alu.c \
> spirv/vtn_cfg.c \
> diff --git a/src/compiler/Makefile.spirv.am
>  b/src/compiler/Makefile.spirv.am
> 
> new file mode 100644
> index 000..6d5dfc0
> --- /dev/null
> +++ b/src/compiler/Makefile.spirv.am 
> @@ -0,0 +1,31 @@
> +# Copyright (C) 2017 Intel Corporation
> +#
> +# Permission is hereby granted, free of charge, to any person
> obtaining a
> +# copy of this software and associated documentation files (the
> "Software"),
> +# to deal in the Software without restriction, including without
> limitation
> +# the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> +# and/or sell copies of the Software, and to permit persons to whom the
> +# Software is furnished to do so, subject to the following conditions:
> +#
> +# The above copyright notice and this permission notice (including
> the next
> +# paragraph) shall be included in all copies or substantial
> portions of the
> +# Software.
> +#
> +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,

Re: [Mesa-dev] [PATCH 1.2/8] spirv: Generate spirv_info.c

[Jason Ekstrand]

On Fri, Jul 14, 2017 at 6:39 PM, Ian Romanick  wrote:

> From: Ian Romanick 
>
> The old table based spirv_*_to_string functions would return NULL for
> any values "inside" the table that didn't have entries.  The tables also
> needed to be updated by hand each time a new spirv.h was imported.
> Generate the file instead.
>

Thanks for working on this!  This is way better than having a hand-rolled
table that requires updating.


> Signed-off-by: Ian Romanick 
> Suggested-by: Jason Ekstrand 
> ---
> I am pretty sure the scons changes are bogus... halp, plz?
>

I'm going to let Emil review your autotools and write you some Android.mk.


>  src/compiler/Makefile.am   |   2 +
>  src/compiler/Makefile.nir.am   |   1 +
>  src/compiler/Makefile.sources  |   4 +-
>  src/compiler/Makefile.spirv.am |  31 
>  src/compiler/SConscript|   1 +
>  src/compiler/SConscript.spirv  |  32 
>  src/compiler/spirv/spirv_info.c| 156 --
> ---
>  src/compiler/spirv/spirv_info_c.py |  90 +
>  8 files changed, 160 insertions(+), 157 deletions(-)
>  create mode 100644 src/compiler/Makefile.spirv.am
>  create mode 100644 src/compiler/SConscript.spirv
>  delete mode 100644 src/compiler/spirv/spirv_info.c
>  create mode 100644 src/compiler/spirv/spirv_info_c.py
>
> diff --git a/src/compiler/Makefile.am b/src/compiler/Makefile.am
> index 4c83365..b8f6697 100644
> --- a/src/compiler/Makefile.am
> +++ b/src/compiler/Makefile.am
> @@ -63,3 +63,5 @@ PYTHON_GEN = $(AM_V_GEN)$(PYTHON2) $(PYTHON_FLAGS)
>  include Makefile.glsl.am
>
>  include Makefile.nir.am
> +
> +include Makefile.spirv.am
> diff --git a/src/compiler/Makefile.nir.am b/src/compiler/Makefile.nir.am
> index 13f02a7..d10f173 100644
> --- a/src/compiler/Makefile.nir.am
> +++ b/src/compiler/Makefile.nir.am
> @@ -29,6 +29,7 @@ nir_libnir_la_LIBADD = \
>  nir_libnir_la_SOURCES =\
> $(NIR_FILES)\
> $(SPIRV_FILES)  \
> +   $(SPIRV_GENERATED_FILES)\
> $(NIR_GENERATED_FILES)
>
>  nir/nir_builder_opcodes.h: nir/nir_opcodes.py nir/nir_builder_opcodes_h.py
> diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources
> index d3447fb..785782b 100644
> --- a/src/compiler/Makefile.sources
> +++ b/src/compiler/Makefile.sources
> @@ -277,12 +277,14 @@ NIR_FILES = \
> nir/nir_worklist.c \
> nir/nir_worklist.h
>
> +SPIRV_GENERATED_FILES = \
> +   spirv/spirv_info.c
> +
>  SPIRV_FILES = \
> spirv/GLSL.std.450.h \
> spirv/nir_spirv.h \
> spirv/spirv.h \
> spirv/spirv_info.h \
> -   spirv/spirv_info.c \
> spirv/spirv_to_nir.c \
> spirv/vtn_alu.c \
> spirv/vtn_cfg.c \
> diff --git a/src/compiler/Makefile.spirv.am b/src/compiler/Makefile.spirv.
> am
> new file mode 100644
> index 000..6d5dfc0
> --- /dev/null
> +++ b/src/compiler/Makefile.spirv.am
> @@ -0,0 +1,31 @@
> +# Copyright (C) 2017 Intel Corporation
> +#
> +# Permission is hereby granted, free of charge, to any person obtaining a
> +# copy of this software and associated documentation files (the
> "Software"),
> +# to deal in the Software without restriction, including without
> limitation
> +# the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +# and/or sell copies of the Software, and to permit persons to whom the
> +# Software is furnished to do so, subject to the following conditions:
> +#
> +# The above copyright notice and this permission notice (including the
> next
> +# paragraph) shall be included in all copies or substantial portions of
> the
> +# Software.
> +#
> +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> OR
> +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> +# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> +# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> +# IN THE SOFTWARE.
> +
> +spirv/spirv_info.c: spirv/spirv_info_c.py spirv/spirv.core.grammar.json
> +   $(MKDIR_GEN)
> +   $(PYTHON_GEN) $(srcdir)/spirv/spirv_info_c.py > $@ || ($(RM) $@;
> false)
>

I'm sorry for all the pain I'm sure this caused you...  Being the first one
to add auto-generation to a directory is the worst...


> +
> +BUILT_SOURCES += $(SPIRV_GENERATED_FILES)
> +CLEANFILES += $(SPIRV_GENERATED_FILES)
> +
> +EXTRA_DIST += \
> +   spirv/spirv_info_c.py \
> +   spirv/spirv.core.grammar.json
> diff --git a/src/compiler/SConscript b/src/compiler/SConscript
> index 44509a9..2797abe 100644
> ---