[PATCH] D113107: Support of expression granularity for _Float16.

[Phoebe Wang via Phabricator via cfe-commits]

pengfei added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1313
+      !CGF.getContext().getTargetInfo().hasFeature("avx512fp16") &&
+      (TT.isX32() || TT.isX86());
+  if ((SrcType->isHalfType() || iSFloat16Allowed) &&
----------------
We don't need to check it. It is included by `TT.isX86()`.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1315
+  if ((SrcType->isHalfType() || iSFloat16Allowed) &&
+      !CGF.getContext().getLangOpts().NativeHalfType) {
     // Cast to FP using the intrinsic if the half type itself isn't supported.
----------------
rjmccall wrote:
> Okay, this condition is pretty ridiculous to be repeating in three different places across the compiler.  Especially since you're going to change it when you implement the new option, right?
> 
> Can we state this condition more generally?  I'm not sure why this is so narrowly restricted, and the variable name isn't telling me anything, since `_Float16` must by definition be "allowed" if we have an expression of `_Float16` type.
> since _Float16 must by definition be "allowed" if we have an expression of _Float16 type.

_Float16 is allowed only on a few targets. https://clang.llvm.org/docs/LanguageExtensions.html#half-precision-floating-point
By the way, we should update for X86 since it's not limited to avx512fp16 now.


================
Comment at: clang/test/CodeGen/X86/Float16-aritmetic.c:8-9
+  // CHECK: alloca half
+  // CHECK: store half {{.*}}, half*
+  // CHECK: store half {{.*}}, half*
+  // CHECK: load half, half*
----------------
This isn't correct without the ABI code change. I did some work in D107082. I plan to refactor (if I have enough time)


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

https://reviews.llvm.org/D113107