Implement f{32,64}.{min,max} instructions #472
Conversation
chfast
commented
Aug 11, 2020
•
chfast
commented
lib/fizzy/execute.cpp
Outdated
| { | ||
| if (std::isnan(a)) | ||
| return a; | ||
| if (std::isnan(b)) |
There was a problem hiding this comment.
Are there any spectests deciding this order? e.g. what happens if both are nan?
To be clear: is there a possibility to have different kind of nans here?
There was a problem hiding this comment.
Yes. The standard NaN propagation rules apply.
|
Has bugs. |
chfast
force-pushed
the
fp_minmax
branch
2 times, most recently
from
ec42871
to
18c7652
Compare
chfast
force-pushed
the
fp_copysign
branch
4 times, most recently
from
ef1f54c
to
6d96868
Compare
| @@ -264,6 +264,8 @@ TYPED_TEST(execute_floating_point_types, binop_nan_propagation) | |||
| constexpr Instr opcodes[] = { | |||
| Instr::f32_add, | |||
| Instr::f32_div, | |||
| Instr::f32_min, | |||
| Instr::f32_max, | |||
There was a problem hiding this comment.
The spec says nothing about canonical nans, yet all these tests below pass?
There was a problem hiding this comment.
It does. Implicitly, by using nans_N{} pseudo-function. I.e. if all NaN inputs are canonical NaNs the output must be a canonical NaN as well. Otherwise, an arithmetic NaN is fine.
| auto instance = instantiate(parse(this->get_binop_code(Instr::f32_min))); | ||
|
|
||
| // Check every pair from cartesian product of the list of values. | ||
| for (size_t i = 0; i < std::size(this->ordered_special_values); ++i) |
There was a problem hiding this comment.
While using this list is nice, I think having explicit test cases based on the spec (in the opening commit) would be useful.
There was a problem hiding this comment.
Duplicating the same checks is less confusing?
There was a problem hiding this comment.
I'm not sold on this list, it is a pain looking it up every single time.
Codecov Report
@@ Coverage Diff @@
## master #472 +/- ##
========================================
Coverage 99.54% 99.55%
========================================
Files 54 54
Lines 16368 16494 +126
========================================
+ Hits 16294 16420 +126
Misses 74 74 |
| if (std::isnan(b)) | ||
| return b; | ||
| if (std::isnan(a) || std::isnan(b)) | ||
| return std::numeric_limits<T>::quiet_NaN(); |
There was a problem hiding this comment.
Where did this manifest itself as an error?
There was a problem hiding this comment.
In the NaN propagation test - for signaling NaN inputs, we must return an arithmetic NaN.
| inline constexpr T fmin(T a, T b) noexcept | ||
| { | ||
| if (std::isnan(a) || std::isnan(b)) | ||
| return std::numeric_limits<T>::quiet_NaN(); |
There was a problem hiding this comment.
Is it guaranteed that it's a canonical NaN? I know we have a test for that, but maybe it's worth a static_assert?
There was a problem hiding this comment.
No. But I don't have a way to check it here.
