Skip to content

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

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

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 10 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

@arichardson arichardson Jan 17, 2025

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

@perry-ca perry-ca Jan 17, 2025

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

@arichardson arichardson Jan 21, 2025

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

@rorth rorth Jan 30, 2025

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

@perry-ca perry-ca Jan 31, 2025

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

@rorth rorth Jan 31, 2025

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

@perry-ca perry-ca Feb 4, 2025

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
6 changes: 3 additions & 3 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,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)
#if defined(CRT_HAS_IEEE_TF) && defined(CRT_HAS_INT128)
// 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 Down Expand Up @@ -402,7 +402,7 @@ static __inline tf_float __compiler_rt_fmaxtf(tf_float x, tf_float y) {
#define crt_fabstf crt_fabsl
#define crt_copysigntf crt_copysignl
#else
#error Unsupported TF mode type
#error Unsupported QUAD_PRECISION mode
#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