Skip to content

rocr/aie: Handle non-HSA_STATUS_SUCCESS during VisitRegion#294

Closed
ypapadop-amd wants to merge 2 commits intoROCm:amd-stagingfrom
ypapadop-amd:aie-agent-handle-visit-region-status
Closed

rocr/aie: Handle non-HSA_STATUS_SUCCESS during VisitRegion#294
ypapadop-amd wants to merge 2 commits intoROCm:amd-stagingfrom
ypapadop-amd:aie-agent-handle-visit-region-status

Conversation

@ypapadop-amd
Copy link
Contributor

This PR address a bug that hides the result of a visit during AieAgent::VisitRegion.

Copy link
Contributor

@atgutier atgutier left a comment

Choose a reason for hiding this comment

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

Good catch. One minor nit: I know I was using status, but I noticed that err seems to be common elsewhere. Can you rename the status var to err here? Thanks!

@ypapadop-amd
Copy link
Contributor Author

ypapadop-amd commented Feb 17, 2025

Good catch. One minor nit: I know I was using status, but I noticed that err seems to be common elsewhere. Can you rename the status var to err here? Thanks!

You mean stat? I don't see err in AieAgent; I picked it up from

hsa_status_t status = call(region_handle, data);

@atgutier
Copy link
Contributor

Good catch. One minor nit: I know I was using status, but I noticed that err seems to be common elsewhere. Can you rename the status var to err here? Thanks!

You mean stat? I don't see err in AieAgent; I picked it up from

hsa_status_t status = call(region_handle, data);

No, I just mean generally throughout the runtime they use err for hsa_status_t codes.

@ypapadop-amd
Copy link
Contributor Author

Good catch. One minor nit: I know I was using status, but I noticed that err seems to be common elsewhere. Can you rename the status var to err here? Thanks!

You mean stat? I don't see err in AieAgent; I picked it up from

hsa_status_t status = call(region_handle, data);

No, I just mean generally throughout the runtime they use err for hsa_status_t codes.

OK. Just this instance or do you mean the whole file? BTW, it can also be HSA_STATUS_INFO_BREAK, which is not error, just an early exit (i.e., you found what you were looking for and you don't want the visitor to continue).

@atgutier
Copy link
Contributor

Good catch. One minor nit: I know I was using status, but I noticed that err seems to be common elsewhere. Can you rename the status var to err here? Thanks!

You mean stat? I don't see err in AieAgent; I picked it up from

hsa_status_t status = call(region_handle, data);

No, I just mean generally throughout the runtime they use err for hsa_status_t codes.

OK. Just this instance or do you mean the whole file? BTW, it can also be HSA_STATUS_INFO_BREAK, which is not error, just an early exit (i.e., you found what you were looking for and you don't want the visitor to continue).

Up to you if you want to change the whole file. Would probably be for the best as I prefer we remain consistent with existing style when possible.

Copy link
Contributor

@atgutier atgutier left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ypapadop-amd
Copy link
Contributor Author

@dayatsin-amd any comments?

Copy link
Collaborator

@dayatsin-amd dayatsin-amd left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you

@ammallya
Copy link
Contributor

!verify

@ypapadop-amd
Copy link
Contributor Author

I think that this can be closed now.

@ypapadop-amd ypapadop-amd deleted the aie-agent-handle-visit-region-status branch April 1, 2025 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants