[Mesa-dev] [PATCH v5 1/5] genxml: New generated header genX_bits.h (v5)

[Chad Versace]

On Sat 25 Mar 2017, Dylan Baker wrote:
> Quoting Chad Versace (2017-03-24 19:28:23)
> > genX_bits.h contains the sizes of bitfields in genxml instructions,
> > structures, and registers. It also defines some functions to query those
> > sizes.
> > 
> > isl_surf_init() will use the new header to validate that requested
> > pitches fit in their destination bitfields.
> > 
> > What's currently in genX_bits.h:
> > 
> >   - Each CONTAINER::Field from gen*.xml that has a bitsize has a macro
> >     in genX_bits.h:
> > 
> >         #define GEN{N}_CONTAINER_Field_bits {bitsize}
> > 
> >   - For each set of macros whose name, after stripping the GEN prefix,
> >     is the same, genX_bits.h contains a query function:
> > 
> >       static inline uint32_t __attribute__((pure))
> >       CONTAINER_Field_bits(const struct gen_device_info *devinfo);
> > 
> > v2 (Chad Versace):
> >   - Parse the XML instead of scraping the generated gen*_pack.h headers.
> > 
> > v3 (Dylan Baker):
> >   - Port to Mako.
> > 
> > v4 (Jason Ekstrand):
> >   - Make the _bits functions take a gen_device_info.
> > 
> > v5 (Chad Versace):
> >   - Fix autotools out-of-tree build.
> >   - Fix Android build. Tested with git://github.com/android-ia/manifest.
> >   - Fix macro names. They were all missing the "_bits" suffix.
> >   - Fix macros names more. Remove all double-underscores.
> >   - Unindent all generated code. (It was floating in a sea of whitespace).
> 
> I really think this is a bad design decision, and feels rather like the tail
> wagging the dog. You're sacrificing the readability and editability of the code
> that humans are actually supposed to edit, to gain something that the compiler
> doesn't care about (the most common path), and the indent command can solve in
> the cases where one wants to carefully read the generated code.

I would normally agree with you here, on the topic of generated code,
but this time...

I think you have the head and tail confused :)

Then generated gen*_pack.h headers, I read and search them very
frequently. vim+ctags is always bringing me there. As I'm coding,
they're my first go-to reference for the hardware.  But the generator
script? I've never opened it, not once, until I wrote this patch series.

> >   - Reformat header to appear human-written not machine-generated.
> >   - Sort gens from high to low. Newest gens should come first because,
> >     when we read code, we likely want to read the gen8/9 code and ignore
> >     the gen4 code. So put the gen4 code at the bottom.
> >   - Replace 'const' attributes with 'pure', because the functions now
> >     have a pointer parameter.
> >   - Add --cpp-guard flag. Used by Android.
> >   - Kill class FieldCollection. After Jason's rewrite, it was just
> >     a dict.
> > 
> > Co-authored-by: Dylan Baker <dylan at pnwbakers.com>
> > Co-authored-by: Jason Ekstrand <jason at jlekstrand.net>
> > ---
> >  src/intel/Android.genxml.mk         |   9 +-
> >  src/intel/Makefile.genxml.am        |   6 +-
> >  src/intel/Makefile.sources          |   6 +-
> >  src/intel/genxml/.gitignore         |   1 +
> >  src/intel/genxml/gen_bits_header.py | 281 ++++++++++++++++++++++++++++++++++++
> >  5 files changed, 300 insertions(+), 3 deletions(-)
> >  create mode 100644 src/intel/genxml/gen_bits_header.py


> > +    def end_element(self, name):
> > +        if name == 'genxml':
> > +            self.gen = None
> > +        elif name in ('instruction', 'struct', 'register'):
> > +            self.container_name = None
> > +        else:
> > +            pass
> 
> else: pass is a bit of a pointless statement

I wanted the code here to explicitly emphasize "we're not parsing those
other elements". I could've added a comment, but the else/pass does
a better job at the emphasis.

> > +        key = (self.container_name, field_name)
> > +        if key not in self.fields.keys():
> 
> key key not in self.feilds
> 
> Which does the same thing, but doesn't build an intermediate list to check.

Nice. I'll fix that.

> > +            self.fields[key] = Field(self.container_name, field_name)
> > +        self.fields[key].add_gen(self.gen, attrs)
> > +
> > +def parse_args():
> > +    p = argparse.ArgumentParser()
> > +    p.add_argument('-o', '--output', type=str,
> > +                   help="If OUTPUT is unset or '-', then it defaults to '/dev/stdout'")
> > +    p.add_argument('--cpp-guard', type=str,
> > +                   help='If unset, then CPP_GUARD is derived from OUTPUT.')
> > +    p.add_argument('xml_sources', metavar='XML_SOURCE', nargs='+')
> > +
> > +    pargs = p.parse_args()
> > +
> > +    if pargs.output in (None, '-'):
> > +        pargs.output = '/dev/stdout'
> 
> This is just a suggestion, feel free to ignore
> 
> I forgot to do something here, but this is one case where using
> argparse.FileType might actually be the right thing to do.
> 
> it would like like:
> p.add_argument('-o', '--output', type=argparse.FileType('wb'), default=sys.stdout, ...)
> 
> Then you could drop the hunk above and the 'with open(...)' in the main
> function. It's also more portable.

I just tried the suggestion, and it behaves badly IMO for output files.  It's
probably fine for input files. The problem is that ArgumentParser creates the
file as soon as it parses the -o option, even when there is a usage error or
the --help option is given.

Example 1:
  $ ./gen_bits_header.py -o bad-output1 --bad-option
  usage: gen_bits_header.py [-h] [-o OUTPUT] [--cpp-guard CPP_GUARD]
                          XML_SOURCE [XML_SOURCE ...]
  gen_bits_header.py: error: too few arguments
  $ echo $?
  2
  $ ls
  ... bad-output1

Example 2:
  $ ./gen_bits_header.py -o bad-output2 --help
  usage: gen_bits_header.py [-h] [-o OUTPUT] [--cpp-guard CPP_GUARD]
                          XML_SOURCE [XML_SOURCE ...]
  $ echo $?
  0
  $ ls
  ... bad-output2