Implement float->int trunc instructions #447
Conversation
Codecov Report
@@ Coverage Diff @@
## master #447 +/- ##
========================================
Coverage 99.51% 99.51%
========================================
Files 54 54
Lines 15153 15349 +196
========================================
+ Hits 15079 15275 +196
Misses 74 74 |
chfast
changed the title
chfast
force-pushed
the
fp_trunc
branch
3 times, most recently
from
4255355
to
e2cb827
Compare
|
Please rebase. |
This comment has been minimized.
This comment has been minimized.
chfast
force-pushed
the
fp_trunc
branch
2 times, most recently
from
95e605e
to
c71fe26
Compare
chfast
force-pushed
the
fp_trunc
branch
2 times, most recently
from
5bcde1d
to
1de5c68
Compare
chfast
force-pushed
the
fp_trunc
branch
3 times, most recently
from
db2c82e
to
c0d90b6
Compare
chfast
force-pushed
the
fp_utils
branch
3 times, most recently
from
707f100
to
167c41b
Compare
lib/fizzy/execute.cpp
Outdated
| using boundaries = trunc_boundaries<SrcT, DstT>; | ||
|
|
||
| const auto input = stack.top().as<SrcT>(); | ||
| if (std::isnan(input) || input <= boundaries::lower || input >= boundaries::upper) |
There was a problem hiding this comment.
Why doesn't this need to check for inf/-inf?
There was a problem hiding this comment.
Because always: -inf <= boundaries::lower and inf >= boundaries::upper.
There was a problem hiding this comment.
Oh, actually, this can be optimized :/
There was a problem hiding this comment.
Even if that is true (seemingly it is because the tests pass), we should have a static_assert here for that, so that anyone reading the code understands the rules of wasm.
There was a problem hiding this comment.
I'm not sure what assert you would like to have here.
There was a problem hiding this comment.
-inf <= boundaries::lower
inf >= boundaries::lower
There was a problem hiding this comment.
No, this does not make sense to me. Every value is >= -inf.
| stack.top() = static_cast<DstT>(input); | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
After the optimisation I'd add assert(std::isnan(input) || input == inf || input == -inf); so that it matches the spec.
There was a problem hiding this comment.
Ok, added correct variants of these asserts.
There was a problem hiding this comment.
Don't want to stretch this too much, but can we also have the static_assert for inf vs. boundaries? Obviously we cannot make the nan-check compile time :)
So what now? |
Dunno, merge? |
The operator from the wasm spec is partial, i.e. it has some undefined results.
But, the execution spec requires to trap on operator's undefined result.