Conversation
Codecov Report
@@ Coverage Diff @@
## master #689 +/- ##
=======================================
Coverage 99.02% 99.02%
=======================================
Files 80 80
Lines 12788 12812 +24
=======================================
+ Hits 12663 12687 +24
Misses 125 125
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| } | ||
|
|
||
| inline uint32_t clz32(uint32_t value) noexcept | ||
| constexpr uint32_t clz32(uint32_t value) noexcept |
There was a problem hiding this comment.
We can replace these with a template and use them as clz<uint32_t> below. Would cat this code duplication into half.
There was a problem hiding this comment.
Maybe we dont even need this wrapper layer, the only thing it does is explicit casting of the return value.
| } | ||
|
|
||
| inline uint32_t ctz32(uint32_t value) noexcept | ||
| constexpr uint32_t ctz32(uint32_t value) noexcept |
There was a problem hiding this comment.
| constexpr uint32_t ctz32(uint32_t value) noexcept | |
| inline constexpr uint32_t ctz32(uint32_t value) noexcept |
There was a problem hiding this comment.
Actually just removing these wrappers seems to be okay?
There was a problem hiding this comment.
Dropping the wrappers means all the results are going to be converted to i32. This is wrong for i64.c[lt]z. This mean you now need to construct a unit test what will reproduce this bug.
There was a problem hiding this comment.
So how about the template proposed in #689 (comment) ?
There was a problem hiding this comment.
This mean you now need to construct a unit test what will reproduce this bug.
This is still missing. It requires a special test where you pollute the stack first and then assign result of i64.clz. If the result is int some garbage should stay in the value. One case should be enough. See TEST(execute_numeric, i64_extend_i32_u).
I can also handle this later.
There was a problem hiding this comment.
It looks it is not easy to omit the wrapper so I will skip creating tests for this.
| namespace fizzy::test | ||
| { | ||
| constexpr std::pair<uint32_t, uint32_t> popcount32_test_cases[]{ | ||
| constexpr std::pair<uint32_t, int> popcount32_test_cases[]{ |
There was a problem hiding this comment.
Duplicating inputs does not make sense. Create a struct with results for all instructions.
template <T>
struct SomeGoodName
{
T input;
int popcount;
int countl_zero;
int countr_zero;
};
constexpr SomeGoodName<uint32_t> bit_counting32_test_cases[]{
{0, 0, 32, 32},
{0x80, 1, 24, 7},
...
};
There was a problem hiding this comment.
I will keep this a separate commit even for merging, as not sure it is more readable alltogether.
gumb0
left a comment
gumb0
left a comment
There was a problem hiding this comment.
Looks good, don't forget to remove the change in CMakeLists.txt
Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
4 participants
Closes #543.