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

Legalize fmin/fmax with NaN quieting semantics #1905

Merged
merged 2 commits into from
Jun 25, 2020

Conversation

abrown
Copy link
Contributor

@abrown abrown commented Jun 19, 2020

This is identical to #1821 but without the guarantee flags.

@abrown abrown requested a review from bnjbvr June 19, 2020 18:52
@abrown abrown force-pushed the legalize-fmin-fmax-slow branch from 6e1f8c3 to 3dda5bc Compare June 19, 2020 19:09
@abrown abrown force-pushed the legalize-fmin-fmax-slow branch from 3dda5bc to 7322f2e Compare June 19, 2020 19:16
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:meta Everything related to the meta-language. labels Jun 19, 2020
@github-actions
Copy link

Subscribe to Label Action

cc @bnjbvr

This issue or pull request has been labeled: "cranelift", "cranelift:meta"

Thus the following users have been cc'd because of the following labels:

  • bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

I won't lie and claim that I understand it all, but it kind of makes sense and if it passes more SIMD tests, that's good enough for me. Approved on the assumption that the loss of sign isn't a problem according to the spec. Thanks!

) {
let ty = func.dfg.ctrl_typevar(inst);
debug_assert!(ty.is_vector());
let (x, y, x86_opcode, is_max) = match func.dfg[inst] {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can we get rid of is_max by just looking at the opcode, instead of the one use of is_max?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could but then it would be let (x, y, x86_opcode, clif_opcode) and we would have to do the opcode comparison twice (once in the match and below in the current if using is_max).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to merge this as-is but I can make a subsequent change to let (x, y, x86_opcode, clif_opcode) if you prefer that.

cranelift/codegen/src/isa/x86/enc_tables.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/x86/enc_tables.rs Outdated Show resolved Hide resolved
@alexcrichton alexcrichton changed the base branch from master to main June 25, 2020 18:48
@abrown abrown force-pushed the legalize-fmin-fmax-slow branch from 7322f2e to 2450111 Compare June 25, 2020 20:47
@abrown abrown merged commit ae63441 into bytecodealliance:main Jun 25, 2020
@abrown abrown deleted the legalize-fmin-fmax-slow branch June 25, 2020 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants