This is an archive of the discontinued LLVM Phabricator instance.

Add _Float16 as a C/C++ source language type
ClosedPublic

Authored by SjoerdMeijer on May 31 2017, 5:25 AM.

Details

Diff Detail

Event Timeline

SjoerdMeijer created this revision.May 31 2017, 5:25 AM
rogfer01 added inline comments.May 31 2017, 6:22 AM
include/clang-c/Index.h
3118

This enumerator is the same as CXType_Float128 above, is that intended?

SjoerdMeijer added inline comments.May 31 2017, 6:26 AM
include/clang-c/Index.h
3118

Ah, thanks, copy-paste mistake. Will fix.

javed.absar added inline comments.May 31 2017, 7:11 AM
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.

bruno added a subscriber: bruno.Jun 1 2017, 6:20 PM

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.

bruno added a reviewer: bruno.Jun 1 2017, 6:21 PM
ahatanak added inline comments.
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;

Also, please create your new patch with context (git diff -U999999 other-branch).

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.

bruno edited edge metadata.Jun 6 2017, 2:50 PM

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.

rogfer01 added inline comments.Jun 15 2017, 1:26 AM
include/clang/AST/Type.h
1669 ↗(On Diff #102543)

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
2016–2017

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.

SjoerdMeijer updated this revision to Diff 103201.EditedJun 20 2017, 6:57 AM
SjoerdMeijer edited the summary of this revision. (Show Details)

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.

rsmith added a subscriber: rsmith.Jul 5 2017, 5:24 PM

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
2458–2461

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
594

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
2458–2461

I've created revision D35295 for the documentation update.

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.

Ping.

(CXX ABI change committed, doc patch created)

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

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.

rogfer01 accepted this revision.Aug 29 2017, 9:00 AM

Thanks @SjoerdMeijer

This LGTM now. Wait a couple of days in case @rsmith has more comments.

This revision is now accepted and ready to land.Aug 29 2017, 9:00 AM
rsmith added inline comments.Aug 29 2017, 6:33 PM
lib/Sema/SemaExpr.cpp
834–835

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
2–3

If you're going to check the tree structure from an AST dump, you should turn on --strict-whitespace in FileCheck.

Comments addressed. Thanks for reviewing.

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.

rsmith accepted this revision.Sep 6 2017, 12:48 PM

Many thanks for reviewing and your help!

This revision was automatically updated to reflect the committed changes.