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

[Robert Bragg]

On Wed, Mar 1, 2017 at 4:56 PM, Matt Turner <mattst88 at gmail.com> wrote:
> 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...

Ah ok, it should at least be changed to depend on Makefile.am to avoid
redundant codegen on each configure, yeah.

It's a very minor corner case where, during development, I/someone
might conceivably change how a script is called in one of the
Makefile.am rules without there having been a change to the script
itself - maybe just as trivial as fixing a typo or copy and paste
mistake. In practice its also easy to just nuke the generated files
manually in those cases, but if a dep on Makefile.am doesn't do any
harm and doesn't have the issue described above, maybe it's ok having
the dependency explicitly specified too? (changing to Makefile.am
locally seems to do the right thing without redundant work after a
configure)

I'll guess for now it's ok go with a Makefile.am dep - but fine to
drop if there are still concerns with that.

Thanks,
- Robert