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

Remove assert in sse_round_fn and handle case where src2 is in memory #3465

Merged

Conversation

sydhds
Copy link

@sydhds sydhds commented Jan 10, 2023

In Singlepass SSE4.2 backend and for sse_round_fn function, some special cases were not handled:

  • src1 != src2
  • src2 is in memory

This should fix those cases.

This is a follow up of ticket #3461

Copy link
Contributor

@ptitSeb ptitSeb left a comment

Choose a reason for hiding this comment

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

Please create a if / else branch

Copy link
Contributor

@ptitSeb ptitSeb left a comment

Choose a reason for hiding this comment

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

So, I reviewed the ROUNDSS/VROUNDSS opcode (for example, it's one of the opcode use here): https://www.felixcloutier.com/x86/roundss
And it's not what I imagined.
So the code should be:

if x != dst {
 move_src_to_dst(emitter, precision, src1, dst);
}
dynasm!(emitter ; $ins Rx((x as u8)), Rx((dst as u8)), $mode)

Basicaly a bigger change, but that version should be exact. It's not important for now, as ROUND functions are basically used as 2 operators opcode, never as 3-way in Singlepass.

Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

It should already be approved. But approving again to see if that trigger tests

@sydhds sydhds force-pushed the singlepass_fix_sse42_backend_sse_round branch 2 times, most recently from eaab3cc to 675cf17 Compare January 12, 2023 07:55
@sydhds sydhds force-pushed the singlepass_fix_sse42_backend_sse_round branch from 675cf17 to bfa1b67 Compare January 12, 2023 07:57
@theduke theduke enabled auto-merge January 16, 2023 17:03
@syrusakbary syrusakbary disabled auto-merge January 19, 2023 22:07
@syrusakbary syrusakbary merged commit 580baeb into wasmerio:master Jan 19, 2023
@syrusakbary
Copy link
Member

syrusakbary commented Jan 19, 2023

Merged manually, as I had no idea why tests are not being executed (we will revert in case tests fail)

@theduke
Copy link
Contributor

theduke commented Jan 19, 2023

@syrusakbary Github Actions have been problematic the last two days, also on other repos.
Timeouts, network errors, jobs just sitting idle forever, ...

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.

4 participants