Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CR-1170195: Fixing issue with seg fault in AIE status/profiling/trace #7654

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

jvillarre
Copy link
Collaborator

Problem solved by the commit

Processing the system_metadata section in profiling was crashing on designs that had PL kernels and AIE kernels. This was because newly introduced code was based on the faulty assumption that only PL kernels and compute units were listed in the specific system_metadata sections that PL profiling was dependent on. On designs that have AIE kernels in addition to PL kernels, we were going outside the bounds of our internal array.

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

This bug was introduced in pull request 7640 and it was discovered with testing on AIE designs.

How problem was solved, alternative solutions (if any) and why they were rejected

This pull request no longer accesses the problematic array, but instead creates a local map of compute unit ID to compute unit name based solely on the information in the system_metadata section.

Risks (if any) associated the changes in the commit

Low risk as this fixes the seg fault and still maintains the functionality, but is dependent on the system metadata section format.

What has been tested and how, request additional testing if necessary

This has been tested on a mixed AIE + PL design on vek280 and a PL only design on the u200.

Documentation impact (if any)

No documentation impact.

Signed-off-by: Jason Villarreal <jvillar@xilinx.com>
@gbuildx
Copy link
Collaborator

gbuildx commented Aug 1, 2023

Build Passed!

Copy link
Collaborator

@IshitaGhosh IshitaGhosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good. However, we may need to look deeper for cases where "compute_unit" metadata has argument name but no id name. This can be a corner case, if possible. As we have no knowledge about the schema of "compute_units" metadata we can ignore possibility of such corner cases and submit this change. If such scenario is found, we can handle it later.

@jvillarre jvillarre merged commit d0795ad into Xilinx:master Aug 1, 2023
2 checks passed
@jvillarre jvillarre deleted the CR1170195 branch December 4, 2023 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants