[Mesa-dev] [PATCH v2 8/14] anv: generate anv_entrypoints.{h, c} in one command

[Eric Engestrom]

On Thursday, 2017-02-23 10:46:21 -0800, Dylan Baker wrote:
> This changes the python generator to write the files itself, rather than
> piping them out. This has a couple of advantages: first, it encapsulates
> the encoding. Second, it ensures that the header file and code file are
> generated at the same time with the same data.
> 
> v2: - Update Android.mk
> 
> Signed-off-by: Dylan Baker <dylanx.c.baker at intel.com>
> ---
>  src/intel/vulkan/Android.mk             |  7 +-----
>  src/intel/vulkan/Makefile.am            |  8 ++----
>  src/intel/vulkan/anv_entrypoints_gen.py | 33 +++++++++++++-------------
>  3 files changed, 22 insertions(+), 26 deletions(-)
> 
> diff --git a/src/intel/vulkan/Android.mk b/src/intel/vulkan/Android.mk
> index 9dabf1c..df10141 100644
> --- a/src/intel/vulkan/Android.mk
> +++ b/src/intel/vulkan/Android.mk
> @@ -60,8 +60,8 @@ $(intermediates)/dummy.c:
>  	@echo "Gen Dummy: $(PRIVATE_MODULE) <= $(notdir $(@))"
>  	$(hide) touch $@
>  
> -$(intermediates)/anv_entrypoints.h:
> -	$(VK_ENTRYPOINTS_SCRIPT) header --xml $(MESA_TOP)/src/vulkan/registry/vk.xml > $@
> +$(intermediates)/anv_entrypoints.h $(intermediates)/anv_entrypoints.c:
> +	$(VK_ENTRYPOINTS_SCRIPT) --xml $(MESA_TOP)/src/vulkan/registry/vk.xml

You forgot to add the new argument here.

>  
>  LOCAL_EXPORT_C_INCLUDE_DIRS := \
>          $(intermediates)
> @@ -176,9 +176,6 @@ LOCAL_WHOLE_STATIC_LIBRARIES := libmesa_anv_entrypoints libmesa_genxml
>  
>  LOCAL_GENERATED_SOURCES += $(intermediates)/anv_entrypoints.c
>  
> -$(intermediates)/anv_entrypoints.c:
> -	$(VK_ENTRYPOINTS_SCRIPT) code --xml $(MESA_TOP)/src/vulkan/registry/vk.xml > $@
> -
>  LOCAL_SHARED_LIBRARIES := libdrm_intel
>  
>  include $(MESA_COMMON_MK)
> diff --git a/src/intel/vulkan/Makefile.am b/src/intel/vulkan/Makefile.am
> index 565559c..38a50c9 100644
> --- a/src/intel/vulkan/Makefile.am
> +++ b/src/intel/vulkan/Makefile.am
> @@ -145,11 +145,9 @@ libvulkan_intel_la_SOURCES = $(VULKAN_GEM_FILES)
>  
>  vulkan_api_xml = $(top_srcdir)/src/vulkan/registry/vk.xml
>  
> -anv_entrypoints.h : anv_entrypoints_gen.py $(vulkan_api_xml)
> -	$(AM_V_GEN)$(PYTHON2) $(srcdir)/anv_entrypoints_gen.py header --xml $(vulkan_api_xml) > $@
> -
> -anv_entrypoints.c : anv_entrypoints_gen.py $(vulkan_api_xml)
> -	$(AM_V_GEN)$(PYTHON2) $(srcdir)/anv_entrypoints_gen.py code --xml $(vulkan_api_xml) > $@
> +anv_entrypoints.h anv_entrypoints.c: anv_entrypoints_gen.py $(vulkan_api_xml)

This will run the script twice, once for anv_entrypoints.h and once for
anv_entrypoints.c, but the script will write both, both times.
This also introduces a race condition when using `make -j N`, as it will
have two threads writing to both files, potentially at the same time.

You're also hardcoding the filename in the script, which will introduce
bugs if/when files get renamed.

> +	$(AM_V_GEN)$(PYTHON2) $(srcdir)/anv_entrypoints_gen.py \
> +		--xml $(vulkan_api_xml) --outdir $(builddir)

I think passing `--out $@` in the makefile and checking the filename to
decide which file to write (and only write that one) is a better option.
See below for what that code would look like.

>  
>  BUILT_SOURCES = $(VULKAN_GENERATED_FILES)
>  CLEANFILES = $(BUILT_SOURCES) dev_icd.json intel_icd. at host_cpu@.json
> diff --git a/src/intel/vulkan/anv_entrypoints_gen.py b/src/intel/vulkan/anv_entrypoints_gen.py
> index 45a463c..5066195 100644
> --- a/src/intel/vulkan/anv_entrypoints_gen.py
> +++ b/src/intel/vulkan/anv_entrypoints_gen.py
[snip]
> @@ -370,10 +371,10 @@ def main():
>  
>      # For outputting entrypoints.h we generate a anv_EntryPoint() prototype
>      # per entry point.
> -    if args.target == 'header':
> -        print TEMPLATE_H.render(entrypoints=entrypoints)
> -    else:
> -        gen_code(entrypoints)
> +    with open(os.path.join(args.outdir, 'anv_entrypoints.h'), 'wb') as f:
> +        f.write(TEMPLATE_H.render(entrypoints=entrypoints))
> +    with open(os.path.join(args.outdir, 'anv_entrypoints.c'), 'wb') as f:
> +        f.write(gen_code(entrypoints))

This would become:
    with open(args.out, 'wb') as f:
        if args.out.endswith('.h'):
            f.write(TEMPLATE_H.render(entrypoints=entrypoints))
        else:
            f.write(gen_code(entrypoints))

>  
>  
>  if __name__ == '__main__':
> -- 
> git-series 0.9.0