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

[Emil Velikov]

On 28 March 2017 at 20:43, Chad Versace <chadversary at chromium.org> wrote:
> On Tue 28 Mar 2017, Emil Velikov wrote:
>> Hi Chad,
>>
>> On 25 March 2017 at 02:28, Chad Versace <chadversary at chromium.org> wrote:
>> > 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).
>> >   - 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
>> >
>> > diff --git a/src/intel/Android.genxml.mk b/src/intel/Android.genxml.mk
>> > index 79de7843801..842d0e13a33 100644
>> > --- a/src/intel/Android.genxml.mk
>> > +++ b/src/intel/Android.genxml.mk
>> > @@ -46,9 +46,16 @@ LOCAL_GENERATED_SOURCES += $(addprefix $(intermediates)/, $(GENXML_GENERATED_FIL
>> >  define header-gen
>> >         @mkdir -p $(dir $@)
>> >         @echo "Gen Header: $(PRIVATE_MODULE) <= $(notdir $(@))"
>> > -       $(hide) $(PRIVATE_SCRIPT) $(PRIVATE_XML) > $@
>> > +       $(hide) $(PRIVATE_SCRIPT) $(PRIVATE_SCRIPT_FLAGS) $(PRIVATE_XML) > $@
>> >  endef
>> >
>> > +$(intermediates)/genxml/genX_bits.h: PRIVATE_SCRIPT := $(MESA_PYTHON2) $(LOCAL_PATH)/genxml/gen_bits_header.py
>> > +$(intermediates)/genxml/genX_bits.h: PRIVATE_SCRIPT_FLAGS := --cpp-guard=GENX_BITS_H
>> > +$(intermediates)/genxml/genX_bits.h: PRIVATE_XML := $(addprefix $(LOCAL_PATH)/,$(GENXML_XML_FILES))
>> > +$(intermediates)/genxml/genX_bits.h: $(LOCAL_PATH)/genxml/gen_bits_header.py
>> > +$(intermediates)/genxml/genX_bits.h: $(addprefix $(LOCAL_PATH)/,$(GENXML_XML_FILES))
>> > +       $(call header-gen)
>> > +
>> >  $(intermediates)/genxml/gen4_pack.h: PRIVATE_SCRIPT := $(MESA_PYTHON2) $(LOCAL_PATH)/genxml/gen_pack_header.py
>> >  $(intermediates)/genxml/gen4_pack.h: PRIVATE_XML := $(LOCAL_PATH)/genxml/gen4.xml
>> >  $(intermediates)/genxml/gen4_pack.h: $(LOCAL_PATH)/genxml/gen4.xml $(LOCAL_PATH)/genxml/gen_pack_header.py
>
>
>
>> > +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.')
>
>> Admittedly --output should be enough for everyone, but I'm not sure
>> what issues you're getting with Android-IA.
>
> I didn't have any issues. I added the --cpp-guard option because it
> allowed me to re-use the same idiom in Android.genxml.mk used by the
> other generated headers. That is, the --cpp-guard option allowed me to
> re-use the Makefile's `header-gen` function instead of adding a custom
> rule just for genX_bits.h.
>
I see, quite an honourable idea.

This spares three lines (two if we ignore the @echo) in
Android.genxml.mk, by adding 6+ of python, and divergent code-flow.
The last quite fragile, IMHO.

That said, if you prefer to stick with it - go ahead.

>> For the future rather please, poke me/others for ideas/help. Be that
>> on #dri-devel, #intel-gfx or #android-ia.
>
> I don't understand. I tested the build on android-ia, and it worked. And
> the hunk in Android.genxml.mk follows the same idioms used elsewhere in
> the file.

The above should read "For the future, rather than spending too much
time on build related issues, please ...". Rather than getting annoyed
or spending lots of time fixing build issues, please shout.

Thanks
Emil