This adds _Float16 as a source language type, which is a 16-bit floating point type defined in C11 extension ISO/IEC TS 18661-3.
Thanks for working on this. Can you add context to your patch?
|1 ↗||(On Diff #100866)|
You don't need codegen to test this. Please move this test to test/Lexer, (see test/Lexer/half-literal.cpp).
Also, is --target=aarch64 really needed here? I don't see anything ARM specific in this patch (maybe because I'm missing more context on the patch?).
|59 ↗||(On Diff #100866)|
Please use -std=c++11 in the RUN line on top of the file and remove the #if here. Can you provide a more reduced testcase and try to put the CHECK lines closer to the statements you want to check? The same ARM question from the comment above also applies here.
|379 ↗||(On Diff #100866)|
Just wanted to confirm that your intention is to unconditionally support _Float16 (for example, c99 supports it too) even though it is a c11 extension. Is that correct?
|588 ↗||(On Diff #100866)|
Do you want to break out of the switch only when the second digit of the suffix is not 6 (for example, f15)?
I feel this is a bit simpler:
if (s + 2 < ThisTokEnd && s == '1' && s == '6') s += 2;
Hi Bruno, Akira,
Many thanks for your feedback! Apologies for the missing context. The patch touches many files and thus with context it is quite big (~4MB). Thought this would be too much if we need a few iterations. Anyway, will include it from now on.
I am working on a new revision and fixing an issue that I noticed while restructuring the regression test: it was actually not creating float16 literals properly. About the tests and using --target=aarch64: you're right that there should nothing be ARM specific here, but it is just that for Aarch64 it will show "half" IR types which I preferred, while it looks like x86 way of dealing with is to convert it and work on i16 types. Perhaps I need tests for both?
Yes, initially I wanted to unconditionally support _Float16, but now that you asked how about it, I agree it makes more sense to enable it for C11 and C++11 and have: KEYWORD(_Float16 , KEYC11|KEYCXX11)
About the tests and using --target=aarch64: you're right that there should nothing be ARM specific here, but it is just that for Aarch64 it will show "half" IR types which I preferred, while it looks like x86 way of dealing with is to convert it and work on i16 types. Perhaps I need tests for both?
The ideal is to have x86 and ARM tests when testing for the codegen part of it. Keep in mind that you need "REQUIRES: <target>-registered-target" for those.
Float16 is added as a native type. Implementing it as some sort of alias to fp16 caused too many type issues in expression/literals/etc., and thus was not an easier first step, and also in the end we want it to be a native type anyway. The tests have been restructured, and now also x86 is tested. It turned out I needed a helper function isFloat16Ty, for which I created llvm patch D34205.
|1669 ↗||(On Diff #102543)|
I think you want to make clear that this is
// C11 extension ISO/IEC TS 18661-3
|1434 ↗||(On Diff #102543)|
Should this be .f16 as suffix for consistency with the floating literal syntax?
|1932–1934 ↗||(On Diff #102543)|
I think you don't need this new LLVM type (that you introduce in D34205) as you are already able to tell the two types apart (_fp16 and _Float16) at the level of clang types. And your change does not seem to need it in any other place.
|445 ↗||(On Diff #102543)|
I think you can make this more obvious to the reader with a comment for this bool parameter.
/* UseNativeHalf */ true
I have added a fix for mixed __fp16 and _Float16 expressions: _Float16 type is converted to __fp16 type and then the operation is completed as if both operands were of __fp16 type. I've implemented this by adding _Float16 to the FloatingRank. By declaring Float16Rank to be smaller than HalfRank, we automatically get the promotions to __fp16 when required. I thought this approach is more elegant than adding more special cases in the floating point conversion functions.
I've added another test case test/Frontend/float16.cpp that checks the AST (this is in addition to the codegen test that we already have). This test also checks mixed type expressions.
This fixes the “DefaultVariadicArgumentPromotion” for Float16: they should be promoted to double, which makes now e.g. printf work.
I have added printf tests to both the AST and codegen test to check variadic functions (and thus checked that printf works), and also restructured the AST test a bit so that the check lines are near the corresponding program statement.
Yesterday I fixed mixed type (fp16 and float16) expressions, and with this fix for variadic arguments, I think the C support is ready. Do you agree @bruno, or do you think I've missed anything?
We need to be completely clear on the differences and interactions between _Float16 and __fp16; you should include a documentation update that describes this as part of this change or as a companion change. In particular, an explanation of why we need two different types here is essential.
|2457–2460 ↗||(On Diff #103397)|
Distinct types should have distinct manglings. Please open an issue at https://github.com/itanium-cxx-abi/cxx-abi/ to discuss how to mangle these cases.
|594 ↗||(On Diff #103397)|
Thanks Richard. I've opened an cxx abi issue, see comments inline. I will start working now on the doc update, and will do that in a companion change. Cheers.
|2457–2460 ↗||(On Diff #103397)|
I've opened issue:
- type mangling for _Float16 (approved here: https://github.com/itanium-cxx-abi/cxx-abi/pull/22, but not yet committed. )
- removed the argument promotion for _Float16 that I added because it turns out that it does not apply to _Float16 but only to the standard float types.
- the nit in the comments of the literal parser.
There are a few tests checking the precise interaction between __fp16 and _Float16 values but I don't see tests interacting _Float16 and the other standard floating types float, double and long double.
Do you reckon it is worth adding them as well?
|82 ↗||(On Diff #106228)|
Is this XXXXX some form of TODO?
No changes were needed to make the conversions work, the existing logic is taking care of that, but I agree it doesn't hurt to add a few test cases. So I've added tests to both files, and cleaned up that comment.
|727–728 ↗||(On Diff #113100)|
Do you mean "there is no default argument promotion for '_Float16'"? I think you could be even more specific: default argument promotion applies only to float. (And apparently to __half / __fp16.)
|1–2 ↗||(On Diff #113100)|
If you're going to check the tree structure from an AST dump, you should turn on --strict-whitespace in FileCheck.
I am going to commit this within a few days. That looks reasonable to me given that the comments in the last reviews were very minor (which I have of course addressed already). Also, in case of issues, I am guessing fixes and/or addition can be easily done post-commit, and if not a revert is cheap.
Committing this allows me to make some progress on the FP16 work. I have mostly fixed up the AArch64 back-end, and will now focus on ARM and the remaining Clang patches (documentation).
Comments are still welcome of course, which I then will address before committing.