[PATCH] D119926: [Clang][AArch64] Enable _Float16 _Complex type

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

Some comments.



================
Comment at: clang/include/clang/AST/ASTContext.h:1117
   CanQualType BFloat16Ty;
-  CanQualType Float16Ty; // C11 extension ISO/IEC TS 18661-3
+  CanQualType Float16Ty, Float16ComplexTy; // C11 extension ISO/IEC TS 18661-3
   CanQualType VoidPtrTy, NullPtrTy;
----------------
Is this necessary?


================
Comment at: clang/lib/AST/ASTContext.cpp:1330
   InitBuiltinType(Float16Ty,           BuiltinType::Float16);
+  InitBuiltinType(Float16ComplexTy,    BuiltinType::Float16);
 
----------------
Is this necessary?


================
Comment at: clang/lib/AST/ASTContext.cpp:6811-6812
     switch (EltRank) {
-    case BFloat16Rank: llvm_unreachable("Complex bfloat16 is not supported");
-    case Float16Rank:
+    case BFloat16Rank:
+      llvm_unreachable("Complex bfloat16 is not supported");
     case HalfRank: llvm_unreachable("Complex half is not supported");
----------------
This can be left unmodified.


================
Comment at: clang/test/CodeGen/aarch64-complex-half-math.c:1
+// RUN: %clang_cc1 %s -O1 -emit-llvm -triple aarch64-unknown-unknown -ffast-math -o - | FileCheck %s --check-prefix=AARCH64
+
----------------
Can these test outputs be autogenerated with update_cc_test_checks.py?


================
Comment at: clang/test/CodeGen/aarch64-complex-half-math.c:2
+// RUN: %clang_cc1 %s -O1 -emit-llvm -triple aarch64-unknown-unknown -ffast-math -o - | FileCheck %s --check-prefix=AARCH64
+
+_Float16 _Complex add_float_rr(_Float16 a, _Float16 b) {
----------------
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.


================
Comment at: clang/test/CodeGen/aarch64-complex-half-math.c:5
+  // AARCH64-LABEL: @add_float_rr(
+  // AARCH64: fadd reassoc nnan ninf nsz arcp afn half
+  // AARCH64-NOT: fadd
----------------
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.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119926/new/

https://reviews.llvm.org/D119926