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

Optimize fabs, fneg, fcopysign #560

Merged
merged 1 commit into from
Oct 8, 2020

Conversation

Yellow-King21
Copy link
Contributor

@Yellow-King21 Yellow-King21 commented Sep 29, 2020

Closes #540.

I created optimized first single case of issue #540 to be sure that i understood a problem.

lib/fizzy/float_handling.hpp Outdated Show resolved Hide resolved
lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
@axic
Copy link
Member

axic commented Oct 2, 2020

Also please squash the commits into a single one.

lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
@axic axic marked this pull request as draft October 2, 2020 12:55
@codecov
Copy link

codecov bot commented Oct 3, 2020

Codecov Report

Merging #560 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #560   +/-   ##
=======================================
  Coverage   98.23%   98.24%           
=======================================
  Files          62       62           
  Lines        9023     9039   +16     
=======================================
+ Hits         8864     8880   +16     
  Misses        159      159           

@Yellow-King21 Yellow-King21 force-pushed the optimize-float-instruction branch 3 times, most recently from 94c147f to f545ae7 Compare October 3, 2020 14:47
lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
@axic axic requested a review from chfast October 5, 2020 15:40
@Yellow-King21 Yellow-King21 force-pushed the optimize-float-instruction branch 2 times, most recently from bae0f48 to b6d21fd Compare October 6, 2020 18:35
lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
@@ -356,6 +362,62 @@ T fnearest(T value) noexcept
return t;
}


template <typename T>
T fabs(T) noexcept = delete;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gumb0, @axic I'm currently in opinion that abs() is better name as we don't have iadd() and fadd(), just add().

Copy link
Member

Choose a reason for hiding this comment

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

I was actually going to suggest to keep the f prefix and also use fneg, given these functions are specifically working on floating points. Since there's no abs on integers in Wasm, it doesn't come up as an issue. Should there be one in the future, happy to rename it to abs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for fcopysign then?

Copy link
Member

@axic axic Oct 8, 2020

Choose a reason for hiding this comment

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

https://en.cppreference.com/w/cpp/numeric/math/copysign seems to be floating point only, so fine to keep it as copysign, but okay either way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Keeping fcopysign then.

lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
@Yellow-King21 Yellow-King21 force-pushed the optimize-float-instruction branch 4 times, most recently from 0ccb8fb to 9db9af5 Compare October 7, 2020 20:50
lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
@chfast chfast marked this pull request as ready for review October 8, 2020 14:37
@chfast chfast force-pushed the optimize-float-instruction branch 2 times, most recently from b2d1126 to 25c9dba Compare October 8, 2020 14:43
@@ -19,6 +20,11 @@ namespace
// code_offset + imm_offset + stack_height
constexpr auto BranchImmediateSize = 3 * sizeof(uint32_t);

constexpr uint32_t F32AbsMask = 0x7fffffff;
Copy link
Member

Choose a reason for hiding this comment

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

Could go crazy and use std::numeric_limits<uint32_t>::max() >> 1, but the constant is nicer.

Use unsigned integers and bit manipulations to implement three
floating-point operators: fabs, fneg, and fcopysign.

This is slightly better than using compiler's builtins because compiler
is able to inline them earlier and avoid using SSE registers required
by calling convention.

Co-authored-by: Paweł Bylica <[email protected]>
@chfast chfast force-pushed the optimize-float-instruction branch from 25c9dba to 21d65e9 Compare October 8, 2020 17:23
@chfast chfast changed the title Optimized abs instead std::fabs Optimize fabs, fneg, fcopysign Oct 8, 2020
@chfast chfast merged commit 50773ce into wasmx:master Oct 8, 2020
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.

Optimize fN.abs, fN.neg, fN.copysign instructions
4 participants