On 07/14/2017 09:22 PM, Jason Ekstrand wrote: > On Fri, Jul 14, 2017 at 6:39 PM, Ian Romanick <i...@freedesktop.org > <mailto:i...@freedesktop.org>> wrote: > > From: Ian Romanick <ian.d.roman...@intel.com > <mailto:ian.d.roman...@intel.com>> > > 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 <ian.d.roman...@intel.com > <mailto:ian.d.roman...@intel.com>> > Suggested-by: Jason Ekstrand <ja...@jlekstrand.net > <mailto:ja...@jlekstrand.net>> > --- > 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 <http://Makefile.nir.am> | 1 + > src/compiler/Makefile.sources | 4 +- > src/compiler/Makefile.spirv.am <http://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 > <http://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 <http://Makefile.glsl.am> > > include Makefile.nir.am <http://Makefile.nir.am> > + > +include Makefile.spirv.am <http://Makefile.spirv.am> > diff --git a/src/compiler/Makefile.nir.am <http://Makefile.nir.am> > b/src/compiler/Makefile.nir.am <http://Makefile.nir.am> > index 13f02a7..d10f173 100644 > --- a/src/compiler/Makefile.nir.am <http://Makefile.nir.am> > +++ b/src/compiler/Makefile.nir.am <http://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 > <http://Makefile.spirv.am> b/src/compiler/Makefile.spirv.am > <http://Makefile.spirv.am> > new file mode 100644 > index 0000000..6d5dfc0 > --- /dev/null > +++ b/src/compiler/Makefile.spirv.am <http://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... It turns out that the biggest problem was me typoing SPRIV instead of SPIRV in a couple places. Once Ken pointed that out to me, everything worked. Sigh... > + > +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 > --- a/src/compiler/SConscript > +++ b/src/compiler/SConscript > @@ -26,4 +26,5 @@ compiler = env.ConvenienceLibrary( > Export('compiler') > > SConscript('SConscript.glsl') > +SConscript('SConscript.spirv') > SConscript('SConscript.nir') > diff --git a/src/compiler/SConscript.spirv > b/src/compiler/SConscript.spirv > new file mode 100644 > index 0000000..8a90c2f > --- /dev/null > +++ b/src/compiler/SConscript.spirv > @@ -0,0 +1,32 @@ > +import common > + > +Import('*') > + > +from sys import executable as python_cmd > + > +env = env.Clone() > + > +env.MSVC2013Compat() > + > +env.Prepend(CPPPATH = [ > + '#include', > + '#src', > + '#src/mapi', > + '#src/mesa', > + '#src/gallium/include', > + '#src/gallium/auxiliary', > + '#src/compiler/nir', > +]) > + > +# Make generated headers reachable from the include path. > +env.Prepend(CPPPATH = [Dir('.').abspath, Dir('glsl').abspath]) > +env.Prepend(CPPPATH = [Dir('.').abspath, Dir('nir').abspath]) > + > +# SPIR-V generated sources > + > +spirv_info_c = env.CodeGenerate( > + target = 'spirv/spirv_info.c', > + script = 'spirv/spirv_info_c.py', > + source = [], > + command = python_cmd + ' $SCRIPT > $TARGET' > +) > diff --git a/src/compiler/spirv/spirv_info.c > b/src/compiler/spirv/spirv_info.c > deleted file mode 100644 > index 1036b41..0000000 > --- a/src/compiler/spirv/spirv_info.c > +++ /dev/null > @@ -1,156 +0,0 @@ > -/* > - * Copyright © 2016 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. > - */ > - > -#include "spirv_info.h" > -#include "util/macros.h" > - > -#define CAPABILITY(cap) [SpvCapability##cap] = #cap > -static const char * const capability_to_string[] = { > - CAPABILITY(Matrix), > - CAPABILITY(Shader), > - CAPABILITY(Geometry), > - CAPABILITY(Tessellation), > - CAPABILITY(Addresses), > - CAPABILITY(Linkage), > - CAPABILITY(Kernel), > - CAPABILITY(Vector16), > - CAPABILITY(Float16Buffer), > - CAPABILITY(Float16), > - CAPABILITY(Float64), > - CAPABILITY(Int64), > - CAPABILITY(Int64Atomics), > - CAPABILITY(ImageBasic), > - CAPABILITY(ImageReadWrite), > - CAPABILITY(ImageMipmap), > - CAPABILITY(Pipes), > - CAPABILITY(Groups), > - CAPABILITY(DeviceEnqueue), > - CAPABILITY(LiteralSampler), > - CAPABILITY(AtomicStorage), > - CAPABILITY(Int16), > - CAPABILITY(TessellationPointSize), > - CAPABILITY(GeometryPointSize), > - CAPABILITY(ImageGatherExtended), > - CAPABILITY(StorageImageMultisample), > - CAPABILITY(UniformBufferArrayDynamicIndexing), > - CAPABILITY(SampledImageArrayDynamicIndexing), > - CAPABILITY(StorageBufferArrayDynamicIndexing), > - CAPABILITY(StorageImageArrayDynamicIndexing), > - CAPABILITY(ClipDistance), > - CAPABILITY(CullDistance), > - CAPABILITY(ImageCubeArray), > - CAPABILITY(SampleRateShading), > - CAPABILITY(ImageRect), > - CAPABILITY(SampledRect), > - CAPABILITY(GenericPointer), > - CAPABILITY(Int8), > - CAPABILITY(InputAttachment), > - CAPABILITY(SparseResidency), > - CAPABILITY(MinLod), > - CAPABILITY(Sampled1D), > - CAPABILITY(Image1D), > - CAPABILITY(SampledCubeArray), > - CAPABILITY(SampledBuffer), > - CAPABILITY(ImageBuffer), > - CAPABILITY(ImageMSArray), > - CAPABILITY(StorageImageExtendedFormats), > - CAPABILITY(ImageQuery), > - CAPABILITY(DerivativeControl), > - CAPABILITY(InterpolationFunction), > - CAPABILITY(TransformFeedback), > - CAPABILITY(GeometryStreams), > - CAPABILITY(StorageImageReadWithoutFormat), > - CAPABILITY(StorageImageWriteWithoutFormat), > - CAPABILITY(MultiViewport), > - CAPABILITY(SubgroupDispatch), > - CAPABILITY(NamedBarrier), > - CAPABILITY(PipeStorage), > - CAPABILITY(SubgroupBallotKHR), > - CAPABILITY(DrawParameters), > -}; > - > -const char * > -spirv_capability_to_string(SpvCapability cap) > -{ > - if (cap < ARRAY_SIZE(capability_to_string)) > - return capability_to_string[cap]; > - else > - return "unknown"; > -} > - > -#define DECORATION(dec) [SpvDecoration##dec] = #dec > -static const char * const decoration_to_string[] = { > - DECORATION(RelaxedPrecision), > - DECORATION(SpecId), > - DECORATION(Block), > - DECORATION(BufferBlock), > - DECORATION(RowMajor), > - DECORATION(ColMajor), > - DECORATION(ArrayStride), > - DECORATION(MatrixStride), > - DECORATION(GLSLShared), > - DECORATION(GLSLPacked), > - DECORATION(CPacked), > - DECORATION(BuiltIn), > - DECORATION(NoPerspective), > - DECORATION(Flat), > - DECORATION(Patch), > - DECORATION(Centroid), > - DECORATION(Sample), > - DECORATION(Invariant), > - DECORATION(Restrict), > - DECORATION(Aliased), > - DECORATION(Volatile), > - DECORATION(Constant), > - DECORATION(Coherent), > - DECORATION(NonWritable), > - DECORATION(NonReadable), > - DECORATION(Uniform), > - DECORATION(SaturatedConversion), > - DECORATION(Stream), > - DECORATION(Location), > - DECORATION(Component), > - DECORATION(Index), > - DECORATION(Binding), > - DECORATION(DescriptorSet), > - DECORATION(Offset), > - DECORATION(XfbBuffer), > - DECORATION(XfbStride), > - DECORATION(FuncParamAttr), > - DECORATION(FPRoundingMode), > - DECORATION(FPFastMathMode), > - DECORATION(LinkageAttributes), > - DECORATION(NoContraction), > - DECORATION(InputAttachmentIndex), > - DECORATION(Alignment), > - DECORATION(MaxByteOffset), > -}; > - > -const char * > -spirv_decoration_to_string(SpvDecoration dec) > -{ > - if (dec < ARRAY_SIZE(decoration_to_string)) > - return decoration_to_string[dec]; > - else > - return "unknown"; > -} > diff --git a/src/compiler/spirv/spirv_info_c.py > b/src/compiler/spirv/spirv_info_c.py > new file mode 100644 > index 0000000..5af832e > --- /dev/null > +++ b/src/compiler/spirv/spirv_info_c.py > @@ -0,0 +1,90 @@ > +# -*- coding: utf-8 -*- > + > +# Copyright © 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. > > > 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? > and then maybe some functions and then > > TEMPLATE = mako.template.Template(COPYRIGHT + """ > /* My template goes here */ > """) > > Followed by the main function. Putting the template inside the main > function (or if statement in this case) means the indentation gets > really weird. > > > + > +import json > +import mako.template > + > +def collect_data(spirv, kind): > + for x in spirv["operand_kinds"]: > + if x["kind"] == kind: > + operands = x > > > break? Yes. I originally structured this a bit differently (finding both Capability and Decoration in one loop), and that didn't need a break here. I'll add it. > + > + # There are some duplicate values in some of the tables (thanks > guys!), so > + # filter them out. > > > That's unfortunate... > > > + last_value = -1 > + values = [] > + for x in operands["enumerants"]: > + if x["value"] != last_value: > + last_value = x["value"] > + values.append(x["enumerant"]) > > > This assumes that the duplicate values will be sequential. Is that > always the case? I suppose it isn't since the C compiles. Maybe better > to use a set and do > > if x["value"] not in seen_values: > seen_values.add(x["value"]) > > What you have here works though so we can always make that change later > if it's ever a problem. Right now there are two pairs of duplicated values, and they appear sequentially in the file. This happens because the names are currently sorted by value. I don't know if that will always be the case, but I've made that assumption in another script too. I considered a more complex scheme for dealing with this that used another table to track names by value, but I opted for simpler. { "enumerant" : "StorageBuffer16BitAccess", "value" : 4433, "extensions" : [ "SPV_KHR_16bit_storage" ] }, { "enumerant" : "StorageUniformBufferBlock16", "value" : 4433, "extensions" : [ "SPV_KHR_16bit_storage" ] }, { "enumerant" : "UniformAndStorageBuffer16BitAccess", "value" : 4434, "capabilities" : [ "StorageBuffer16BitAccess", "StorageUniformBufferBlock16" ], "extensions" : [ "SPV_KHR_16bit_storage" ] }, { "enumerant" : "StorageUniform16", "value" : 4434, "capabilities" : [ "StorageBuffer16BitAccess", "StorageUniformBufferBlock16" ], "extensions" : [ "SPV_KHR_16bit_storage" ] }, One of the pairs is used as a dependency, and both are listed. This isn't relevant for this script, but it was a hassle for another that I'm writing. { "enumerant" : "UniformAndStorageBuffer16BitAccess", "value" : 4434, "capabilities" : [ "StorageBuffer16BitAccess", "StorageUniformBufferBlock16" ], "extensions" : [ "SPV_KHR_16bit_storage" ] }, { "enumerant" : "StorageUniform16", "value" : 4434, "capabilities" : [ "StorageBuffer16BitAccess", "StorageUniformBufferBlock16" ], "extensions" : [ "SPV_KHR_16bit_storage" ] }, > + > + return (kind, values) > + > +if __name__ == "__main__": > + t = mako.template.Template("""/* > + * 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. > + */ > + > +#include "spirv_info.h" > + % for kind,values in info: > > > Dylan will probably tell you to indent your C code along with the mako. > I, personally, don't care. If anything, I would just dedent all the > mako by a step. But I don't really care all that much. It's readable > as-is and we don't really have a mako style guide. > > > + > +const char * > +spirv_${kind.lower()}_to_string(Spv${kind} v) > +{ > + switch (v) { > + % for name in values: > + case Spv${kind}${name}: return "Spv${kind}${name}"; > + % endfor > + case Spv${kind}Max: break; /* silence warnings about unhandled > enums. */ > + } > + > + return "unknown"; > +} > + % endfor > +""") > + > + spirv_info = > json.JSONDecoder().decode(open("spirv/spirv.core.grammar.json", > "r").read()) > + > + capabilities = collect_data(spirv_info, "Capability") > + decorations = collect_data(spirv_info, "Decoration") > + > + print(t.render(info=[capabilities, decorations])) > > > The more recent generators have started using python's argparse and have > --xml (or --json in this case) parameter for the input and --out for the > output. Take a look at mesa/main/format_fallbacks.py for an example. > Having the generator write the output file makes adding it to automake a > bit easier to do correctly. That's a good suggestion. I'll look into that. I wasn't terribly happy with hard coding the path. > --Jason _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
