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

[mono] Fix missing simd mul/div case #81201

Merged
merged 3 commits into from
Jan 26, 2023
Merged

[mono] Fix missing simd mul/div case #81201

merged 3 commits into from
Jan 26, 2023

Conversation

lewing
Copy link
Member

@lewing lewing commented Jan 26, 2023 •
edited
Loading

Without this only the scalar cases emit anything for Vector128 which is key to emit_vector64_vector128_t working properly. This code was removed in the Vector512 changes probably because it needs additional checks on the types.

@lewing lewing added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it area-Codegen-AOT-mono labels Jan 26, 2023
@ghost ghost assigned lewing Jan 26, 2023
@lewing lewing requested a review from fanyang-mono January 26, 2023 03:34
@lewing lewing requested a review from radekdoulik January 26, 2023 03:38
@lewing
Copy link
Member Author

lewing commented Jan 26, 2023

https://gist.github.com/radekdoulik/7d27da1693192e16b621f7cd746f2dec has some diffs of good vs bad generated code

@radekdoulik radekdoulik mentioned this pull request Jan 26, 2023
Merged
radekdoulik
radekdoulik reviewed Jan 26, 2023
View reviewed changes
Copy link
Member

@radekdoulik radekdoulik left a comment

Choose a reason for hiding this comment

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

LGTM, it might have been disabled by mistake? I am asking in #76642 (review) to find out if there was a reason to disable it.

fanyang-mono
fanyang-mono approved these changes Jan 26, 2023
View reviewed changes
Copy link
Member

@fanyang-mono fanyang-mono left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for fixing this. They were accidentally removed somehow. Seems to be a merge mistake.

@lewing lewing removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Jan 26, 2023
@lewing lewing marked this pull request as ready for review January 26, 2023 15:34
@lewing lewing changed the title [mono] Test missing simd mul/div case [mono] Fix missing simd mul/div case Jan 26, 2023
@lambdageek
Copy link
Member

lambdageek commented Jan 26, 2023 •
edited
Loading

It looks like it was a bad merge into #76642. The correct code is in 8158cb8 (from #78784) but then when the PR was merged an incomplete version from #76642was picked instead during the merge (67abfd0)

@lambdageek
Copy link
Member

I verified this PR restores most of the performance in the threaded raytracer compared to net7

@lewing lewing merged commit 1429bdf into dotnet:main Jan 26, 2023
@lewing lewing deleted the test-theory branch January 26, 2023 15:43
This was referenced Jan 28, 2023
Merged
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Feb 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@radekdoulik radekdoulik radekdoulik left review comments

@lambdageek lambdageek lambdageek approved these changes

@fanyang-mono fanyang-mono fanyang-mono approved these changes

@vargaz vargaz Awaiting requested review from vargaz

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

Assignees

@lewing lewing

Labels
area-Codegen-AOT-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lewing @lambdageek @radekdoulik @fanyang-mono