Skip to content
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

Found one more delta to unbreak build for z/os #82789

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Found one more delta to unbreak build for z/os #82789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
66989a1
fix build for z/os
perry-ca Feb 23, 2024
1996816
Merge branch 'llvm:main' into perry/quad-non-tf-mode
perry-ca Sep 5, 2024
6c45d60
Fix files so div & mult are included for z/OS and excluded for 32-bit…
perry-ca Sep 6, 2024
eb3bd1b
Merge branch 'llvm:main' into perry/quad-non-tf-mode
perry-ca Sep 25, 2024
b151da0
reorg some more so sparc doesn't get error when CRT_HAS_TF_MODE isn't…
perry-ca Sep 25, 2024
e4192b2
Merge branch 'main' into perry/quad-non-tf-mode
perry-ca Sep 26, 2024
6c8c0ab
Merge branch 'llvm:main' into perry/quad-non-tf-mode
perry-ca Oct 7, 2024
4815fdc
Merge remote-tracking branch 'origin/main' into perry/quad-non-tf-mode
perry-ca Jan 20, 2025
b3bcc70
Merge remote-tracking branch 'origin/main' into perry/quad-non-tf-mode
perry-ca Jan 20, 2025
fef5e08
simplify conditional code for QUAD
perry-ca Jan 21, 2025
5e87718
Revert "simplify conditional code for QUAD"
perry-ca Jan 21, 2025
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
11 changes: 9 additions & 2 deletions compiler-rt/lib/builtins/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,8 +765,15 @@ set(riscv64_SOURCES
${riscv_SOURCES}
)

set(sparc_SOURCES ${GENERIC_SOURCES} ${GENERIC_TF_SOURCES})
set(sparcv9_SOURCES ${GENERIC_SOURCES} ${GENERIC_TF_SOURCES})
# Exclude the FT sources for 32-bit SPARC. Clang doesn't
# support 128-bit long double on 32-bit SPARC.
if("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "sparc")
set(sparc_SOURCES ${GENERIC_SOURCES})
set(sparcv9_SOURCES ${GENERIC_SOURCES})
else()
set(sparc_SOURCES ${GENERIC_SOURCES} ${GENERIC_TF_SOURCES})
set(sparcv9_SOURCES ${GENERIC_SOURCES} ${GENERIC_TF_SOURCES})
endif()
Comment on lines +770 to +776
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand this conditional. Why do we need to check COMPILER_RT_DEFAULT_TARGET_ARCH?

Also is the sparcv9 build 32-bit? If not could this be simplified to:

Suggested change
if("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "sparc")
set(sparc_SOURCES ${GENERIC_SOURCES})
set(sparcv9_SOURCES ${GENERIC_SOURCES})
else()
set(sparc_SOURCES ${GENERIC_SOURCES} ${GENERIC_TF_SOURCES})
set(sparcv9_SOURCES ${GENERIC_SOURCES} ${GENERIC_TF_SOURCES})
endif()
set(sparc_SOURCES ${GENERIC_SOURCES})
set(sparcv9_SOURCES ${GENERIC_SOURCES} ${GENERIC_TF_SOURCES})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something I need @rorth feedback on. I don't know enough about the versions of sparc and what's supported or not. From the discussions, it sounded like "sparc" is 32-bit and the other sparc* are 64-bit.

I think you are right. Hopefully Rainer, provides feedback too.

Copy link
Member

Choose a reason for hiding this comment

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

ping @rorth . I think this is the only open issue blocking this from being merged.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've meanwhile tried 2-stage builds on both sparcv9-sun-solaris2.11 and sparc64-unknown-linux-gnu, each starting from gcc-14 and clang-19. There were no failures.

However, the above is as wrong as it can get: on both Solaris/sparcv9 and Linux/sparc64 there were no TF files
compiled any longer, neither sparc nor sparcv9. For one, AFAICS MATCHES isn't anchored, so it matches all of sparc, sparcv9, and
sparc64. Besides, the concrete default SPARC target arch is completely irrelevant here: both a 64-bit-default compiler and a
32-bit-default compiler can produce both 32 and 64-bit objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @rorth. Can you provide a solution for this part that does what you need.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not right now. I seriously think we need to take two steps back first. So far, you haven't provided some quite crucial information:

  • What is the goal of this patch?
  • What exactly fails with current main?
  • Then for every change we need a detailed justification why the current code is wrong, while your proposed change is right, not just for s390x, but in general.

Right now, I can't help but feel that we've poking in the dark for months without any real progress or even understanding what the problem is. This is extremely tiring: I at least have long run out of energy, time, and patience about this patch. The upcoming releases of binutils 2.44, GCC 15, and LLVM 20 don't help in the slightest for this.

An added problem is the fact that to the best of my knowledge there's no public s390x system where one could try things for your target. OTOH, there's a Solaris/sparcv9 system in the cfarm that is perfectly capable of building and testing LLVM. I don't even know if there's any s390x ABI document available, so knowing the system's properties is all but impossible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then for every change we need a detailed justification why the current code is wrong, while your proposed change is right, not just for s390x, but in general.

@arichardson has reviewed this and approves the change. In this case the changes recently made to divtc3.c and multc3.c had a bad assumption connecting native 128-bit long double support to 128-bit int support. That is not generally correct. I could simply reverse that but I know that will break sparc hence the change to this file.

The answers to the other questions are in previous PRs. I'll add the answers in the description of this one to save searching.


set(wasm32_SOURCES
${GENERIC_TF_SOURCES}
Expand Down
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_128BIT) && defined(CRT_HAS_F128)
#if defined(CRT_HAS_F128)

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

Expand Down
10 changes: 6 additions & 4 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 @@ -368,10 +368,11 @@ 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)
#if defined(CRT_HAS_TF_MODE)
#if defined(CRT_HAS_IEEE_TF)
Comment on lines +372 to +373
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#if defined(CRT_HAS_TF_MODE)
#if defined(CRT_HAS_IEEE_TF)
#if defined(CRT_HAS_IEEE_TF) && defined(CRT_HAS_INT128)

Could we simplify this to just be

#if defined(CRT_HAS_IEEE_TF) && defined(CRT_HAS_INT128)
...
#elif defined(CRT_LDBL_128BIT)
...
#else
#error Unsupported TF mode type  
#endif

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 don't think that #if structure you suggest works with sparc. The logic of the sequence isn't semantically accurate either because the error in the else clause says all of the other conditions above are various TF mode and CRT_LDBL_128BIT is not one.

We will also be losing the check for CRT_HAS_TF_MODE.

The logic I tried to capture in my changes is "if has a TF mode pick the mode (or give an error), otherwise if has 128-bit long double ...". I don't think we can flatten this.

Copy link
Member

Choose a reason for hiding this comment

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

CRT_HAS_TF_MODE is defined as follows (f128 and int128):

// __(u)int128_t is currently needed to compile the *tf builtins as we would
// otherwise need to manually expand the bit manipulation on two 64-bit value.
#if defined(CRT_HAS_128BIT) && defined(CRT_HAS_F128)
#define CRT_HAS_TF_MODE
#endif

CRT_HAS_F128 has two options: CRT_HAS_IEEE_TF or CRT_LDBL_128BIT so I believe these checks are equivalent.

We only need the int128 support for the compiler-rt internal IEEE128 functions, for the non-IEEE we fall back to libc calls.

So in my proposed structure, I believe this would work for 32-bit sparc: it has 128-bit float which is ieee but no int128. But since it also has 128-bit long double it would take the second branch and use the libc functions that don't depend on int128.

Copy link
Contributor Author

@perry-ca perry-ca Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I tried out your proposed structure, and it works for z/OS. I'll make the change. Shall I change the wording in this:

#else
#error Unsupported TF mode type

The condition isn't entirely correct anymore since the CRT_LDBL_128BIT case wasn't a TF mode. The if/elif chain handles more than just the different TF modes.

If so, any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for testing! I guess we could do something more generic like "TF mode cannot be supported in current configuration", but keeping the current error seems fine to me.

// 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)
static __inline tf_float __compiler_rt_logbtf(tf_float x) {
return __compiler_rt_logbX(x);
}
Expand All @@ -386,6 +387,9 @@ static __inline tf_float __compiler_rt_fmaxtf(tf_float x, tf_float y) {
#define __compiler_rt_fmaxl __compiler_rt_fmaxtf
#define crt_fabstf crt_fabsf128
#define crt_copysigntf crt_copysignf128
#else
#error Unsupported TF mode type
#endif
#elif defined(CRT_LDBL_128BIT)
static __inline tf_float __compiler_rt_logbtf(tf_float x) {
return crt_logbl(x);
Expand All @@ -401,8 +405,6 @@ static __inline tf_float __compiler_rt_fmaxtf(tf_float x, tf_float y) {
#define __compiler_rt_fmaxl crt_fmaxl
#define crt_fabstf crt_fabsl
#define crt_copysigntf crt_copysignl
#else
#error Unsupported TF mode type
#endif

#endif // *_PRECISION
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_128BIT) && defined(CRT_HAS_F128)
#if defined(CRT_HAS_F128)

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

Expand Down
Loading