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.

Sign up for GitHub

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

Jump to bottom

Update ELT profiler tests to include tests when a function returns HFA/HVA struct on Arm64 #54912

Merged
merged 6 commits into from
Sep 29, 2021
Merged

Update ELT profiler tests to include tests when a function returns HFA/HVA struct on Arm64 #54912

merged 6 commits into from
Sep 29, 2021

Conversation

echesakov
Copy link
Contributor

@echesakov echesakov commented Jun 29, 2021 •
edited
Loading

  • Add various ELT profiler tests for HFA/HVA arguments/return values.
  • Fix an issue with ProfileArgIterator::GetNextArgAddr and ProfileArgIterator::GetReturnBufferAddr not properly handling HFA/HVAs on Arm64.
  • Fix an issue with ProfileArgIterator::GetReturnBufferAddr not properly handling struct returned in registers on Linux x64 and macOS x64.
  • Add tests to cover different ways of returning a struct on System V .
  • Fix an issue with not passing an actual value of rdx down to a callback in ProfileLeaveNaked and ProfileTailcallNaked helpers.

@echesakov echesakov self-assigned this Jun 29, 2021
@ghost
Copy link

ghost commented Jun 29, 2021

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details
Author: echesakovMSFT
Assignees: echesakovMSFT
Labels:

arch-arm64, area-Diagnostics-coreclr
Milestone: -

@tommcdon
Copy link
Member

@davmason

@echesakov echesakov force-pushed the Leave-Profiler-Hook-HFA-HVA-Return-Values branch from 3ef78f5 to a32f5b2 Compare June 30, 2021 02:31
@davmason
Copy link
Member

We talked about this on teams but I realized I never commented here. We need to check if we hit the same condition with function args but otherwise LGTM

@echesakov
Copy link
Contributor Author

echesakov commented Jul 19, 2021 •
edited
Loading

We talked about this on teams but I realized I never commented here. We need to check if we hit the same condition with function args but otherwise LGTM

Yes, I will do this today - haven't done this yet as I was sidetracked by other work recently.

@echesakov echesakov force-pushed the Leave-Profiler-Hook-HFA-HVA-Return-Values branch 5 times, most recently from 66e785c to d8e0b35 Compare July 23, 2021 00:12
davmason
davmason approved these changes Jul 23, 2021
View reviewed changes
Copy link
Member

@davmason davmason left a comment

Choose a reason for hiding this comment

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

Thanks Egor! I am not an expert in the arm64 ABI but from what I can see it all looks good

@echesakov echesakov marked this pull request as ready for review July 23, 2021 16:10
@echesakov
Copy link
Contributor Author

@davmason Found and fixed another issue with System V - we didn't have an actual value of PROFILE_PLATFORM_SPECIFIC_DATA.rdx since the assembly helpers we not passing it to ProfileLeave, ProfileTail callbacks.

@davmason
Copy link
Member

I just took a look at the new changes and they all look good to me

@echesakov
Copy link
Contributor Author

@davmason Apparently, the vararg test I added failed on win-x86 yesterday that indicates that we have an issue. For the purposes of this PR, I will disable the test on that platform and open a GitHub issue.

@echesakov echesakov added this to the 7.0.0 milestone Aug 10, 2021
@jaredpar jaredpar mentioned this pull request Aug 17, 2021
Closed
@tommcdon
Copy link
Member

tommcdon commented Sep 7, 2021

@echesakovMSFT do you know if any other work is needed before this PR is merged?

@tommcdon
Copy link
Member

@davmason @echesakovMSFT is this PR ready to be merged?

@davmason
Copy link
Member

Egor is on vacation for the month of September, my understanding is that the tests have issues but the product changes should be OK to merge. Can we wait for him to get back, or do we need to split it up and merge just the product changes now?

@tommcdon
Copy link
Member

Thanks David - I wasn't aware that Egor was on vacation. I'm not aware of a reason to merge immediately, so we can wait until Egor returns.

@echesakov
Copy link
Contributor Author

echesakov commented Sep 27, 2021 •
edited
Loading

@echesakovMSFT do you know if any other work is needed before this PR is merged?

@tommcdon I added some cases (Int32x4VarArgStructFunc) that causes failures (most of the failures due to varargs not supported on non-Windows platforms) - I will need to remove the test cases before this PR can be merged. Also I will open a GitHub issue describing what other problems I found. Then we can decide whether they need to be addressed.

@echesakov
Don't store zero in PROFILE_PLATFORM_SPECIFIC_DATA.rdx in ProfileLeav…
ac86d03 
…eNaked and ProfileTailcallNaked helpers - fill it with an actual value instead.
@echesakov echesakov force-pushed the Leave-Profiler-Hook-HFA-HVA-Return-Values branch from 641bf62 to ac86d03 Compare September 29, 2021 18:19
@echesakov
Copy link
Contributor Author

Rebased the PR and also removed the failing varargs test cases.

@echesakov echesakov merged commit cd1cd1d into dotnet:main Sep 29, 2021
@echesakov echesakov deleted the Leave-Profiler-Hook-HFA-HVA-Return-Values branch September 29, 2021 21:35
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davmason davmason davmason approved these changes

Assignees

@echesakov echesakov

Labels
arch-arm64 area-Diagnostics-coreclr
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@echesakov @tommcdon @davmason