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

zahiraam added a subscriber: bkramer. zahiraam added a comment. Thanks @bkramer Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 ___ cfe-commits mailing list

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

zahiraam added a comment. In D113107#3747095 , @rjmccall wrote: > Thanks, LGTM Thanks for all the reviews. There is still more work to be done :) I will push it tomorrow as it's the end of day for me and I am afraid there will be some fails that I

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

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 ___ cfe-commits mailing list

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

rjmccall added a comment. Thank you! I think all the invariants look right here; just some minor comments now, and then it should be ready to commit. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:403 + Builder.CreateStore(Val.second, ImagPtr, isVolatile); +} +

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

pengfei added a comment. In D113107#3744782 , @rjmccall wrote: > In D113107#3744505 , @pengfei wrote: > >>> I'm not sure what optimization you mean. Because the ABI returns 16-bit and >>> 32-bit FP values

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

rjmccall added a comment. In D113107#3744505 , @pengfei wrote: >> I'm not sure what optimization you mean. Because the ABI returns 16-bit and >> 32-bit FP values differently, there really isn't a way that we can return a >> value without going through

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

pengfei added a comment. > I'm not sure what optimization you mean. Because the ABI returns 16-bit and > 32-bit FP values differently, there really isn't a way that we can return a > value without going through a truncation/extension cycle. I explained it to Zahira offline. I forgot we have

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:972 +} + } + auto result = Visit(const_cast(E)); The unary operator cases seem to have disappeared from this function. Comment at:

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

zahiraam added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:615 + ComplexPairTy result = VisitMinus(E, promotionTy); + return result; +} rjmccall wrote: > This is not unpromoting if the original `PromotionType` is null. The idea for this

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

rjmccall added a comment. I'm not sure what optimization you mean. Because the ABI returns 16-bit and 32-bit FP values differently, there really isn't a way that we can return a value without going through a truncation/extension cycle. There's potential to eliminate those with IPO, but we

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

zahiraam added a comment. In D113107#3743318 , @rjmccall wrote: > Somehow we've taken a huge step back on unpromotion, and I'm worried you're > now doing the exact thing I didn't want us doing and forcing all the > downstream clients to handle the

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

rjmccall added a comment. Somehow we've taken a huge step back on unpromotion, and I'm worried you're now doing the exact thing I didn't want us doing and forcing all the downstream clients to handle the possibility of a promoted result. Comment at:

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

pengfei added inline comments. Comment at: clang/test/CodeGen/X86/Float16-arithmetic.c:207 +// CHECK-NEXT:[[EXT:%.*]] = fpext half [[TMP0]] to float +// CHECK-NEXT:store float [[EXT]], ptr [[RETVAL]], align 2 +// CHECK-NEXT:[[TMP1:%.*]] = load half, ptr [[RETVAL]],

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

rjmccall added inline comments. Comment at: clang/test/CodeGen/X86/Float16-arithmetic.c:207 +// CHECK-NEXT:[[EXT:%.*]] = fpext half [[TMP0]] to float +// CHECK-NEXT:store float [[EXT]], ptr [[RETVAL]], align 2 +// CHECK-NEXT:[[TMP1:%.*]] = load half, ptr [[RETVAL]],

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

pengfei added inline comments. Comment at: clang/test/CodeGen/X86/Float16-arithmetic.c:207 +// CHECK-NEXT:[[EXT:%.*]] = fpext half [[TMP0]] to float +// CHECK-NEXT:store float [[EXT]], ptr [[RETVAL]], align 2 +// CHECK-NEXT:[[TMP1:%.*]] = load half, ptr [[RETVAL]],

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

rjmccall added inline comments. Comment at: clang/test/CodeGen/X86/Float16-arithmetic.c:207 +// CHECK-NEXT:[[EXT:%.*]] = fpext half [[TMP0]] to float +// CHECK-NEXT:store float [[EXT]], ptr [[RETVAL]], align 2 +// CHECK-NEXT:[[TMP1:%.*]] = load half, ptr [[RETVAL]],

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

zahiraam added inline comments. Comment at: clang/test/CodeGen/X86/Float16-arithmetic.c:207 +// CHECK-NEXT:[[EXT:%.*]] = fpext half [[TMP0]] to float +// CHECK-NEXT:store float [[EXT]], ptr [[RETVAL]], align 2 +// CHECK-NEXT:[[TMP1:%.*]] = load half, ptr [[RETVAL]],

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

zahiraam marked an inline comment as done. zahiraam added inline comments. Comment at: clang/test/CodeGen/X86/Float16-arithmetic.c:207 +// CHECK-NEXT:[[EXT:%.*]] = fpext half [[TMP0]] to float +// CHECK-NEXT:store float [[EXT]], ptr [[RETVAL]], align 2 +// CHECK-NEXT:

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

pengfei added inline comments. Comment at: clang/test/CodeGen/X86/Float16-arithmetic.c:42 _Float16 add2(_Float16 a, _Float16 b, _Float16 c) { return a + b + c; } Missing the same ternary operation test in complex tests? Comment at:

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

zahiraam added a comment. In D113107#3736325 , @rjmccall wrote: > In D113107#3736312 , @zahiraam > wrote: > >> This is a reduced test case from the codegen/complex-strictfp.c >> >> _Complex double g1, g2; >>

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

rjmccall added a comment. In D113107#3736312 , @zahiraam wrote: > This is a reduced test case from the codegen/complex-strictfp.c > > _Complex double g1, g2; > double D; > > void foo(void) { > > g1 = g1 + D; > > } > > The issue is that we are calling

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

zahiraam added a comment. This is a reduced test case from the codegen/complex-strictfp.c _Complex double g1, g2; double D; void foo(void) { g1 = g1 + D; } The issue is that we are calling in VisitBinAssign the function EmitUnpromotion (since promotionTy is null). This creates 2

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

rjmccall added a comment. IRGen has a strong postcondition about what IR types it expects specific from specific evaluations. Scalar expression emission is expected to return a scalar of type `ConvertType(E->getType())`, and complex expression emission is expected to return a pair of scalars

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

zahiraam marked 3 inline comments as done. zahiraam added a comment. Comment at: clang/test/CodeGen/X86/Float16-complex.c:1184 +// X86-NEXT:store float [[NEG_I]], ptr [[RETVAL_IMAGP]], align 2 +// X86-NEXT:[[TMP0:%.*]] = load <2 x half>, ptr [[RETVAL]], align 2 +//

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

rjmccall added a comment. That sounds right to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

pengfei added inline comments. Comment at: clang/test/Sema/Float16.c:13 - -#ifdef HAVE _Complex _Float16 a; rjmccall wrote: > I don't know why these test changes are in this patch. My understanding is > that we already committed a patch to enable `_Float16`,

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:613 +result = EmitUnpromotion(promotionTy, E->getSubExpr()->getType(), result); + return result; +} zahiraam wrote: > rjmccall wrote: > > You should unpromote only if we're not

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

zahiraam added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:613 +result = EmitUnpromotion(promotionTy, E->getSubExpr()->getType(), result); + return result; +} rjmccall wrote: > You should unpromote only if we're not in a promoted

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:613 +result = EmitUnpromotion(promotionTy, E->getSubExpr()->getType(), result); + return result; +} You should unpromote only if we're not in a promoted context, which is to

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:609 + ComplexPairTy Op; + PromotionType = getPromotionType(E->getSubExpr()->getType()); + if (!PromotionType.isNull()) zahiraam wrote: > rjmccall wrote: > > This is overwriting

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

zahiraam added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:609 + ComplexPairTy Op; + PromotionType = getPromotionType(E->getSubExpr()->getType()); + if (!PromotionType.isNull()) rjmccall wrote: > This is overwriting the argument, so the

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:213 TestAndClearIgnoreImag(); +PromotionType = getPromotionType(E->getSubExpr()->getType()); +if (!PromotionType.isNull()) Same problem Comment at:

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

zahiraam added a comment. With this last patch, the behaviors of CodeGen/volatile-1.c and CodeGenCXX/volatile-1.cpp have changed ( change with __imag). I have uploaded the changes but still trying to figure out what exactly changed and why. Thanks. CHANGES SINCE LAST ACTION

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

zahiraam added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3140 + return CGF.Builder.CreateFPExt(result, ConvertType(E->getType())); + } + return result; rjmccall wrote: > zahiraam wrote: > > rjmccall wrote: > > > Please extract this

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

rjmccall added a comment. In D113107#3726496 , @zahiraam wrote: > I think I introduced a bug when replacing the VisitUnaryMinus/Plus/Imag/Real > with VisitMinus/Plus/Imag/Real. Now this simple test case is failing in the > non-promotion path (with the

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

zahiraam added a comment. I think I introduced a bug when replacing the VisitUnaryMinus/Plus/Imag/Real with VisitMinus/Plus/Imag/Real. Now this simple test case is failing in the non-promotion path (with the +avx512fp16). _Float16 _Complex MinusOp_c_c(_Float16 c) { return -c; } error:

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

zahiraam marked 2 inline comments as done. zahiraam added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3140 + return CGF.Builder.CreateFPExt(result, ConvertType(E->getType())); + } + return result; rjmccall wrote: > Please extract this

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

rjmccall added a comment. Thanks, this is looking very close now. Please handle the unary minus case in the complex emitter as well; I think that's all we need in terms of basic features. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:296 + ComplexPairTy

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3112 +result = CGF.Builder.CreateFPExt(result, ConvertType(E->getType())); + return result; +} zahiraam wrote: > rjmccall wrote: > > The first version of the fallback path (the

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

zahiraam added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:950 +ComplexExprEmitter::EmitPromotedComplexOperand(const Expr *E, + QualType PromotionType) { + if (E->getType()->isAnyComplexType()) {

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:290 + } + return PromotedTy; + } Indentation is off Comment at: clang/lib/CodeGen/CGExprComplex.cpp:305 +result.second =

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

zahiraam added a comment. @pengfei , @rjmccall Thanks for the reviews. Sorry the updated patch took so long, I was on my sabbatical and returned back this week. I think I have responded to all the raised issues. Let me know if there is more to be done on this patch. Again thanks for the

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

zahiraam added inline comments. Comment at: clang/test/CodeGen/X86/Float16-arithmetic.c:5-14 + // CHECK-LABEL: @add1 + // CHECK: [[A:%.*]] = alloca half + // CHECK-NEXT: [[B:%.*]] = alloca half + // CHECK: [[A_LOAD:%.*]] = load half, ptr [[A]] + // CHECK-NEXT: [[A_EXT:%.*]]

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

zahiraam updated this revision to Diff 451261. zahiraam marked 7 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 Files: clang/docs/LanguageExtensions.rst clang/include/clang/Basic/TargetInfo.h

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

pengfei added a comment. > Then we need to add the option -fexcess-precision. I am not sure for now > where and what values the _FLT_EVAL_METHOD should have when excess precision > is enabled/disabled. I'm fine with a follow up patch to enable this option. Please notice LLVM15 will branch on

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

rjmccall added a comment. I think you should also at least support promoted arithmetic through the `_Real` and `_Imag` scalar operators before calling this complete. That should be simple — just a matter of calling EmitPromotedComplexExpr from the scalar path. Comment at:

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

zahiraam added a comment. Refactored the code as requested and fixed the compound operator promotion for scalar. The operators that are left to complete are compound operators for complex type and ternary operator for scalar and complex types. Then we need to add the option -fexcess-precision.

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

zahiraam updated this revision to Diff 441702. zahiraam marked 7 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseNotes.rst

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

pengfei added a comment. > Am I understanding correctly? @pengfei you are interested in the > -fexcess-precision=16 part of this right? @rjmccall what do yo think? I agree with @rjmccall , we just need to disable what we do here for `-fexcess-precision=16`. CHANGES SINCE LAST ACTION

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:964 TestAndClearIgnoreImag(); BinOpInfo Ops; rjmccall wrote: > Please make a helper function to emit an operand as a possibly-promoted > complex value. As requested, please

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

zahiraam updated this revision to Diff 441169. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseNotes.rst clang/include/clang/Basic/TargetInfo.h clang/lib/Basic/Targets/X86.cpp

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

zahiraam added a comment. Fixed LIT tests. Fixed EmitPromoted for the Complex emitter (not 100% sure about it?). Added compound operator scalar emulation. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107

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

zahiraam updated this revision to Diff 441167. zahiraam marked 2 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseNotes.rst

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

rjmccall added a comment. In D113107#3620014 , @zahiraam wrote: > In D113107#3615372 , @zahiraam > wrote: > >> In D113107#3606106 , @rjmccall >> wrote: >> >>> In

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

zahiraam added a comment. In D113107#3615372 , @zahiraam wrote: > In D113107#3606106 , @rjmccall > wrote: > >> In D113107#3606094 , @zahiraam >> wrote: >> >>> In

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

zahiraam marked an inline comment as done. zahiraam added a comment. In D113107#3606106 , @rjmccall wrote: > In D113107#3606094 , @zahiraam > wrote: > >> In D113107#3605797

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

pengfei added a comment. @zahiraam I'm going to enable the FE support when I reland the backend patch. Community people report correctness issue due to the ABI issue in compiler-rt. See https://github.com/llvm/llvm-project/issues/56204 CHANGES SINCE LAST ACTION

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3075 +ConvertType(DstType), ScalarConversionOpts()); +} + ...I wrote out what this function should look like, and Phabricator just threw it away. Let me try

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:934 + return ComplexExprEmitter(*this).EmitPromoted(E, DstTy); +} + rjmccall wrote: > `EmitPromotedComplexExpr` should look like `EmitPromotedScalarExpr`, i.e. it > should start

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

rjmccall added a comment. In D113107#3606107 , @zahiraam wrote: > For this test case: > _Float16 add_half_cr(_Float16 a, _Float16 b) { > > return a > b ? a : b; > > } > > are we expecting the phi node to be > > cond.true: > > %2 = load float , ptr >

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

zahiraam added a comment. For this test case: _Float16 add_half_cr(_Float16 a, _Float16 b) { return a > b ? a : b; } are we expecting the phi node to be cond.true: %2 = load float , ptr ... cond.false: %3 = load float, ptr ... %cond = phi float {{.*}} {{.*}} ? CHANGES SINCE

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

rjmccall added a comment. In D113107#3606094 , @zahiraam wrote: > In D113107#3605797 , @rjmccall > wrote: > >> I think on balance the right thing to do is probably to add an alternative >> to

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

zahiraam added a comment. In D113107#3605797 , @rjmccall wrote: > I think on balance the right thing to do is probably to add an alternative to > `-fexcess-precision`, like `-fexcess-precision=none`. We can default to > `-fexcess-precision=standard`

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

zahiraam added inline comments. Comment at: clang/docs/LanguageExtensions.rst:746 * SPIR -* X86 (Only available under feature AVX512-FP16) +* X86 (Enabled with feature SSE2 and up) rjmccall wrote: > Note that I'm not trying to propose this specific name for

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

zahiraam updated this revision to Diff 439502. zahiraam marked 2 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseNotes.rst

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

rjmccall added a comment. I think on balance the right thing to do is probably to add an alternative to `-fexcess-precision`, like `-fexcess-precision=none`. We can default to `-fexcess-precision=standard` and treat `-fexcess-precision=fast` as an alias for `standard` for now.

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

zahiraam updated this revision to Diff 439385. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseNotes.rst clang/include/clang/Basic/TargetInfo.h clang/lib/Basic/Targets/X86.cpp

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

zahiraam added a comment. In D113107#3604048 , @pengfei wrote: > @zahiraam, community requires to enable the `_Float16` support in FE, see > https://discourse.llvm.org/t/how-to-build-compiler-rt-for-new-x86-half-float-abi/63366 > Is there any blocking

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

zahiraam added a comment. In D113107#3603761 , @pengfei wrote: >> Supporting the lowering in the backend is sensible in order to support >> -fexcess-precision=16, because I agree that the most reasonable IR output in >> that configuration is to simply

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

pengfei added a comment. @zahiraam, community requires to enable the `_Float16` support in FE, see https://discourse.llvm.org/t/how-to-build-compiler-rt-for-new-x86-half-float-abi/63366 Is there any blocking issue to land it soon? Otherwise, we can split the changes in `X86.cpp`,

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

pengfei added a comment. > Supporting the lowering in the backend is sensible in order to support > -fexcess-precision=16, because I agree that the most reasonable IR output in > that configuration is to simply generate half operations. But under > -fexcess-precision=32, I do not want the

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

rjmccall added a comment. In D113107#3602478 , @zahiraam wrote: > Do we want to add an -fexcess-precision option? Well, generally we try to offer the same options GCC does. However, it looks like GCC's `-fexcess-precision` does not line up with the

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

zahiraam updated this revision to Diff 439130. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseNotes.rst clang/include/clang/Basic/TargetInfo.h clang/lib/Basic/Targets/X86.cpp

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

zahiraam updated this revision to Diff 439128. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseNotes.rst clang/include/clang/Basic/TargetInfo.h clang/lib/Basic/Targets/X86.cpp

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

zahiraam updated this revision to Diff 439113. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseNotes.rst clang/include/clang/Basic/TargetInfo.h clang/lib/Basic/Targets/X86.cpp

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

zahiraam updated this revision to Diff 439110. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseNotes.rst clang/include/clang/Basic/TargetInfo.h clang/lib/Basic/Targets/X86.cpp

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

zahiraam added a comment. In D113107#3601873 , @rjmccall wrote: > Supporting the lowering in the backend is sensible in order to support > `-fexcess-precision=16`, because I agree that the most reasonable IR output > in that configuration is to simply

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

rjmccall added a comment. Supporting the lowering in the backend is sensible in order to support `-fexcess-precision=16`, because I agree that the most reasonable IR output in that configuration is to simply generate `half` operations. But under `-fexcess-precision=32`, I do not want the

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

zahiraam added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:896 + +ComplexPairTy ComplexExprEmitter::EmitPromoted(const Expr *E) { + if (auto *BinOp = dyn_cast(E->IgnoreParens())) { pengfei wrote: > zahiraam wrote: > > pengfei wrote: > > >

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

zahiraam updated this revision to Diff 439020. zahiraam marked 2 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseNotes.rst

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

pengfei added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:896 + +ComplexPairTy ComplexExprEmitter::EmitPromoted(const Expr *E) { + if (auto *BinOp = dyn_cast(E->IgnoreParens())) { zahiraam wrote: > pengfei wrote: > > rjmccall wrote: > > >

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

zahiraam marked 7 inline comments as done. zahiraam added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:896 + +ComplexPairTy ComplexExprEmitter::EmitPromoted(const Expr *E) { + if (auto *BinOp = dyn_cast(E->IgnoreParens())) { pengfei wrote: >

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

pengfei added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:896 + +ComplexPairTy ComplexExprEmitter::EmitPromoted(const Expr *E) { + if (auto *BinOp = dyn_cast(E->IgnoreParens())) { rjmccall wrote: > pengfei wrote: > > rjmccall wrote: > > >

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:896 + +ComplexPairTy ComplexExprEmitter::EmitPromoted(const Expr *E) { + if (auto *BinOp = dyn_cast(E->IgnoreParens())) { pengfei wrote: > rjmccall wrote: > > zahiraam wrote: > > >

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

pengfei added inline comments. Comment at: clang/docs/ReleaseNotes.rst:491 +- Support for ``AVX512-FP16`` instructions has been added. +- Support for ``_Float16`` type has been added. This line doesn't need anymore. Comment at:

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:896 + +ComplexPairTy ComplexExprEmitter::EmitPromoted(const Expr *E) { + if (auto *BinOp = dyn_cast(E->IgnoreParens())) { zahiraam wrote: > rjmccall wrote: > > `EmitPromoted` should

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

zahiraam marked 2 inline comments as done. zahiraam added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:896 + +ComplexPairTy ComplexExprEmitter::EmitPromoted(const Expr *E) { + if (auto *BinOp = dyn_cast(E->IgnoreParens())) { rjmccall wrote:

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

rjmccall added a comment. In D113107#3585672 , @zahiraam wrote: > @rjmccall > The comma && ternary cases for EmitPromoted, I am not sure what needs to > happen there? Basically just the same thing that the normal paths do, except you recurse with

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

zahiraam added a comment. @rjmccall I made some progress on the requests above. There are still a few things that need to be nailed down. The comma && ternary cases for EmitPromoted, I am not sure what needs to happen there? I am also not quite sure about the ComplexExpr’s generated IR . I

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

zahiraam updated this revision to Diff 437191. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseNotes.rst clang/include/clang/Basic/TargetInfo.h clang/lib/Basic/Targets/X86.cpp

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

rjmccall added a comment. In D113107#3556886 , @zahiraam wrote: > In D113107#3554538 , @rjmccall > wrote: > >> Hmm. I'm sorry, seeing the impact of this, I think it's not great to have >> this `PromotionTy`

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

zahiraam added a comment. In D113107#3554538 , @rjmccall wrote: > Hmm. I'm sorry, seeing the impact of this, I think it's not great to have > this `PromotionTy` field; it's too hard to get the invariants right. Let's do > this instead: > > 1. Add an

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

rjmccall added a comment. Hmm. I'm sorry, seeing the impact of this, I think it's not great to have this `PromotionTy` field; it's too hard to get the invariants right. Let's do this instead: 1. Add an `EmitPromoted` method to `ScalarExprEmitter`. The general rule for this method is that it

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

pengfei added inline comments. Comment at: clang/lib/Basic/Targets/X86.cpp:242 HasAVX512FP16 = true; HasFloat16 = true; + HasLegalHalfType = true; This can be removed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/

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

zahiraam added a comment. In D113107#3520773 , @rjmccall wrote: > Okay, well, first off, we definitely shouldn't be repeating conditions like > `isX86()` all over the place. What you want to do is to have a general > predicate which answers whether we

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

zahiraam updated this revision to Diff 433104. Herald added a subscriber: jsji. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseNotes.rst clang/include/clang/Basic/TargetInfo.h

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

rjmccall added a comment. Okay, well, first off, we definitely shouldn't be repeating conditions like `isX86()` all over the place. What you want to do is to have a general predicate which answers whether we should emit this operation with excess precision; imagine an architecture that wanted

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

zahiraam added a comment. Finally got around to work on this. @rjmccall, @andrew.w.kaylor at your convenience please let me know your thoughts. There are a couple of things I'm still not sure about! Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/

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

zahiraam updated this revision to Diff 430115. zahiraam marked an inline comment as done. Herald added subscribers: llvm-commits, hiraditya. Herald added projects: LLVM, All. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 Files:

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

rjmccall added a comment. So, this is still approach #4, i.e. changing the types of expressions, which is not what GCC appears to be doing: https://godbolt.org/z/67hqeb37h. You need a custom emitter in IRGen for `_Float16`-typed arithmetic expressions. Comment at:

  1   2   >