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

Implement int->float convert instructions #455

Merged
merged 2 commits into from
Aug 11, 2020
Merged

Implement int->float convert instructions #455

merged 2 commits into from
Aug 11, 2020

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Aug 4, 2020

Here situation is simple - we convert an integer to the closer floating point value. No special cases as range of f32 is greater than i64.

lib/fizzy/execute.cpp Outdated Show resolved Hide resolved
@chfast chfast force-pushed the fp_trunc branch 2 times, most recently from 25c50ab to eca4e3f Compare August 7, 2020 14:30
test/unittests/execute_floating_point_test.cpp Outdated Show resolved Hide resolved
EXPECT_THAT(execute(*instance, 0, {-max}), Result(-max_expected));

const auto min_expected = -std::pow(FloatT{2}, FloatT{IntLimits::digits});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder how std::pow is implemented, is it guaranteed to always have the exact result for power of 2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think pow() matters here, but floating point types can represent all powers of 2 around integer ranges due to having the exponent part which produces 2^e values.

@chfast chfast force-pushed the fp_trunc branch 2 times, most recently from a3c4df0 to 17bccc6 Compare August 7, 2020 21:16
Base automatically changed from fp_trunc to master August 8, 2020 06:42
@chfast chfast force-pushed the fp_convert branch 2 times, most recently from bd3ae4a to e5a3d18 Compare August 8, 2020 07:04
@codecov
Copy link

codecov bot commented Aug 8, 2020

Codecov Report

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

@@           Coverage Diff           @@
##           master     #455   +/-   ##
=======================================
  Coverage   99.53%   99.53%           
=======================================
  Files          54       54           
  Lines       15926    16015   +89     
=======================================
+ Hits        15852    15941   +89     
  Misses         74       74           

template <typename SrcT, typename DstT>
inline void convert(OperandStack& stack) noexcept
{
stack.top() = static_cast<DstT>(stack.top().as<SrcT>());
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's getting difficult to see which of these small helpers are suitable for which types. Maybe we should add static_assert here that SrcT is integer and DstT is floating point?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's why I used FloatT and IntT in tests. I will try doing something about it here.

Copy link
Member

Choose a reason for hiding this comment

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

These static asserts for types are nice, we should have them in each helper where it makes sense.

Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

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

Tests seems to be quite slim, but we can improve it later.

@chfast chfast merged commit 85e4b84 into master Aug 11, 2020
@chfast chfast deleted the fp_convert branch August 11, 2020 22:07
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.

3 participants