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

[Ppc64le] binaryformatter and tensors tests fix #108120

Conversation

alhad-deshpande
Copy link
Contributor

@alhad-deshpande alhad-deshpande commented Sep 23, 2024

Fixes #97296. This is a backport PR for release/9.0 branch.

Customer Impact

  • Customer reported
  • Found internally

The IBM team found issues related to float 32 bit load/store operations targeting their ppc64le architecture. This change alters the runtime to generate correct assembler code whenever float32 is used in C# applications.

Regression

  • Yes
  • No

[If yes, specify when the regression was introduced. Provide the PR or commit if known.]

Testing

Manual testing

Risk

Low - this change does not target anything we directly ship

alhad-deshpande and others added 29 commits September 8, 2022 10:55
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 23, 2024
Copy link
Contributor

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

@alhad-deshpande
Copy link
Contributor Author

@akoeplinger
Created this new PR for backporting changes of PR #107678 as directed by @leecow. This is a required PR to get merged in .NET9 release branch as changes are critical related to float32 load/store operations.. Added business case above. Can you please merge this PR?

Cc: @uweigand, @janani66

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

lgtm. we will take for consideration in 9 GA

@steveisok steveisok self-requested a review October 8, 2024 17:27
@steveisok steveisok added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Oct 9, 2024
@jeffschwMSFT
Copy link
Member

@steveisok can you take a look at the PR failures?

@steveisok
Copy link
Member

@steveisok can you take a look at the PR failures?

The builds no longer exist, so I'll have to kick it again.

@alhad-deshpande
Copy link
Contributor Author

Hi @jeffschwMSFT @steveisok @lambdageek @akoeplinger

Added changes from PR #108429 which fixes assertion failure which was occurring while running System.Runtime.Serialization.Formatters.Tests tests. This was a very small change in description file. Hence thought to add in this PR instead of opening a new PR against .NET 9 release branch.

Can you please merge this PR now?

Cc: @leecow @uweigand @janani66

@steveisok
Copy link
Member

Can you please merge this PR now?

Yeah, we'll merge after CI completes. I have no problem with the additional commit as it's ppc specific.

@alhad-deshpande
Copy link
Contributor Author

Can you please merge this PR now?

Yeah, we'll merge after CI completes. I have no problem with the additional commit as it's ppc specific.

Thanks @steveisok.

@jeffschwMSFT
Copy link
Member

@alhad-deshpande in the future, once things are approved any additional changes would cause us to go back through the approval process. in this case, it is OK just to have a new code review from @steveisok .

@jeffschwMSFT jeffschwMSFT merged commit 6193a1f into dotnet:release/9.0 Oct 10, 2024
10 of 16 checks passed
@alhad-deshpande
Copy link
Contributor Author

@jeffschwMSFT
Thanks for considering a new change this time with a single review from @steveisok .

@github-actions github-actions bot locked and limited conversation to collaborators Nov 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-ppc64le area-Codegen-JIT-mono community-contribution Indicates that the PR has been added by a community member Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants