Skip to content

Conversation

@bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Jul 26, 2021

No description provided.

@cfallin
Copy link
Member

cfallin commented Jul 26, 2021

Would you mind adding tests (probably runtests, constrain to x64-only for now unless aarch64 happens to support this already)?

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen labels Jul 26, 2021
@bjorn3
Copy link
Contributor Author

bjorn3 commented Jul 27, 2021

There don't seem to be any tests for fmin_pseudo and fmax_pseudo at all.

@cfallin
Copy link
Member

cfallin commented Jul 27, 2021

There don't seem to be any tests for fmin_pseudo and fmax_pseudo at all.

Yes, exactly; no better reason than that to add some (and thanks!) :-) It's odd that the vector variants don't have tests; you could either add that too if it's not too much work, or just leave it for another PR (maybe file an issue?) otherwise.

The PR at WebAssembly/simd#122, referenced in the instructions' description text, has some examples of how pseudo-min/pseudo-max differ from true min/max, specifically w.r.t. NaNs and signed zeroes. I think it makes sense to add a test with a few input tuples from those examples, along with some more ordinary cases.

@bjorn3 bjorn3 force-pushed the fminmax_pseudo_scalar branch from e2287cd to f3de81f Compare August 27, 2021 15:00
@bjorn3
Copy link
Contributor Author

bjorn3 commented Aug 27, 2021

preopt.serialized got accidentally added by one PR and changed by another. It should probably be added to .gitignore.

@bjorn3 bjorn3 force-pushed the fminmax_pseudo_scalar branch from f3de81f to 8adb40b Compare August 27, 2021 15:59
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Looks good with new tests -- thanks!

@cfallin cfallin merged commit 16854e7 into bytecodealliance:main Aug 30, 2021
@bjorn3 bjorn3 deleted the fminmax_pseudo_scalar branch August 30, 2021 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants