Skip to content

Legalize fmin/fmax with NaN quieting semantics#1905

Merged
abrown merged 2 commits into
bytecodealliance:mainfrom
abrown:legalize-fmin-fmax-slow
Jun 25, 2020
Merged

Legalize fmin/fmax with NaN quieting semantics#1905
abrown merged 2 commits into
bytecodealliance:mainfrom
abrown:legalize-fmin-fmax-slow

Conversation

@abrown
Copy link
Copy Markdown
Member

@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
Comment thread cranelift/codegen/meta/src/isa/x86/legalize.rs
@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
Copy Markdown

Subscribe to Label Action

cc @bnjbvr

Details 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member 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
Copy Markdown
Member 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.

Comment thread cranelift/codegen/src/isa/x86/enc_tables.rs Outdated
Comment thread cranelift/codegen/src/isa/x86/enc_tables.rs Outdated
Comment thread cranelift/filetests/filetests/isa/x86/simd-arithmetic-run.clif
Comment thread cranelift/filetests/filetests/isa/x86/simd-arithmetic-run.clif Outdated
Comment thread cranelift/filetests/filetests/isa/x86/simd-arithmetic-run.clif Outdated
@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