Skip to content

[builtins] Generate __multc3 for z/OS #77554

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jan 12, 2024
Merged

[builtins] Generate __multc3 for z/OS #77554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7ba4d61
Generate __multc3 for z/OS
perry-ca Jan 10, 2024
81b814c
formatting update
perry-ca Jan 10, 2024
2cb932a
only define CRT_HAS_F128 & CRT_HAS_IEEE_TF for IEEE
perry-ca Jan 10, 2024
f94e376
Merge branch 'main' into compiler-rt-quad-funcs
perry-ca Jan 10, 2024
bb5c0c3
Update compiler-rt/lib/builtins/int_types.h
perry-ca Jan 11, 2024
4a24da7
Update compiler-rt/lib/builtins/fp_lib.h
perry-ca Jan 11, 2024
28e0981
Only generate the code if CRT_HAS_F128 is defined
perry-ca Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler-rt/lib/builtins/divtc3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
#define QUAD_PRECISION
#include "fp_lib.h"

#if defined(CRT_HAS_TF_MODE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

#if defined(CRT_HAS_F128)

// Returns: the quotient of (a + ib) / (c + id)

Expand Down
8 changes: 6 additions & 2 deletions compiler-rt/lib/builtins/fp_lib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,8 @@ static __inline void wideMultiply(rep_t a, rep_t b, rep_t *hi, rep_t *lo) {
#undef Word_HiMask
#undef Word_LoMask
#undef Word_FullMask
#else
typedef long double fp_t;
#endif // defined(CRT_HAS_TF_MODE)
#else
#error SINGLE_PRECISION, DOUBLE_PRECISION or QUAD_PRECISION must be defined.
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was actually required. Probably needs to be changed to #elif defined(QUAD_PRECISION) && defined(CRT_HAS_F128). @perry-ca Could you open a new PR with this change? See #77898 and #77880

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put up #77981 for review.

// The generic implementation only works for ieee754 floating point. For other
// floating point types, continue to rely on the libm implementation for now.
#if defined(CRT_HAS_IEEE_TF)
#if defined(CRT_HAS_IEEE_TF) && defined(CRT_HAS_128BIT)
static __inline tf_float __compiler_rt_logbtf(tf_float x) {
return __compiler_rt_logbX(x);
}
Expand Down Expand Up @@ -405,6 +407,8 @@ static __inline tf_float __compiler_rt_fmaxtf(tf_float x, tf_float y) {
#define __compiler_rt_logbl crt_logbl
#define __compiler_rt_scalbnl crt_scalbnl
#define __compiler_rt_fmaxl crt_fmaxl
#define crt_fabstf crt_fabsl
#define crt_copysigntf crt_copysignl
#else
#error Unsupported TF mode type
#endif
Expand Down
8 changes: 6 additions & 2 deletions compiler-rt/lib/builtins/int_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,12 +189,16 @@ typedef long double tf_float;
#define CRT_LDBL_IEEE_F128
#endif
#define TF_C(x) x##L
#elif __LDBL_MANT_DIG__ == 113
// Use long double instead of __float128 if it matches the IEEE 128-bit format.
#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
#if __LDBL_MANT_DIG__ == 113
#define CRT_HAS_IEEE_TF
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These last two macros should only be defined for the __LDBL_MANT_DIG__ == 113 case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double checked and Qcomplex is defined via CRT_LDBL_128BIT. I can limit CRT_HAS_F128 to IEEE too.

#define CRT_LDBL_IEEE_F128
#endif
typedef long double tf_float;
#define TF_C(x) x##L
#elif defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__)
Expand Down
2 changes: 1 addition & 1 deletion compiler-rt/lib/builtins/multc3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
#include "int_lib.h"
#include "int_math.h"

#if defined(CRT_HAS_TF_MODE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#if defined(CRT_HAS_F128)

// Returns: the product of a + ib and c + id

Expand Down