This is an archive of the discontinued LLVM Phabricator instance.

Support for soft fp16 to fp64 IEEE conversions
Needs ReviewPublic

Authored by ldrumm on Jul 29 2020, 11:07 AM.

Details

Summary

Support for soft fp16 to fp64 IEEE conversions

This patch adds initial support for lowering conversions from half to
double with single rounding.

No backends have been updated to use this builtin yet.

I'm particularly interested whether the exhaustive testcases make sense for non-identical bittpatterns for NaN values

Diff Detail

Event Timeline

ldrumm created this revision.Jul 29 2020, 11:07 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 29 2020, 11:07 AM
Herald added subscribers: Restricted Project, mgorny. · View Herald Transcript
ldrumm requested review of this revision.Jul 29 2020, 11:07 AM
atrosinenko added inline comments.
compiler-rt/test/builtins/Unit/extendhfdf2_test.c
10

Shouldn't it be marked with COMPILER_RT_ABI identically to the corresponding definition?

12

There is a COMPILE_TIME_ASSERT(expr) macro inside the int_util.h. Grepping the assumption string shows lots of relevant lines under compiler-rt/test/builtins/Unit. On the other hand, COMPILE_TIME_ASSERT is used sometimes as well. I wonder whether it is against the coding style for tests to use too generic macro. Or maybe those manually defined assumption_Ns just predates introduction of that descriptive macro.

ldrumm added inline comments.Aug 20 2020, 9:38 AM
compiler-rt/test/builtins/Unit/extendhfdf2_test.c
10

Yes you're right. It seems most of the other tests do this now. I'll update

12

Good suggestion; will fix

ldrumm updated this revision to Diff 286849.Aug 20 2020, 10:14 AM

Thanks @ldrumm! Some comments on the test follow.

compiler-rt/test/builtins/Unit/extendhfdf2_test.c
19

If I get it right, this testcases array will most probably be placed on the stack. As it is quite large, I would rather declare this struct separately and place it outside of main():

struct testcase {
  uint16_t src16;
  uint64_t dst64;
};

static const struct testcase testcases[] = {
// ...
};

int main() {
  // ...
28

If I remember correctly, such "write to one field, read from the another" cases may need special attention to not introduce Undefined Behavior. I would rather simply use toRep64, fromRep64 and so on from fp_test.h.

43

I would rather just use return err;

ldrumm added inline comments.Aug 21 2020, 4:56 AM
compiler-rt/test/builtins/Unit/extendhfdf2_test.c
19

Yep. Good point. Not everyone has an 8MiB stack by default. I'll make it a global

28

I worry that the rumours that type punning through a union being undefined have become greatly exaggerated. However I should perhaps save my rants for the C standards committee for not clearing up their mess. I accept your offer of helper functions ;)

34–35

indentation is all over the place

43

will do

ldrumm updated this revision to Diff 287015.Aug 21 2020, 5:36 AM
ldrumm marked 2 inline comments as done.

Sorry for such an iterative feedback. A couple of general thoughts:

  • It may be worth looking at D86453: [AArch64] Support conversion between fp16 and fp128: if I get it right, it introduces a TYPE_FP16 macro to be used instead of uint16_t when using it instead of half-size float. When one of this patch and D86453 lands, the other one will probably need some updates
  • Unlike many other tests, this one always checks all the test cases instead of failing with non-zero exit code on the first failed one. If something goes wrong with this libcall, build log can be flooded with up to 65k warning lines. Still I'm not sure whether this is an issue.
compiler-rt/lib/builtins/extendhfdf2.c
20

Where does the name __gnu_h2d_ieee comes from? Google finds this page only :)

compiler-rt/test/builtins/Unit/extendhfdf2_test.c
16–17

It may be worth giving these fields more descriptive names, something like argument and expected_result or so.

27

In most of the tests here, an actual libcall invocation is usually performed inside a separate function declared like this

int test__extendhfdf2(uint16_t a, uint64_t expected)

This function prints a message in case of an error, too.

I guess this is much less significant for a table-driven test. On the other hand, this seems to not harm here as well and makes the tests look a bit more uniformly.

29–31

Using compareResultD could probably eliminate this hand-written comparison. It has a slightly different semantics for NaN, though. Some of extendXfYf2 tests use it (as well as tests for many other fp-related libcalls), so it should probably be usable (except for adjusting extendhfdf2_testcases.inc). #include <math.h> could probably be dropped then.

ldrumm marked an inline comment as done.Aug 25 2020, 7:06 AM
ldrumm added inline comments.
compiler-rt/lib/builtins/extendhfdf2.c
20

Just copying the name mangling name used by libgcc. I can check with the gcc developers whether this is correct (libgcc doesn't have this conversion yet)

compiler-rt/test/builtins/Unit/extendhfdf2_test.c
16–17

I'll go with input and expected_output

27

I really don't see the value of that level of uniformity among programs that compute different things. This loop is incredibly simple and the extra abstraction doesn't really improve anything; a simple comparison and print statement has been moved somewhere else.

Premature abstraction is the root of all grepping
— Me

29–31

The semantics being different here actually looks useful. I can see that in compareResultD the NaN checks distinguish signalling and quiet NaNs. I think that's what I wanted when I asked about NaNs in the original review request. Thanks. I'll see how it fares then update here

ldrumm marked an inline comment as not done.Mar 30 2021, 3:12 AM

kito-cheng would you mind looking at the testing here and see whether the correctness testing of NaNs is sufficient?

compiler-rt/test/builtins/Unit/extendhfdf2_test.c