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.
Details
Diff Detail
Event Timeline
include/clang-c/Index.h | ||
---|---|---|
3015 | This enumerator is the same as CXType_Float128 above, is that intended? |
include/clang-c/Index.h | ||
---|---|---|
3015 | Ah, thanks, copy-paste mistake. Will fix. |
include/clang/Sema/DeclSpec.h | ||
---|---|---|
283 | Any particular reason why its not "TST_float16" ? |
Fixed typos 'TST_float16' and 'CXType_Float16 = 30', and have also added it to a switch that I had missed.
Hi Sjoerd,
Thanks for working on this. Can you add context to your patch?
test/CodeGenCXX/float16-declarations-error.cpp | ||
---|---|---|
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?). |
test/CodeGenCXX/float16-declarations.cpp | ||
60 | 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. |
include/clang/Basic/TokenKinds.def | ||
---|---|---|
379 | 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? | |
lib/Lex/LiteralSupport.cpp | ||
588 | 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] == '1' && s[2] == '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)
Thanks,
Sjoerd.
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.
include/clang/AST/Type.h | ||
---|---|---|
1669 | I think you want to make clear that this is // C11 extension ISO/IEC TS 18661-3 | |
lib/AST/StmtPrinter.cpp | ||
1434 | Should this be .f16 as suffix for consistency with the floating literal syntax? | |
lib/CodeGen/CGExprScalar.cpp | ||
1932–1934 | 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. | |
lib/CodeGen/CodeGenTypes.cpp | ||
445 | I think you can make this more obvious to the reader with a comment for this bool parameter. /* UseNativeHalf */ true |
Thanks Roger. I did the clean up; there were indeed still a few fixmes there. The good thing is that it's a self-contained clang patch again: we don't need D34205, which I have abandoned.
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?
I have create a separate patch for the _Float16 preprocessor macro definitions in D34695.
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.
lib/AST/ItaniumMangle.cpp | ||
---|---|---|
2447–2450 | 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. | |
lib/Lex/LiteralSupport.cpp | ||
588 | s/tokens/characters/ |
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.
lib/AST/ItaniumMangle.cpp | ||
---|---|---|
2447–2450 | I've opened issue: |
This fixes:
- 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?
test/Frontend/float16.cpp | ||
---|---|---|
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.
Thanks @SjoerdMeijer
This LGTM now. Wait a couple of days in case @rsmith has more comments.
lib/Sema/SemaExpr.cpp | ||
---|---|---|
825–826 | 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.) | |
test/Frontend/float16.cpp | ||
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.
This enumerator is the same as CXType_Float128 above, is that intended?