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

perry-ca · 2024-02-23T16:37:26Z

I found we had one more delta in addition to the change in #82208 that was needed to getting the builds for z/os to work again.

Since this PR was first created, a change to divtc3.c and multc3.c were made that don't work on z/OS. The changes in this PR should fix the original problem that broke z/OS without breaking z/OS.

The types supported on z/OS are:

a native 128-bit long double (slightly different than the gcc format but that difference doesn't matter for this)
no 128-bit int
all of the other usual types are supported too (eg. 64-bit doubles, 64-bit ints)

The original change in this PR fixed a compile error for QUAD_PRECISSION. The #if structure would land in the #error Unsupported TF mode type on z/OS instead of in the CRT_LDBL_128BIT case.

The current behaviour is that divtc3.c and multc3.c are compiling to empty .o files. This is because native 128-bit long double support is assumed to be only available if native 128-bit int support is too.

arichardson

Thanks this looks good to me, I will merge once I've tested various configurations locally.

arichardson · 2024-08-16T21:54:51Z

compiler-rt/lib/builtins/fp_lib.h

@@ -383,10 +383,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_TF_MODE) && defined(CRT_HAS_IEEE_TF)


The functions below work without int128, so this makes sense to me, but it would be good to check that this doesn't break 32-bit sparc.

@rorth can you confirm this works with 32-bit sparc (replacing your change in #101662 with this one). Thanks

While this does work with gcc-14, it breaks when using clang-20 instead: 26 builtins files fail with the likes of

FAILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-sparc.dir/extendhftf2.c.o /vol/llvm/src/llvm-project/dist/compiler-rt/lib/builtins/fp_lib.h:411:2: error: Unsupported TF mode type 411 | #error Unsupported TF mode type | ^

long double on SPARC is a royal mess, unfortunately: while the SPARC psABI (both 32 and 64-bit) requires long double to be 128 bit (although no current hardware does support that) and Solaris follows the spec, Linux/sparc64 chose to ignore that, keeping long double as 64 bit. While gcc gets this right, clang never did.

The following shows the values of the relevant macros:

clang-20 gcc-14 QUAD_PRECISION defined defined CRT_HAS_TF_MODE undef undef CRT_HAS_128BIT && CRT_HAS_F128 CRT_HAS_IEEE_TF undef defined same as next + __LDBL_MANT_DIG__ == 113 CRT_LDBL_128BIT undef defined __LDBL_MANT_DIG__ == 113 || (__FLT_RADIX__ == 16 && __LDBL_MANT_DIG__ == 28) CRT_HAS_F128 undef defined __LDBL_MANT_DIG__ 53 113 __FLT_RADIX__ 2 2

Due to all this, dealing with long double on SPARC is fragile as hell...

Given what you have said, I would suggest the real fix for SPARC is to change the list of source files for SPARC so you exclude all of the source files related to QUAD_PRECISSION. This looks like you just need to skip adding GENERIC_TF_SOURCES to the list of source files for SPARC.

So Clang and GCC disagree on the Linux ABI here? It sounds like clang should be following GCC and using 128-bit long double since that will be used for all existing Linux code?

If there is not 128-bit floating point type on sparc (at least with clang), the it sounds to me like it should not be building the tf files at all?

It looks like GCC uses IEEE 128-bit long double, so maybe we can just hoist the ifdefs above the fp_lib.h include?

@rorth Ping. Have you looked at this? I would like to get the z/os builds working again.

So Clang and GCC disagree on the Linux ABI here? It sounds like clang should be following GCC and using 128-bit long double since that will be used for all existing Linux code?

No: Solaris/sparc follows the ELF SPARC psABI which dictates that long double be 128-bit. While Linux/sparc, generally following the psABI, it chose to ignore that particular part of the spec and went for 64-bit long double instead. clang should indeed match the psABI/GCC/Solaris libc, but fixing this is way beyond my abilities, and LLVM SPARC maintenance these days is limited.

If there is not 128-bit floating point type on sparc (at least with clang), the it sounds to me like it should not be building the tf files at all?

There is on Solaris, but it's soft-float only.

It looks like GCC uses IEEE 128-bit long double, so maybe we can just hoist the ifdefs above the fp_lib.h include?

I've meanwhile found that the builtins situation is even messier than I thought: until LLVM 17, libclang_rt.builtins-sparc.a did contain __divtc3 and__multc3. Sometime before LLVM 18, those definitions got lost (and apparently there are no checks that the builtins interface remains stable). So instead of cementing that regression, the definitions should be restored, not removed for good. I've started looking into when the removal happened, but unfortunately so many intermediate revisions don't even build that this is very hard to do ;-(

What I believe should happen first is identify the revision that caused this breakage, than look into fixing that to continue working on SPARC together with whatever it was meant to achieve.

Thanks for the update.

Also look at the compiler side. Can the compiler generate calls to __divtc3 and __multc3 or any of the TF functions? If the compiler can't generate a call to these TF functions then it is ok to remove them from the archive even though LLVM 17 had them in it. I'd also look at what code would be in the __divtc3 in LLVM 17 if you can't provide that logfb function. How did that file compile in the first place?

When making your patch can you start with this change so we are building on top of the same code. And when you are ready ping me and I will try your change on z/OS.

… sparc

arichardson · 2024-09-10T14:23:07Z

This looks good to me if @rorth is fine with it.

perry-ca · 2024-09-10T18:32:49Z

@rorth I've updated this PR with the changes I think we need for Sparc 32-bit. Can you try it.

perry-ca · 2024-09-24T00:57:48Z

@rorth ping. Curious if you tried this out yet?

rorth · 2024-09-24T13:46:46Z

I've been quite busy with all sorts of issues, including several broken build.

I've now tried this patch on sparcv9-sun-solaris2.11, first in a 1-stage build with gcc-14, then a 2-stage build also starting from gcc-14. The results weren't good: in both cases I get 3 regressions:

  Builtins-sparc-sunos :: compiler_rt_fmaxl_test.c
  Builtins-sparc-sunos :: compiler_rt_logbl_test.c
  Builtins-sparc-sunos :: compiler_rt_scalbnl_test.c

The tests fail in the same way:

In file included from compiler-rt/test/builtins/Unit/compiler_rt_logbl_test.c:4:
compiler-rt/lib/builtins/fp_lib.h:402:2: error: Unsupported TF mode type
  402 | #error Unsupported TF mode type
      |  ^

Even worse, I happened to try a amd64-pc-freebsd14.0 build off the same tree: there the build was broken:

AILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-i386.dir/extendxftf2.c.o
[...]
In file included from /vol/llvm/src/llvm-project/dist/compiler-rt/lib/builtins/extendxftf2.c:13:
compiler-rt/lib/builtins/fp_lib.h:402:2: error:
Unsupported TF mode type
  402 | #error Unsupported TF mode type
      |  ^

This is getting more and more of a nightmare: it feels like completely flying in the dark, (sort of) fixing one port while simultaneously breaking at least two others. This cannot continue like this without a clear understanding how this is all supposed to work. It's just an enormous waste of time.

perry-ca · 2024-09-24T20:34:28Z

Thanks for trying this out @rorth. I agree with you. The builtins library touches a lot of platform uniqueness and as a result some of these problems can't be avoided. One thing I have noticed is that you can't run the unit tests for the builtins library unless you have at least one of the other higher level runtime libraries. That will be a major headache to fix.

The first three look to me like they should be excluded since these aren't being included in builtins. My quick thought is they need a line like // REQUIRES: librt_has_XXX. I see a couple other tests that define QUAD_PRECISION and have a requires line.

@arichardson any thoughts on the last one? I'm thinking we tweak the fb_libs.h a little more and use this conditional include structure for this block of code:

#elif defined(QUAD_PRECISION)
  #if defined(CRT_HAS_TF_MODE)
    #if defined(CRT_HAS_IEEE_TF)
    #else
      #error Unsupported TF mode type
    #endif
  #elif defined(CRT_LDBL_128BIT)
  #endif
#endif // *_PRECISION

I'll put this change up shortly once I try it out.

… defined

arichardson

This looks correct to me, but I have not tested it!

perry-ca · 2024-10-03T20:40:49Z

@rorth When you get a chance can you try this latest. Thanks

perry-ca · 2024-11-25T21:56:25Z

@rorth, gentle ping. Have you tried this? Thanks

perry-ca · 2025-01-17T16:20:21Z

@rorth ping. I've been a while. I'd really like you to try this before I merge it. Thanks

arichardson · 2025-01-17T18:03:00Z

compiler-rt/lib/builtins/CMakeLists.txt

+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()


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

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.

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

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.

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

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.

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.

arichardson · 2025-01-17T18:07:49Z

compiler-rt/lib/builtins/fp_lib.h

+#if defined(CRT_HAS_TF_MODE)
+#if defined(CRT_HAS_IEEE_TF)


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

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.

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.

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?

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.

arichardson

LGTM once @rorth is happy too.

This reverts commit fef5e08.

perry-ca · 2025-01-21T19:35:19Z

@arichardson, I reverted the previous change. Some builds failed. There are cases where CRT_HAS_TF_MODE and CRT_LDBL_128BIT are not defined. The trimmed down conditional would fall into the #error case.

fix build for z/os

66989a1

llvmbot added compiler-rt compiler-rt:builtins labels Feb 23, 2024

perry-ca mentioned this pull request Feb 23, 2024

Unbreak *tf builtins for hexfloat #82208

Merged

arichardson reviewed Feb 23, 2024

View reviewed changes

perry-ca closed this Apr 24, 2024

perry-ca deleted the perry/quad-non-tf-mode branch April 24, 2024 20:50

perry-ca mentioned this pull request Aug 16, 2024

[builtins] Fix divtc3.c etc. compilation on Solaris/SPARC with gcc #101662

Merged

perry-ca restored the perry/quad-non-tf-mode branch August 16, 2024 18:23

perry-ca reopened this Aug 16, 2024

arichardson reviewed Aug 16, 2024

View reviewed changes

perry-ca mentioned this pull request Sep 3, 2024

release/19.x: [builtins] Fix divtc3.c etc. compilation on Solaris/SPARC with gcc (#101662) #101847

Merged

perry-ca and others added 2 commits September 5, 2024 17:12

Merge branch 'llvm:main' into perry/quad-non-tf-mode

1996816

Fix files so div & mult are included for z/OS and excluded for 32-bit…

6c45d60

… sparc

perry-ca added 3 commits September 25, 2024 11:12

Merge branch 'llvm:main' into perry/quad-non-tf-mode

eb3bd1b

reorg some more so sparc doesn't get error when CRT_HAS_TF_MODE isn't…

b151da0

… defined

Merge branch 'main' into perry/quad-non-tf-mode

e4192b2

arichardson approved these changes Sep 27, 2024

View reviewed changes

Merge branch 'llvm:main' into perry/quad-non-tf-mode

6c8c0ab

arichardson reviewed Jan 17, 2025

View reviewed changes

perry-ca added 2 commits January 20, 2025 16:58

Merge remote-tracking branch 'origin/main' into perry/quad-non-tf-mode

4815fdc

Merge remote-tracking branch 'origin/main' into perry/quad-non-tf-mode

b3bcc70

simplify conditional code for QUAD

fef5e08

arichardson approved these changes Jan 21, 2025

View reviewed changes

Revert "simplify conditional code for QUAD"

5e87718

This reverts commit fef5e08.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

perry-ca commented Feb 23, 2024 •

edited

Loading

arichardson left a comment

arichardson Aug 16, 2024

perry-ca Aug 19, 2024

rorth Aug 20, 2024

perry-ca Aug 20, 2024

arichardson Aug 20, 2024

perry-ca Aug 23, 2024

rorth Aug 23, 2024

perry-ca Aug 23, 2024

arichardson commented Sep 10, 2024

perry-ca commented Sep 10, 2024

perry-ca commented Sep 24, 2024

rorth commented Sep 24, 2024

perry-ca commented Sep 24, 2024

arichardson left a comment

perry-ca commented Oct 3, 2024

perry-ca commented Nov 25, 2024

perry-ca commented Jan 17, 2025

arichardson Jan 17, 2025

perry-ca Jan 17, 2025

arichardson Jan 21, 2025

rorth Jan 30, 2025

perry-ca Jan 31, 2025

rorth Jan 31, 2025

perry-ca Feb 4, 2025

arichardson Jan 17, 2025

perry-ca Jan 17, 2025

arichardson Jan 17, 2025

perry-ca Jan 20, 2025 •

edited

Loading

arichardson Jan 21, 2025

arichardson left a comment

perry-ca commented Jan 21, 2025

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

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

Are you sure you want to change the base?

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

Conversation

perry-ca commented Feb 23, 2024 • edited Loading

arichardson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichardson commented Sep 10, 2024

perry-ca commented Sep 10, 2024

perry-ca commented Sep 24, 2024

rorth commented Sep 24, 2024

perry-ca commented Sep 24, 2024

arichardson left a comment

Choose a reason for hiding this comment

perry-ca commented Oct 3, 2024

perry-ca commented Nov 25, 2024

perry-ca commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perry-ca Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichardson left a comment

Choose a reason for hiding this comment

perry-ca commented Jan 21, 2025

perry-ca commented Feb 23, 2024 •

edited

Loading

perry-ca Jan 20, 2025 •

edited

Loading