[PATCH] D33719: Add _Float16 as a C/C++ source language type

[Roger Ferrer Ibanez via Phabricator via cfe-commits]

rogfer01 added inline comments.


================
Comment at: include/clang/AST/Type.h:1669
   bool isHalfType() const;         // OpenCL 6.1.1.1, NEON (IEEE 754-2008 half)
+  bool isFloat16Type() const;      // FIXME
   bool isRealType() const;         // C99 6.2.5p17 (real floating + integer)
----------------
I think you want to make clear that this is
```
// C11 extension ISO/IEC TS 18661-3
```


================
Comment at: lib/AST/StmtPrinter.cpp:1434
   default: llvm_unreachable("Unexpected type for float literal!");
+  case BuiltinType::Float16:
   case BuiltinType::Half:       break; // FIXME: suffix?
----------------
Should this be `.f16` as suffix for consistency with the floating literal syntax?


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1932-1934
+      else if (value->getType()->isFloat16Ty()) {
+        FS = &CGF.getTarget().getHalfFormat();  //FIXME?
+      } else
----------------
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.


================
Comment at: lib/CodeGen/CodeGenTypes.cpp:445
+          getTypeForFormat(getLLVMContext(), Context.getFloatTypeSemantics(T),
+                           true);
+      break;
----------------
I think you can make this more obvious to the reader with a comment for this bool parameter.
```
/* UseNativeHalf */ true
```



https://reviews.llvm.org/D33719