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-1159280: Fixing profiling table to correctly associate argument with connection #7640

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

jvillarre
Copy link
Collaborator

Problem solved by the commit

Profiling tables that report data movement between kernels and memory report the original kernel arguments associated with the connection. With connections to host memory via host bridge, this association was not correctly made and the tables had empty entries. Additionally, if multiple kernels were present in a design and had arguments with the same name, they were incorrectly being reported as attached to the wrong memory component.

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

The issue was discovered through comprehensive regression testing.

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

This change explicitly adds a check for host bridge connections. Additionally, it changes the argument to memory mapping data structure to make the connection on a compute unit basis rather than a kernel basis, so multiple kernels with identical named arguments don't overwrite each other.

Risks (if any) associated the changes in the commit

Low risk as this should only affect information reported in a single table.

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

Tested on original failing test case.

Documentation impact (if any)

No documentation impact

…ute unit based. Also adding support for host memory.

Signed-off-by: Jason Villarreal <[email protected]>
@gbuildx
Copy link
Collaborator

gbuildx commented Jul 25, 2023

Build failed :(

@dayeh-xilinx
Copy link

retest this please.

@gbuildx
Copy link
Collaborator

gbuildx commented Jul 26, 2023

Build failed :(

@manikandan-xilinx
Copy link
Collaborator

retest this please.

@gbuildx
Copy link
Collaborator

gbuildx commented Jul 26, 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

@jvillarre jvillarre merged commit 66f6b88 into Xilinx:master Jul 26, 2023
2 checks passed
@jvillarre jvillarre deleted the CR1159280 branch December 4, 2023 19:52
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.

5 participants