[Mesa-dev] [PATCH 2/8] spirv: Add a prepass to set types on vtn_values

Jason Ekstrand jason at jlekstrand.net
Mon Dec 11 22:44:21 UTC 2017


On Mon, Dec 11, 2017 at 11:13 AM, Ian Romanick <idr at freedesktop.org> wrote:

> On 12/11/2017 10:50 AM, Jason Ekstrand wrote:
> > On Mon, Dec 11, 2017 at 9:50 AM, Ian Romanick <idr at freedesktop.org
> > <mailto:idr at freedesktop.org>> wrote:
> >
> >     On 12/07/2017 08:12 AM, Jason Ekstrand wrote:
> >     > This autogenerated pass will automatically find and set the type
> field
> >     > on all vtn_values.  This way we always have the type and can use
> >     it for
> >     > validation and other checks.
> >     > ---
> >     >  src/compiler/Makefile.nir.am <http://Makefile.nir.am>
> >      |   4 +
> >     >  src/compiler/nir/meson.build             |  11 ++-
> >     >  src/compiler/spirv/spirv_to_nir.c        |   6 +-
> >     >  src/compiler/spirv/vtn_gather_types_c.py | 125
> >     +++++++++++++++++++++++++++++++
> >     >  src/compiler/spirv/vtn_private.h         |   4 +
> >     >  5 files changed, 148 insertions(+), 2 deletions(-)
> >     >  create mode 100644 src/compiler/spirv/vtn_gather_types_c.py
> >     >
> >     > diff --git a/src/compiler/Makefile.nir.am <http://Makefile.nir.am>
> >     b/src/compiler/Makefile.nir.am <http://Makefile.nir.am>
> >     > index 1533ee5..dd38c45 100644
> >     > --- a/src/compiler/Makefile.nir.am <http://Makefile.nir.am>
> >     > +++ b/src/compiler/Makefile.nir.am <http://Makefile.nir.am>
> >     > @@ -56,6 +56,10 @@ spirv/spirv_info.c: spirv/spirv_info_c.py
> >     spirv/spirv.core.grammar.json
> >     >       $(MKDIR_GEN)
> >     >       $(PYTHON_GEN) $(srcdir)/spirv/spirv_info_c.py
> >     $(srcdir)/spirv/spirv.core.grammar.json $@ || ($(RM) $@; false)
> >     >
> >     > +spirv/vtn_gather_types.c: spirv/vtn_gather_types_c.py
> >     spirv/spirv.core.grammar.json
> >     > +     $(MKDIR_GEN)
> >     > +     $(PYTHON_GEN) $(srcdir)/spirv/vtn_gather_types_c.py
> >     $(srcdir)/spirv/spirv.core.grammar.json $@ || ($(RM) $@; false)
> >     > +
> >     >  noinst_PROGRAMS += spirv2nir
> >     >
> >     >  spirv2nir_SOURCES = \
> >     > diff --git a/src/compiler/nir/meson.build
> >     b/src/compiler/nir/meson.build
> >     > index b61a077..5dd21e6 100644
> >     > --- a/src/compiler/nir/meson.build
> >     > +++ b/src/compiler/nir/meson.build
> >     > @@ -72,6 +72,14 @@ spirv_info_c = custom_target(
> >     >    command : [prog_python2, '@INPUT0@', '@INPUT1@', '@OUTPUT@'],
> >     >  )
> >     >
> >     > +vtn_gather_types_c = custom_target(
> >     > +  'vtn_gather_types.c',
> >     > +  input : files('../spirv/vtn_gather_types_c.py',
> >     > +                '../spirv/spirv.core.grammar.json'),
> >     > +  output : 'vtn_gather_types.c',
> >     > +  command : [prog_python2, '@INPUT0@', '@INPUT1@', '@OUTPUT@'],
> >     > +)
> >     > +
> >     >  files_libnir = files(
> >     >    'nir.c',
> >     >    'nir.h',
> >     > @@ -189,7 +197,8 @@ files_libnir = files(
> >     >  libnir = static_library(
> >     >    'nir',
> >     >    [files_libnir, spirv_info_c, nir_opt_algebraic_c, nir_opcodes_c,
> >     > -   nir_opcodes_h, nir_constant_expressions_c,
> nir_builder_opcodes_h],
> >     > +   nir_opcodes_h, nir_constant_expressions_c,
> nir_builder_opcodes_h,
> >     > +   vtn_gather_types_c],
> >     >    include_directories : [inc_common, inc_compiler,
> >     include_directories('../spirv')],
> >     >    c_args : [c_vis_args, c_msvc_compat_args,
> no_override_init_args],
> >     >    link_with : libcompiler,
> >     > diff --git a/src/compiler/spirv/spirv_to_nir.c
> >     b/src/compiler/spirv/spirv_to_nir.c
> >     > index a50b14d..a2426bc 100644
> >     > --- a/src/compiler/spirv/spirv_to_nir.c
> >     > +++ b/src/compiler/spirv/spirv_to_nir.c
> >     > @@ -1261,7 +1261,6 @@ vtn_handle_constant(struct vtn_builder *b,
> >     SpvOp opcode,
> >     >                      const uint32_t *w, unsigned count)
> >     >  {
> >     >     struct vtn_value *val = vtn_push_value(b, w[2],
> >     vtn_value_type_constant);
> >     > -   val->type = vtn_value(b, w[1], vtn_value_type_type)->type;
> >     >     val->constant = rzalloc(b, nir_constant);
> >     >     switch (opcode) {
> >     >     case SpvOpConstantTrue:
> >     > @@ -3268,6 +3267,8 @@ static bool
> >     >  vtn_handle_variable_or_type_instruction(struct vtn_builder *b,
> >     SpvOp opcode,
> >     >                                          const uint32_t *w,
> >     unsigned count)
> >     >  {
> >     > +   vtn_set_instruction_result_type(b, opcode, w, count);
> >     > +
> >     >     switch (opcode) {
> >     >     case SpvOpSource:
> >     >     case SpvOpSourceContinued:
> >     > @@ -3658,6 +3659,9 @@ spirv_to_nir(const uint32_t *words, size_t
> >     word_count,
> >     >     words = vtn_foreach_instruction(b, words, word_end,
> >     >
> >      vtn_handle_variable_or_type_instruction);
> >     >
> >     > +   /* Set types on all vtn_values */
> >     > +   vtn_foreach_instruction(b, words, word_end,
> >     vtn_set_instruction_result_type);
> >     > +
> >     >     vtn_build_cfg(b, words, word_end);
> >     >
> >     >     assert(b->entry_point->value_type == vtn_value_type_function);
> >     > diff --git a/src/compiler/spirv/vtn_gather_types_c.py
> >     b/src/compiler/spirv/vtn_gather_types_c.py
> >     > new file mode 100644
> >     > index 0000000..8cd8d9f
> >     > --- /dev/null
> >     > +++ b/src/compiler/spirv/vtn_gather_types_c.py
> >     > @@ -0,0 +1,125 @@
> >     > +COPYRIGHT = """\
> >     > +/*
> >     > + * 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.
> >     > + */
> >     > +"""
> >     > +
> >     > +import argparse
> >     > +import json
> >     > +from sys import stdout
> >     > +from mako.template import Template
> >     > +
> >     > +def find_result_types(spirv):
> >     > +    for inst in spirv['instructions']:
> >     > +        name = inst['opname']
> >     > +
> >     > +        if 'operands' not in inst:
> >     > +            continue
> >     > +
> >     > +        res_arg_idx = -1
> >     > +        res_type_arg_idx = -1
> >     > +        for idx, arg in enumerate(inst['operands']):
> >     > +            if arg['kind'] == 'IdResult':
> >     > +                res_arg_idx = idx
> >     > +            elif arg['kind'] == 'IdResultType':
> >     > +                res_type_arg_idx = idx
> >     > +
> >     > +        if res_type_arg_idx >= 0:
> >     > +            assert res_arg_idx >= 0
> >     > +        elif res_arg_idx >= 0:
> >     > +            untyped_insts = [
> >     > +                'OpString',
> >     > +                'OpExtInstImport',
> >     > +                'OpDecorationGroup',
> >     > +                'OpLabel',
> >     > +            ]
> >     > +            assert name.startswith('OpType') or name in
> untyped_insts
> >     > +
> >     > +        if res_arg_idx >= 0 or res_type_arg_idx >= 0:
> >     > +            yield (name, res_arg_idx, res_type_arg_idx)
> >     > +
> >     > +TEMPLATE  = Template(COPYRIGHT + """\
> >     > +
> >     > +/* DO NOT EDIT - This file is generated automatically by the
> >     > + * vtn_gather_types_c.py script
> >     > + */
> >     > +
> >     > +#include "vtn_private.h"
> >     > +
> >     > +struct type_args {
> >     > +    int res_idx;
> >     > +    int res_type_idx;
> >     > +};
> >     > +
> >     > +static struct type_args
> >     > +result_type_args_for_opcode(SpvOp opcode)
> >     > +{
> >     > +   switch (opcode) {
> >     > +% for opcode in opcodes:
> >     > +   case Spv${opcode[0]}: return (struct type_args){ ${opcode[1]},
> >     ${opcode[2]} };
> >     > +% endfor
> >     > +   default: return (struct type_args){ -1, -1 };
> >     > +   }
> >     > +}
> >     > +
> >     > +bool
> >     > +vtn_set_instruction_result_type(struct vtn_builder *b, SpvOp
> opcode,
> >     > +                                const uint32_t *w, unsigned count)
> >     > +{
> >     > +   struct type_args args = result_type_args_for_opcode(opcode);
> >     > +
> >     > +   if (args.res_idx < 0 || args.res_type_idx < 0)
> >     > +      return true;
> >     > +
> >     > +   struct vtn_value *val = vtn_untyped_value(b, w[1 +
> args.res_idx]);
> >     > +   val->type = vtn_value(b, w[1 + args.res_type_idx],
> >     > +                         vtn_value_type_type)->type;
> >     > +
> >     > +   return true;
> >
> >     It seems like one of the returns in this function should be false.
> The
> >     first one?
> >
> >
> > No, they should both return true.  This function uses the callback
> > interface for vtn_foreach_instruction where returning false means "stop
> > iterating".  We want to continue on even if we see an instruction which
> > has no result type.  I suppose we could at some point replace that bool
> > with an enum if that would be more clear.
>
> Ah.  For this function, if you restructure it as
>
>    if (args.res_index >= 0 && args.res_type_idx >= 0) {
>       ...
>    }
>
>    return true;
>
> the next person to look at the code won't think it's weird.


Done.


>   With that
> or a comment describing why all returns are true, this patch is
>
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
>

Thanks!


> >     > +}
> >     > +
> >     > +""")
> >     > +
> >     > +if __name__ == "__main__":
> >     > +    p = argparse.ArgumentParser()
> >     > +    p.add_argument("json")
> >     > +    p.add_argument("out")
> >     > +    args = p.parse_args()
> >     > +
> >     > +    spirv_info = json.JSONDecoder().decode(open(args.json,
> >     "r").read())
> >     > +
> >     > +    opcodes = list(find_result_types(spirv_info))
> >     > +
> >     > +    try:
> >     > +        with open(args.out, 'w') as f:
> >     > +            f.write(TEMPLATE.render(opcodes=opcodes))
> >     > +    except Exception:
> >     > +        # In the even there's an error this imports some helpers
> >     from mako
> >     > +        # to print a useful stack trace and prints it, then exits
> >     with
> >     > +        # status 1, if python is run with debug; otherwise it
> >     just raises
> >     > +        # the exception
> >     > +        if __debug__:
> >     > +            import sys
> >     > +            from mako import exceptions
> >     > +
> >     sys.stderr.write(exceptions.text_error_template().render() + '\n')
> >     > +            sys.exit(1)
> >     > +        raise
> >     > diff --git a/src/compiler/spirv/vtn_private.h
> >     b/src/compiler/spirv/vtn_private.h
> >     > index 6d4ad3c..a0a4f3a 100644
> >     > --- a/src/compiler/spirv/vtn_private.h
> >     > +++ b/src/compiler/spirv/vtn_private.h
> >     > @@ -620,6 +620,10 @@ vtn_value(struct vtn_builder *b, uint32_t
> >     value_id,
> >     >     return val;
> >     >  }
> >     >
> >     > +bool
> >     > +vtn_set_instruction_result_type(struct vtn_builder *b, SpvOp
> opcode,
> >     > +                                const uint32_t *w, unsigned
> count);
> >     > +
> >     >  struct vtn_ssa_value *vtn_ssa_value(struct vtn_builder *b,
> >     uint32_t value_id);
> >     >
> >     >  struct vtn_ssa_value *vtn_create_ssa_value(struct vtn_builder *b,
> >     >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171211/f9b81a4f/attachment-0001.html>


More information about the mesa-dev mailing list