[Mesa-dev] [PATCH 5/8] i965: Add script to gen code for OA counter queries

[Matt Turner]

On Wed, Mar 1, 2017 at 8:49 AM, Robert Bragg <robert at sixbynine.org> wrote:
> On Fri, Feb 24, 2017 at 2:50 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> Hi Robert,
>>
>> There's a few minor comments below. Feel free to address here or as
>> follow-up, if applicable.
>>
>> On 24 February 2017 at 13:58, Robert Bragg <robert at sixbynine.org> wrote:
>>> Avoiding lots of error prone boilerplate and easing our ability to add +
>>> maintain support for multiple OA performance counter queries for each
>>> generation:
>>>
>>> This adds a python script to generate code for building up
>>> performance_queries from the metric sets and counters described in
>>> brw_oa_hsw.xml as well as functions to normalize each counter based on
>>> the RPN expressions given.
>>>
>>> Although the XML file currently only includes a single metric set, the
>>> code generated assumes there could be many sets.
>>>
>>> The metrics as described in XML get translated into C structures
>>> which are registered in a brw->perfquery.oa_metrics_table hash table
>>> keyed by the GUID of the metric set in XML.
>>>
>> Mauro, Tapani, we have another piece which will break Androids. Feel
>> free to send a fixup for Robert to squash.
>
> Ah, can you maybe give me a bit more of a hint about what I'm breaking
> for Android? Just extra work running python scripts as part of an
> Android build I guess and requiring some change to
> ./src/mesa/drivers/dri/i965/Android.gen.mk?
>
>>
>>> Signed-off-by: Robert Bragg <robert at sixbynine.org>
>>> ---
>>>  src/mesa/drivers/dri/i965/Makefile.am      |  15 +-
>>>  src/mesa/drivers/dri/i965/Makefile.sources |   2 +
>>>  src/mesa/drivers/dri/i965/brw_oa.py        | 543 +++++++++++++++++++++++++++++
>>>  3 files changed, 559 insertions(+), 1 deletion(-)
>>>  create mode 100644 src/mesa/drivers/dri/i965/brw_oa.py
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/Makefile.am b/src/mesa/drivers/dri/i965/Makefile.am
>>> index f87fa67ef8..0130afff5f 100644
>>> --- a/src/mesa/drivers/dri/i965/Makefile.am
>>> +++ b/src/mesa/drivers/dri/i965/Makefile.am
>>> @@ -93,7 +93,9 @@ BUILT_SOURCES = $(i965_compiler_GENERATED_FILES)
>>>  CLEANFILES = $(BUILT_SOURCES)
>>>
>>>  EXTRA_DIST = \
>>> -       brw_nir_trig_workarounds.py
>>> +       brw_nir_trig_workarounds.py \
>>> +       brw_oa_hsw.xml \
>>> +       brw_oa.py
>>>
>>>  TEST_LIBS = \
>>>         libi965_compiler.la \
>>> @@ -169,3 +171,14 @@ test_eu_validate_SOURCES = \
>>>  test_eu_validate_LDADD = \
>>>         $(top_builddir)/src/gtest/libgtest.la \
>>>         $(TEST_LIBS)
>>> +
>>> +BUILT_SOURCES = \
>>> +       brw_oa_hsw.h \
>>> +       brw_oa_hsw.c
>>> +
>>> +brw_oa_hsw.h brw_oa_hsw.c: brw_oa_hsw.xml brw_oa.py Makefile
>> Eric reminded me that this is a bit buggy/racy. Something like the
>> following should be better:
>>
>> brw_oa_hsw.h: brw_oa_hsw.c
>> brw_oa_hsw.c: brw_oa_hsw.xml brw_oa.py
>
> as one issue, I can see that brw_oa_hsw.h isn't declared as a
> prerequisite for compiling brw_oa_hsw.c, am I missing more issues?
> Based on that I'd imagine adding:
>
> brw_oa_hsw.c: brw_oa_hsw.h
>
> as a way to ensure we can't attempt to compile brw_oa_hsw.c before
> brw_oa_hsw.h is generated.
> Is there a reason for removing the Makefile prerequisite? If I were to
> mess with the arguments passed to the script in the codegen rules I'd
> like a Makefile change to result in re-running those rules.

We depend on the Makefile in src/intel/Makefile.genxml.am, and one
unintended side-effect of that is that after every ./configure the
files that depend on Makefile have to be recompiled.

In that case, contents of the Makefile end up in the resulting files,
so I think the dependency is okay if a bit annoying. This patch,
however, I'd remove the Makefile dependency, since the Makefile just
contains the execution of the script. I'm not sure why the script
arguments would change...