[builtins] Generate __multc3 for z/OS #77554
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
perry-ca
changed the title
| #elif __LDBL_MANT_DIG__ == 113 || \ | ||
| (__FLT_RADIX__ == 16 && __LDBL_MANT_DIG__ == 28) | ||
| // Use long double instead of __float128 if it matches the IEEE 128-bit format | ||
| // or the IBM hexadecimal format. | ||
| #define CRT_LDBL_128BIT | ||
| #define CRT_HAS_F128 | ||
| #define CRT_HAS_IEEE_TF |
There was a problem hiding this comment.
These last two macros should only be defined for the __LDBL_MANT_DIG__ == 113 case.
There was a problem hiding this comment.
The CRT_HAS_F128 needs to be defined for hex float too. The Qcomplex and COMPLEXTF_REAL/COMPLEXTF_IMAGINARY ids are only defined if CRT_HAS_F128 is defined and multc3.c/divtc3.c depend on those.
I agree with CRT_HAS_IEEE_TF. This should also only be defined if CRT_HAS_TF_MODE is defined.
There was a problem hiding this comment.
I double checked and Qcomplex is defined via CRT_LDBL_128BIT. I can limit CRT_HAS_F128 to IEEE too.
| @@ -15,8 +15,6 @@ | |||
| #include "int_lib.h" | |||
| #include "int_math.h" | |||
|
|
|||
| #if defined(CRT_HAS_TF_MODE) | |||
There was a problem hiding this comment.
Is this diff still needed? The macro should be defined with the change to int_types.h. Or is the problem here that int128 is not supported?
There was a problem hiding this comment.
int128 isn't supported on z/OS yet. This change enables quad precision without needing to have TF mode enabled. We need this diff otherwise we don't get __multc3 defined.
Co-authored-by: Alexander Richardson <mail@alexrichardson.me>
Co-authored-by: Alexander Richardson <mail@alexrichardson.me>
| @@ -15,8 +15,6 @@ | |||
| #include "int_lib.h" | |||
| #include "int_math.h" | |||
|
|
|||
| #if defined(CRT_HAS_TF_MODE) | |||
There was a problem hiding this comment.
I'm not sure if we support architectures without a "tf" floating point type. To be safe I believe we still need a guard here since this file is compiled unconditionally. We just have to replace it with "CRT_HAS_F128".
A nice future cleanup would be to change CRT_HAS_TF_MODE to actually mean tf_float exists and explicitly guard for INT128 where needed. But that is a larger cleanup that probably shouldn't be part of this PR.
| @@ -13,8 +13,6 @@ | |||
| #define QUAD_PRECISION | |||
| #include "fp_lib.h" | |||
|
|
|||
| #if defined(CRT_HAS_TF_MODE) | |||
There was a problem hiding this comment.
I'd suggest replacing this with CRT_HAS_F128 as noted below (unless you're will to audit all current uses of CRT_HAS_TF_MODE since that currently is a proxy for TF_MODE&&INT128 rather than just TF_MODE).
|
Thanks. I don't have write access yet. Would you be able to merge this. |
| @@ -374,10 +376,10 @@ static __inline fp_t __compiler_rt_fmax(fp_t x, fp_t y) { | |||
| #endif | |||
| } | |||
|
|
|||
| #elif defined(QUAD_PRECISION) && defined(CRT_HAS_TF_MODE) | |||
| #elif defined(QUAD_PRECISION) | |||
There was a problem hiding this comment.
I've put up #77981 for review.
llvm#68132 ended up removing __multc3 & __divtc3 from compiler-rt library builds that have QUAD_PRECISION but not TF_MODE due to missing int128 support. I added support for QUAD_PRECISION to use the native hex float long double representation. --------- Co-authored-by: Alexander Richardson <mail@alexrichardson.me>
Reverts llvm#77554 because of llvm#77880
#68132 ended up removing __multc3 & __divtc3 from compiler-rt library builds that have QUAD_PRECISION but not TF_MODE. I added support for QUAD_PRECISION to use the native long double representation if not in TF_MODE.