Page MenuHomePhabricator

[Clang][AArch64] Enable _Float16 _Complex type
AbandonedPublic

Authored by MattDevereau on Feb 16 2022, 2:45 AM.

Diff Detail

Event Timeline

MattDevereau created this revision.Feb 16 2022, 2:45 AM
MattDevereau requested review of this revision.Feb 16 2022, 2:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2022, 2:45 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
paulwalker-arm added inline comments.Feb 16 2022, 2:53 AM
clang/include/clang/AST/ASTContext.h
1117 ↗(On Diff #409186)

Is this necessary? I don't see any other *ComplexTy types in this file (i.e. there is no DoubleComplexTy).

clang/lib/AST/ASTContext.cpp
1330

As above.

Some comments.

clang/include/clang/AST/ASTContext.h
1117 ↗(On Diff #409186)

Is this necessary?

clang/lib/AST/ASTContext.cpp
1330

Is this necessary?

6810–6812

This can be left unmodified.

clang/test/CodeGen/aarch64-complex-half-math.c
2

Can these test outputs be autogenerated with update_cc_test_checks.py?

3

I think this test needs a // REQUIRES: aarch64-registered-target to protect builds which don't have a LLVM_TARGETS_TO_BUILD which implies AArch64 is available.

6

My read is that this appears to be testing both -ffast-math and _Float16. Do we want to be testing both these things at once like this, can anyone comment?

My preference would be to have these test only the operators, and not the flags, and then maybe have a single test which does
https://clang.llvm.org/docs/LanguageExtensions.html#extensions-to-specify-floating-point-flags

#pragma clang fp contract(fast)

and verifies that the flags are present.

fhahn added a subscriber: fhahn.
fhahn added inline comments.
clang/test/CodeGen/aarch64-complex-half-math.c
2

Do the tests need to depend on LLVM optimizations? In general this is undesirable as it makes them more fragile if things change on the LLVM side.

aaron.ballman added inline comments.Feb 16 2022, 5:33 AM
clang/include/clang/AST/ASTContext.h
1117 ↗(On Diff #409186)

This is the wrong way to handle this -- complex types are stored in the ComplexTypes folding set and are created through ASTContext::getComplexType(). So I don't think this change is necessary.

clang/lib/AST/ASTContext.cpp
1330

Same here.

Removed -O1 and -ffast-math flags

peterwaller-arm accepted this revision.Feb 16 2022, 7:48 AM

LGTM.

clang/test/CodeGen/aarch64-complex-half-math.c
6

Discussed off-review: concluded for now that a no-optimizations test with no fast math flags seems reasonable, to address @fhahn's comment.

This revision is now accepted and ready to land.Feb 16 2022, 7:48 AM
aaron.ballman added inline comments.Feb 17 2022, 5:22 AM
clang/test/CodeGen/aarch64-complex-half-math.c
3

There's nothing AArch64-specific about the change, so I'd expect some more general codegen tests.

Actually, I took a look to see what calls getFloatingTypeOfSizeWithinDomain() and I can't find any callers of that. Do you have a test case where you were hitting that particular unreachable?

clang/test/CodeGen/aarch64-complex-half-math.c
3

Thanks for your observation @aaron.ballman.

It appears this has been working since D105331, which enabled it for X86.

Unless I'm missing something, the function in question appears to have been unused since 2010 (the last use was removed in d005ac937e4c08e0c607ddc42708f8bf2064c243 by @rjmccall), I presume getFloatingTypeOfSizeWithinDomain should be removed then.

aaron.ballman added inline comments.Feb 23 2022, 6:10 AM
clang/test/CodeGen/aarch64-complex-half-math.c
3

I presume getFloatingTypeOfSizeWithinDomain should be removed then.

Agreed; I've gone ahead and removed it in 841355c1e4e35fc02b5b171419979f5f9af0ebc8.

I think this patch can be abandoned, unless you think the extra test coverage is worth adding as an NFC commit?

MattDevereau abandoned this revision.EditedFeb 28 2022, 5:34 AM

Abandoning this patch as it is redundant due to changes in D105331. This patch also drew attention to unused code that has since been removed in 841355c1e4e35fc02b5b171419979f5f9af0ebc8. Many thanks to @aaron.ballman