[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: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:
> pengfei wrote:
> > 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.
> > _Float16 is allowed only on a few targets.
> 
> Yes, I know that.  But if `SrcType->isFloat16Type()` is true, we must be on one of those targets, because the type doesn't otherwise exist.
I see your point now. The problem here is we want to allow the `_Float16` to be used more broadly. But the target doesn't really support it sometime. Currently full arithmatic operations are supported only on target with AVX512FP16.
We should cast for those targets without AVX512FP16 while avoid to do on AVX512FP16.


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

https://reviews.llvm.org/D113107