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

[Robert Bragg]

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.

I just noticed that I should also be using += for BUILT_SOURCES here, oops.

>  ...
>
>> +       $(PYTHON2) $(PYTHON_FLAGS) $(srcdir)/brw_oa.py \
>> +          --header=$(builddir)/brw_oa_hsw.h \
>> +          --code=$(builddir)/brw_oa_hsw.c \
>> +          --chipset="hsw" \
>> +          $(srcdir)/brw_oa_hsw.xml
>> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources b/src/mesa/drivers/dri/i965/Makefile.sources
>> index 5278e86339..60acd15d41 100644
>> --- a/src/mesa/drivers/dri/i965/Makefile.sources
>> +++ b/src/mesa/drivers/dri/i965/Makefile.sources
>> @@ -135,6 +135,8 @@ i965_FILES = \
>>         brw_nir_uniforms.cpp \
>>         brw_object_purgeable.c \
>>         brw_pipe_control.c \
>> +       brw_oa_hsw.h \
>> +       brw_oa_hsw.c \
> H is after C ;-)
>
>>         brw_performance_query.h \
>>         brw_performance_query.c \
>>         brw_program.c \
>> diff --git a/src/mesa/drivers/dri/i965/brw_oa.py b/src/mesa/drivers/dri/i965/brw_oa.py
>
>> new file mode 100644
>> index 0000000000..2c622531af
>> --- /dev/null
>> +++ b/src/mesa/drivers/dri/i965/brw_oa.py
>> @@ -0,0 +1,543 @@
>> +#!/usr/bin/env python2
> Thanks for omitting the execute bit !
> Maybe drop this, since one should/can not execute the file directly ?

yeah, can do. I suppose it could be considered a hint that it requires
python2, but just checking whether the script really depends on
python2 if I avoid xrange then the script actually runs fine with
either version.

>
>> +
>> +if args.header:
>> +    header_file = open(args.header, 'w')
>> +
>
>> +
>> +h(copyright)
>> +h("""#pragma once
> Use proper ifndef guards please.

Oh, I really like #pragma once though :-)

It looks like we already assume #pragma once is supported by all the
compilers we care about. git grep '#pragma once'|wc -l shows 107
existing uses with 9 of those under /dri/i965/

Is it ok to keep considering that it seems to already be in quite wide
use within mesa?


Thanks for the comments,
- Robert

>
> Thanks
> Emil