Use tablegen Python bindings for trace events instead of separate generation#3038
Conversation
…se pattern; remove generate_events_enum.py and use events generated through tablegen.
yenjames
requested review from
abisca,
andrej,
erwei-xilinx,
fifield,
hunhoffe,
jackl-xilinx,
jgmelber,
pvasireddy-amd and
stephenneuendorffer
as code owners
There was a problem hiding this comment.
Pull request overview
Removes the redundant Python trace-event enum generation pipeline and switches Python trace utilities to consume TableGen-generated enum bindings, while checking in a unified JSON event database for C++ lookups.
Changes:
- Switch
aie.utils.trace.eventsto import architecture event enums fromaie.dialects._aie_enum_genand provideget_events_for_device()viaSimpleNamespace. - Remove Python CMake targets/steps that generated and installed per-arch Python enum modules and the events JSON.
- Add
events_database.jsonto the regdb resources and provide an offline generatorutils/generate_events_json.py.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| utils/generate_events_json.py | Drops Python enum generation and emits JSON-only event database from aie-rt headers. |
| python/utils/trace/events/init.py | Uses TableGen Python enums and updates device→arch selection + GenericEvent int/enum handling. |
| python/CMakeLists.txt | Removes build/install steps for generated Python trace-event enums/JSON. |
| lib/Dialect/AIE/Util/events_database.json | Checks in the unified event database JSON used by the C++ register database. |
| lib/Dialect/AIE/Util/CMakeLists.txt | Copies/installs events_database.json as part of regdb resources. |
| test/python/trace_events.py | Updates tests to use get_events_for_device() instead of importing per-arch enum modules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hunhoffe
left a comment
hunhoffe
left a comment
There was a problem hiding this comment.
Looks like a clean improvement! I had some questions, but overall, I like it!
|
Does this PR address this issue? #3012 |
Yes, just linked it to that issue. |
|
|
||
| from aie.dialects.aie import WireBundle, DMAChannelDir | ||
|
|
||
| # Default to AIE2 for backwards compatibility |
There was a problem hiding this comment.
I'm a little confused abut why we set defaults when there's a function below that does more correct conditional sets? Can you clarify a bit?
There was a problem hiding this comment.
Yes this is a part where I don't have a clear solution. The issue is the tablegen events suffixes the event names with the architecture (CoreEventAIE2.), but I still wanted to keep the aliases CoreEvent etc to paper over this name. The backwards compatibility comment is not very correct.
The get_events_for_device is more correct but it will require changing all examples to first acquire events with it.
| ): | ||
| # For backwards compatibility, allow integer as event | ||
| if isinstance(code, int): | ||
| # For backwards compatibility, allow plain integer as event. |
There was a problem hiding this comment.
Honestly curious: do we need backwards compatibility here?
There was a problem hiding this comment.
I think not really needed, we should use events by the enum type and error for other types.
| def main(): | ||
| argparser = argparse.ArgumentParser( | ||
| description="Generate Python enums and JSON database from AIE event headers" | ||
| description="Generate JSON event database from AIE event headers" |
There was a problem hiding this comment.
Just to double check -- the JSON file is checked in in GitHub on this PR. In the past, I thought this was generated at compile time, and not checked in. I don't feel strongly about which one is best, but just to check -- was that purposeful?
There was a problem hiding this comment.
As pointed out by Jeff, this was a mistake on my end.
fifield
left a comment
fifield
left a comment
There was a problem hiding this comment.
This PR goes farther than removing the duplicated event enums by also removing the generated events_database.json. Generating the events_database.json is intended and not duplication. Can we restore that part, or fold the generation of the json into the cmake rules for generating the tablegen files (if reducing lines of cmake is also a goal of the PR)?
Reverted, this was overdone and a mistake on my end. Thank you for catching this! |
Summary
Trace event enums were duplicated across two generation pipelines that both parsed the same aie-rt C headers. We remove the redundant Python enum generation.
Changes
python/utils/trace/events/__init__.py: Import event enums from aie.dialects._aie_enum_gen instead of generated per-arch modules. Use SimpleNamespace for get_events_for_device(). Fix GenericEvent.init to check Enum before int since IntEnum is a subclass of both.python/CMakeLists.txt: Remove Python enum generation, GenerateTraceEventsEnum target, and associated copy/install steps.include/aie/Dialect/AIE/IR/CMakeLists.txt: Foldevents_database.jsongeneration into the existingGenerateAIEEventsTDtarget alongside the tablegen files, sharing the same aie-rt header inputs.utils/generate_events_json.py: Renamed fromgenerate_events_enum.py, stripped of Python enum generation. Now JSON-only, invoked at build time byGenerateAIEEventsTD.test/python/trace_events.py: Updated to assertMemTileEvent is Nonefor AIE1 instead of iterating an empty enum.