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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
