-
Notifications
You must be signed in to change notification settings - Fork 34
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 f{32,64}.nearest instructions #486
Conversation
chfast
commented
Aug 13, 2020
•
edited
Loading
edited
0c96df1
to
91fa494
Compare
e098abc
to
e4c2652
Compare
Codecov Report
@@ Coverage Diff @@
## master #486 +/- ##
==========================================
- Coverage 99.68% 99.67% -0.01%
==========================================
Files 54 54
Lines 17218 17108 -110
==========================================
- Hits 17163 17053 -110
Misses 55 55 |
e4c2652
to
1fcc1db
Compare
lib/fizzy/execute.cpp
Outdated
@@ -551,6 +552,18 @@ inline T ftrunc(T value) noexcept | |||
return std::trunc(value); | |||
} | |||
|
|||
template <typename T> | |||
T fnearest(T a) noexcept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to enforce the same NaN rules as #481, yet the spec seems to be the same regarding it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have them tested. I was expecting this will fail and I will need if (isnan())
case, but seems not required. Will check against the C spec though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as in other related functions: for NaN input it returns unspecified NaN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we making it deterministic for our case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we always return canonical NaN now as in other similar functions.
I also noticed that at least glibc is just setting the "quiet" bit in NaN to 1, leaving all others (including the sign) unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is done.
@@ -829,223 +829,6 @@ TEST(execute, memory_copy_32bytes) | |||
EXPECT_EQ(output, input); | |||
} | |||
|
|||
TEST(execute, fp_instructions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a landmark event!
auto instance = instantiate(parse(this->get_unop_code(Instr::f32_nearest))); | ||
const auto exec = [&](auto arg) { return execute(*instance, 0, {arg}); }; | ||
|
||
for (const auto& [arg, expected_trunc] : this->positive_trunc_tests) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self:
fnearest(+-inf) = +-inf
fnearest(+-0) = +-0
are included in positive_trunc_tests
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you leave this here as a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add this comment for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good now?
08def8c
to
b3282b4
Compare
778c3f2
to
4c85f0f
Compare
4c862ed
to
bc2bfea
Compare
This is a working version with We may spend another week to:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay bar the comment changes.
The test is to detect unimplemented floating-point instructions, but all are implemented now.