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

Roger Ferrer Ibanez via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jun 15 01:26:57 PDT 2017

rogfer01 added inline comments.

Comment at: include/clang/AST/Type.h:1669
   bool isHalfType() const;         // OpenCL, 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


More information about the cfe-commits mailing list