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.

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

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

Merged
merged 1 commit into from
Aug 3, 2024

Conversation

rorth
Copy link
Collaborator

@rorth rorth commented Aug 2, 2024

compiler-rt/lib/builtins/divtc3.c and multc3.c don't compile on Solaris/sparcv9 with gcc -m32:

FAILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-sparc.dir/divtc3.c.o
[...]
compiler-rt/lib/builtins/divtc3.c: In function ‘__divtc3’:
compiler-rt/lib/builtins/divtc3.c:22:18: error: implicit declaration of function ‘__compiler_rt_logbtf’ [-Wimplicit-function-declaration]
   22 |   fp_t __logbw = __compiler_rt_logbtf(
      |                  ^~~~~~~~~~~~~~~~~~~~

and many more. It turns out that while the definition of __divtc3 is guarded with CRT_HAS_F128, the __compiler_rt_logbtf and other declarations use CRT_HAS_128BIT && CRT_HAS_F128 as guard. This only shows up with gcc since, as documented in Issue #41838, clang violates the SPARC psABI in not using 128-bit long double, so this code path isn't used.

Fixed by changing the guards to match.

Tested on sparcv9-sun-solaris2.11.

`compiler-rt/lib/builtins/divtc3.c` and `multc3.c` don't compile on
Solaris/sparcv9 with `gcc -m32`:
```
FAILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-sparc.dir/divtc3.c.o
[...]
compiler-rt/lib/builtins/divtc3.c: In function ‘__divtc3’:
compiler-rt/lib/builtins/divtc3.c:22:18: error: implicit declaration of function ‘__compiler_rt_logbtf’ [-Wimplicit-function-declaration]
   22 |   fp_t __logbw = __compiler_rt_logbtf(
      |                  ^~~~~~~~~~~~~~~~~~~~
```
and many more.  It turns out that while the definition of `__divtc3` is
guarded with `CRT_HAS_F128`, the `__compiler_rt_logbtf` and other
declarations use `CRT_HAS_128BIT && CRT_HAS_F128` as guard.  This only
shows up with `gcc` since, as documented in Issue llvm#41838, `clang` violates
the SPARC psABI in not using 128-bit `long double`, so this code path isn't
used.

Fixed by changing the guards to match.

Tested on `sparcv9-sun-solaris2.11`.
@rorth rorth merged commit 63a7786 into llvm:main Aug 3, 2024
9 checks passed
@rorth
Copy link
Collaborator Author

rorth commented Aug 3, 2024

/cherry-pick 63a7786

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 3, 2024
…lvm#101662)

`compiler-rt/lib/builtins/divtc3.c` and `multc3.c` don't compile on
Solaris/sparcv9 with `gcc -m32`:
```
FAILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-sparc.dir/divtc3.c.o
[...]
compiler-rt/lib/builtins/divtc3.c: In function ‘__divtc3’:
compiler-rt/lib/builtins/divtc3.c:22:18: error: implicit declaration of function ‘__compiler_rt_logbtf’ [-Wimplicit-function-declaration]
   22 |   fp_t __logbw = __compiler_rt_logbtf(
      |                  ^~~~~~~~~~~~~~~~~~~~
```
and many more. It turns out that while the definition of `__divtc3` is
guarded with `CRT_HAS_F128`, the `__compiler_rt_logbtf` and other
declarations use `CRT_HAS_128BIT && CRT_HAS_F128` as guard. This only
shows up with `gcc` since, as documented in Issue llvm#41838, `clang`
violates the SPARC psABI in not using 128-bit `long double`, so this
code path isn't used.

Fixed by changing the guards to match.

Tested on `sparcv9-sun-solaris2.11`.

(cherry picked from commit 63a7786)
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 3, 2024

/pull-request #101847

@llvm-ci
Copy link
Collaborator

llvm-ci commented Aug 3, 2024

LLVM Buildbot has detected a new failure on builder clang-hip-vega20 running on hip-vega20-0 while building compiler-rt at step 3 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/3153

Here is the relevant piece of the build log for the reference:

Step 3 (annotate) failure: '../llvm-zorg/zorg/buildbot/builders/annotated/hip-build.sh --jobs=' (failure)
...
[38/40] : && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang++ -O3 -DNDEBUG  External/HIP/CMakeFiles/memmove-hip-6.0.2.dir/memmove.hip.o -o External/HIP/memmove-hip-6.0.2  --rocm-path=/buildbot/Externals/hip/rocm-6.0.2 --hip-link -rtlib=compiler-rt -unwindlib=libgcc -frtlib-add-rpath && cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /usr/local/bin/cmake -E create_symlink /buildbot/llvm-test-suite/External/HIP/memmove.reference_output /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/memmove.reference_output-hip-6.0.2
[39/40] /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang++ -DNDEBUG  -O3 -DNDEBUG   -w -Werror=date-time --rocm-path=/buildbot/Externals/hip/rocm-6.0.2 --offload-arch=gfx908 --offload-arch=gfx90a --offload-arch=gfx1030 --offload-arch=gfx1100 -xhip -mfma -MD -MT External/HIP/CMakeFiles/TheNextWeek-hip-6.0.2.dir/workload/ray-tracing/TheNextWeek/main.cc.o -MF External/HIP/CMakeFiles/TheNextWeek-hip-6.0.2.dir/workload/ray-tracing/TheNextWeek/main.cc.o.d -o External/HIP/CMakeFiles/TheNextWeek-hip-6.0.2.dir/workload/ray-tracing/TheNextWeek/main.cc.o -c /buildbot/llvm-test-suite/External/HIP/workload/ray-tracing/TheNextWeek/main.cc
[40/40] : && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang++ -O3 -DNDEBUG  External/HIP/CMakeFiles/TheNextWeek-hip-6.0.2.dir/workload/ray-tracing/TheNextWeek/main.cc.o -o External/HIP/TheNextWeek-hip-6.0.2  --rocm-path=/buildbot/Externals/hip/rocm-6.0.2 --hip-link -rtlib=compiler-rt -unwindlib=libgcc -frtlib-add-rpath && cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /usr/local/bin/cmake -E create_symlink /buildbot/llvm-test-suite/External/HIP/TheNextWeek.reference_output /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/TheNextWeek.reference_output-hip-6.0.2
+ build_step 'Testing HIP test-suite'
+ echo '@@@BUILD_STEP Testing HIP test-suite@@@'
+ ninja -v check-hip-simple
@@@BUILD_STEP Testing HIP test-suite@@@
[0/1] cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/llvm-lit -sv empty-hip-6.0.2.test with-fopenmp-hip-6.0.2.test saxpy-hip-6.0.2.test memmove-hip-6.0.2.test InOneWeekend-hip-6.0.2.test TheNextWeek-hip-6.0.2.test blender.test
-- Testing: 7 tests, 7 workers --
Testing:  0.. 10.. 20.. 30.. 40
FAIL: test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test (4 of 7)
******************** TEST 'test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test' FAILED ********************

/buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/timeit-target --timeout 7200 --limit-core 0 --limit-cpu 7200 --limit-file-size 209715200 --limit-rss-size 838860800 --append-exitstatus --redirect-output /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out --redirect-input /dev/null --summary /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.time /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/InOneWeekend-hip-6.0.2
cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP ; /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out InOneWeekend.reference_output-hip-6.0.2

+ cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP
+ /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out InOneWeekend.reference_output-hip-6.0.2
/buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target: Comparison failed, textual difference between 'M' and 'i'

********************
/usr/bin/strip: /bin/bash.stripped: Bad file descriptor
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test


Testing Time: 373.93s

Total Discovered Tests: 7
  Passed: 6 (85.71%)
  Failed: 1 (14.29%)
FAILED: External/HIP/CMakeFiles/check-hip-simple-hip-6.0.2 
cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/llvm-lit -sv empty-hip-6.0.2.test with-fopenmp-hip-6.0.2.test saxpy-hip-6.0.2.test memmove-hip-6.0.2.test InOneWeekend-hip-6.0.2.test TheNextWeek-hip-6.0.2.test blender.test
ninja: build stopped: subcommand failed.
Step 12 (Testing HIP test-suite) failure: Testing HIP test-suite (failure)
@@@BUILD_STEP Testing HIP test-suite@@@
[0/1] cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/llvm-lit -sv empty-hip-6.0.2.test with-fopenmp-hip-6.0.2.test saxpy-hip-6.0.2.test memmove-hip-6.0.2.test InOneWeekend-hip-6.0.2.test TheNextWeek-hip-6.0.2.test blender.test
-- Testing: 7 tests, 7 workers --
Testing:  0.. 10.. 20.. 30.. 40
FAIL: test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test (4 of 7)
******************** TEST 'test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test' FAILED ********************

/buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/timeit-target --timeout 7200 --limit-core 0 --limit-cpu 7200 --limit-file-size 209715200 --limit-rss-size 838860800 --append-exitstatus --redirect-output /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out --redirect-input /dev/null --summary /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.time /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/InOneWeekend-hip-6.0.2
cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP ; /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out InOneWeekend.reference_output-hip-6.0.2

+ cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP
+ /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out InOneWeekend.reference_output-hip-6.0.2
/buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target: Comparison failed, textual difference between 'M' and 'i'

********************
/usr/bin/strip: /bin/bash.stripped: Bad file descriptor
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test


Testing Time: 373.93s

Total Discovered Tests: 7
  Passed: 6 (85.71%)
  Failed: 1 (14.29%)
FAILED: External/HIP/CMakeFiles/check-hip-simple-hip-6.0.2 
cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/llvm-lit -sv empty-hip-6.0.2.test with-fopenmp-hip-6.0.2.test saxpy-hip-6.0.2.test memmove-hip-6.0.2.test InOneWeekend-hip-6.0.2.test TheNextWeek-hip-6.0.2.test blender.test
ninja: build stopped: subcommand failed.
program finished with exit code 1
elapsedTime=484.875180

kutemeikito added a commit to kutemeikito/llvm-project that referenced this pull request Aug 4, 2024
* 'main' of https://github.com/llvm/llvm-project:
  [RISCV] Improve hasAllNBitUsers for users of SLLI.
  [RISCV] Invert if conditions in the switch in RISCVDAGToDAGISel::hasAllNBitUsers. NFC
  [Transforms] Construct SmallVector with ArrayRef (NFC) (llvm#101851)
  [RISCV] Capitalize some variable names. NFC
  [sanitizer_common] Fix UnwindFast on SPARC (llvm#101634)
  [builtins] Fix divtc3.c etc. compilation on Solaris/SPARC with gcc (llvm#101662)
  [NFC][asan] Track current dynamic init module (llvm#101597)
  [libc] enable most of the entrypoints on aarch64 (llvm#101797)
  [SandboxIR][Tracker] Track InsertIntoBB (llvm#101595)
  [SCEV] Use const SCEV * explicitly in more places.
  [ELF] Move outputSections into Ctx. NFC
  [ELF] Move ElfSym into Ctx. NFC
  [ELF] Move Out into Ctx. NFC
  [test][asan] Fix the test checks
  [NFC][asan] Switch from list to DynInitGlobalsByModule (llvm#101596)

Signed-off-by: Edwiin Kusuma Jaya <kutemeikito0905@gmail.com>
banach-space pushed a commit to banach-space/llvm-project that referenced this pull request Aug 7, 2024
…lvm#101662)

`compiler-rt/lib/builtins/divtc3.c` and `multc3.c` don't compile on
Solaris/sparcv9 with `gcc -m32`:
```
FAILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-sparc.dir/divtc3.c.o
[...]
compiler-rt/lib/builtins/divtc3.c: In function ‘__divtc3’:
compiler-rt/lib/builtins/divtc3.c:22:18: error: implicit declaration of function ‘__compiler_rt_logbtf’ [-Wimplicit-function-declaration]
   22 |   fp_t __logbw = __compiler_rt_logbtf(
      |                  ^~~~~~~~~~~~~~~~~~~~
```
and many more. It turns out that while the definition of `__divtc3` is
guarded with `CRT_HAS_F128`, the `__compiler_rt_logbtf` and other
declarations use `CRT_HAS_128BIT && CRT_HAS_F128` as guard. This only
shows up with `gcc` since, as documented in Issue llvm#41838, `clang`
violates the SPARC psABI in not using 128-bit `long double`, so this
code path isn't used.

Fixed by changing the guards to match.

Tested on `sparcv9-sun-solaris2.11`.
@zibi2
Copy link
Contributor

zibi2 commented Aug 14, 2024

@rorth Rainer, this change broke the following libcxx'`s lit tests on z/OS:

std/numerics/complex.number/complex.transcendentals/pow_complex_complex.pass.cpp
std/numerics/complex.number/complex.member.ops/divide_equal_complex.pass.cpp
std/numerics/complex.number/complex.transcendentals/pow_complex_scalar.pass.cpp
std/numerics/complex.number/complex.ops/scalar_divide_complex.pass.cpp
std/numerics/complex.number/complex.transcendentals/atan.pass.cpp
std/numerics/complex.number/complex.transcendentals/atanh.pass.cpp
std/numerics/complex.number/complex.ops/complex_divide_complex.pass.cpp
std/numerics/complex.number/complex.member.ops/times_equal_complex.pass.cpp
std/numerics/complex.number/complex.transcendentals/pow_scalar_complex.pass.cpp
std/numerics/complex.number/cmplx.over/pow.pass.cpp

They all failed since after this change __divtc3 and __multc3 are not available. It looks like only CRT_HAS_F128 macro is defined. which was used originally to guard definitions of those functions.

On z/OS we don't define CRT_HAD_128BIT and according to this snippet code from int)types.h Microsoft as well so they might be affected as well.

Would you be able to revert this change and come up with the proper fix for this?

@zibi2
Copy link
Contributor

zibi2 commented Aug 14, 2024

In a nutshell the issue is when CRT_HAS_F128 is defined but CRT_HAS_128BIT is not.

@rorth
Copy link
Collaborator Author

rorth commented Aug 15, 2024

@rorth Rainer, this change broke the following libcxx'`s lit tests on z/OS:
[...]

Sorry for the failure.

They all failed since after this change __divtc3 and __multc3 are not available. It looks like only CRT_HAS_F128 macro is defined. which was used originally to guard definitions of those functions.

On z/OS we don't define CRT_HAD_128BIT and according to this snippet code from int)types.h Microsoft as well so they might be affected as well.

Me knowing nothing about z/OS and not having access to such a system, could you please provide the output of the compilation of divtc3.c with -g3 -dD -save-temps added? Going through the maze of macro definitions was bad enough when I had to on SPARC, on a foreign system it's pure guesswork.

Would you be able to revert this change and come up with the proper fix for this?

Given that the patch has been in for two weeks without complaint, could you give me a day or two to investigate and come up with a fix? This avoid the constant churn of Revert/Reland/Re-Revert/Re-Reland. Thanks.

@zibi2
Copy link
Contributor

zibi2 commented Aug 15, 2024

Me knowing nothing about z/OS and not having access to such a system, could you please provide the output of the compilation of divtc3.c with -g3 -dD -save-temps added? Going through the maze of macro definitions was bad enough when I had to on SPARC, on a foreign system it's pure guesswork.

Thank you for responding. The -save-tempsdoes not work. I'm going to attached output from compilingdivtc3.cwith--dD -E` added.
divthc3.c.txt

Given that the patch has been in for two weeks without complaint, could you give me a day or two to investigate and come up with a fix? This avoid the constant churn of Revert/Reland/Re-Revert/Re-Reland. Thanks.

Waiting fews days should be fine with us,.

@rorth
Copy link
Collaborator Author

rorth commented Aug 15, 2024

Me knowing nothing about z/OS and not having access to such a system, could you please provide the output of the compilation of divtc3.c with -g3 -dD -save-temps added? Going through the maze of macro definitions was bad enough when I had to on SPARC, on a foreign system it's pure guesswork.

Thank you for responding. The -save-tempsdoes not work. I'm going to attached output from compilingdivtc3.cwith--dD -E` added. divthc3.c.txt

-save-temps probably did work, but what's confusing is that the .i file is stored alongside the .o, not in the working directory as one would expect.

There are a few things that confuse me, though:

  • I find CRT_HAS_128BIT first defined (from int_types.h since __LP64__ is defined), that immediately undefined. However, in the source, that's guarded like this:
    #if defined(_MSC_VER) && !defined(__clang__)
    #undef CRT_HAS_128BIT
    #endif
    
    However, in the .i file _MSC_VER isn't defined while __clang__ is.
  • The .i file claims to be from compiler-rt/lib/builtins/divthc3.c, which files doesn't exist upstream. The body of the file isn't anything like divtc3.c either.
    Whatever the case, when looking at my patch again, I found that it should have guarded the __divtc3 and __multc3 definitions like this (relative to current sources):
diff --git a/compiler-rt/lib/builtins/divtc3.c b/compiler-rt/lib/builtins/divtc3.c
--- a/compiler-rt/lib/builtins/divtc3.c
+++ b/compiler-rt/lib/builtins/divtc3.c
@@ -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) && defined(CRT_HAS_TF_MODE)
 
 // Returns: the quotient of (a + ib) / (c + id)
 
diff --git a/compiler-rt/lib/builtins/multc3.c b/compiler-rt/lib/builtins/multc3.c
--- a/compiler-rt/lib/builtins/multc3.c
+++ b/compiler-rt/lib/builtins/multc3.c
@@ -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) && defined(CRT_HAS_TF_MODE)
 
 // Returns: the product of a + ib and c + id
 

All the functions use (__compiler_rt_logbtf and several more) are guarded by CRT_HAS_TF_MODE. That works on SPARC, but won't for you since it isn't defined in your .i file.
Are you maybe using modified sources?

Given that the patch has been in for two weeks without complaint, could you give me a day or two to investigate and come up with a fix? This avoid the constant churn of Revert/Reland/Re-Revert/Re-Reland. Thanks.

Waiting fews days should be fine with us,.

Good, thanks.

@zibi2
Copy link
Contributor

zibi2 commented Aug 15, 2024

-save-temps probably did work, but what's confusing is that the .i file is stored alongside the .o, not in the working directory as one would expect.

Unfortunately the .i is not created due to unrelated errors.

  • I find CRT_HAS_128BIT first defined (from int_types.h since __LP64__ is defined), that immediately undefined. However, in the source, that's guarded like this:
    #if defined(_MSC_VER) && !defined(__clang__)
    #undef CRT_HAS_128BIT
    #endif
    

Our downstream code has modified guard as follows:

#if (defined(_MSC_VER) && !defined(__clang__)) ||                              \
    defined(__MVS__)
#undef CRT_HAS_128BIT
#endif
  • The .i file claims to be from compiler-rt/lib/builtins/divthc3.c, which files doesn't exist upstream. The body of the file isn't anything like divtc3.c either.

Sorry I included wrong file, let me try again,
divtc3.c.txt

All the functions use (__compiler_rt_logbtf and several more) are guarded by CRT_HAS_TF_MODE. That works on SPARC, but won't for you since it isn't defined in your .i file. Are you maybe using modified sources?

Unfortunately, we don't have CRT_HAS_TF_MODE defined. I will check how come calling __compiler_rt_logbtf1 is fine for us.

@zibi2
Copy link
Contributor

zibi2 commented Aug 15, 2024

His is the downstream code (not up streamed yet) guarded with CRT_LDBL_128BIT

# 488 "/plex/zibi/llvm/Woz/llvm-project/compiler-rt/lib/builtins/fp_lib.h"
static __inline tf_float __compiler_rt_logbtf(tf_float x) {
  return __builtin_logbl((x));
}

Maybe the guard for the __divtc3 and __multc3 should be:

#if defined(CRT_HAS_F128) && (defined(CRT_HAS_TF_MODE) || defined(CRT_LDBL_128BIT))

kstoimenov pushed a commit to kstoimenov/llvm-project that referenced this pull request Aug 15, 2024
…lvm#101662)

`compiler-rt/lib/builtins/divtc3.c` and `multc3.c` don't compile on
Solaris/sparcv9 with `gcc -m32`:
```
FAILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-sparc.dir/divtc3.c.o
[...]
compiler-rt/lib/builtins/divtc3.c: In function ‘__divtc3’:
compiler-rt/lib/builtins/divtc3.c:22:18: error: implicit declaration of function ‘__compiler_rt_logbtf’ [-Wimplicit-function-declaration]
   22 |   fp_t __logbw = __compiler_rt_logbtf(
      |                  ^~~~~~~~~~~~~~~~~~~~
```
and many more. It turns out that while the definition of `__divtc3` is
guarded with `CRT_HAS_F128`, the `__compiler_rt_logbtf` and other
declarations use `CRT_HAS_128BIT && CRT_HAS_F128` as guard. This only
shows up with `gcc` since, as documented in Issue llvm#41838, `clang`
violates the SPARC psABI in not using 128-bit `long double`, so this
code path isn't used.

Fixed by changing the guards to match.

Tested on `sparcv9-sun-solaris2.11`.
@arichardson
Copy link
Member

If you can just call the long double libc versions, then it does sound like using
#if defined(CRT_HAS_TF_MODE) || defined(CRT_LDBL_128BIT) as the guard would work.

This would be equivalent to #if defined(CRT_HAS_F128) && (defined(CRT_HAS_128BIT) || defined(CRT_LDBL_128BIT)) but is IMO easier to understand.

@zibi2
Copy link
Contributor

zibi2 commented Aug 15, 2024

If you can just call the long double libc versions, then it does sound like using #if defined(CRT_HAS_TF_MODE) || defined(CRT_LDBL_128BIT) as the guard would work.

This would be equivalent to #if defined(CRT_HAS_F128) && (defined(CRT_HAS_128BIT) || defined(CRT_LDBL_128BIT)) but is IMO easier to understand.

We are fine with either one.

@rorth
Copy link
Collaborator Author

rorth commented Aug 16, 2024

Neither of those works for 32-bit Solaris/sparc with gcc 14: the declarations of __compiler_rt_logbtf and friends in fp_lib.h all depend on CRT_HAS_TF_MODE being defined. If it's not (as in the sparc case), we're again breaking what my original patch fixed.

The more I see this, the more I wonder if it's wise to make changes here that no in-tree target needs, arguing about a WIP z/OS port that may or may not need changes. Maybe it's better to keep things as is for now and revisit this when that port is submitted for real and can be reviewed/tested as is, rather than arguing about unknown downstream code. That would mean making whatever change is necessary to fix your issue locally for now.

@zibi2
Copy link
Contributor

zibi2 commented Aug 16, 2024

I would rather deal with this issue now when it's still fresh. Can you provide more details

I'm confused, If __compiler_rt_logbtf depends on CRT_HAS_TF_MODE the following guard should work:
#if defined(CRT_HAS_TF_MODE) || defined(CRT_LDBL_128BIT)
Do you mean in 32-bit Solaris/sparc with gcc CRT_HAS_TF_MODE is not defined but CRT_LDBL_128BIT is and therefor the call to __compiler_rt_logbtf is unresolved? Is there any other macro we can use to distinguish this case? If you don't mind can you provide -dM -E output for 32-bit Solaris/sparc with gcc and the 64-bit one which I assume works?

@perry-ca
Copy link
Contributor

The z/OS port is not a WIP. It's real and we are in the process of upstreaming everything. These breakages impact us and delay us being able to upstream everything. Alex and I worked on getting things working. I'd like to keep it working.

@perry-ca
Copy link
Contributor

perry-ca commented Aug 16, 2024

@rorth Can you try using the change in #82789 with your build instead of this change. I think that will solve your problem. Some how that pr got closed instead of being merged.

tru pushed a commit to llvmbot/llvm-project that referenced this pull request Sep 10, 2024
…lvm#101662)

`compiler-rt/lib/builtins/divtc3.c` and `multc3.c` don't compile on
Solaris/sparcv9 with `gcc -m32`:
```
FAILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-sparc.dir/divtc3.c.o
[...]
compiler-rt/lib/builtins/divtc3.c: In function ‘__divtc3’:
compiler-rt/lib/builtins/divtc3.c:22:18: error: implicit declaration of function ‘__compiler_rt_logbtf’ [-Wimplicit-function-declaration]
   22 |   fp_t __logbw = __compiler_rt_logbtf(
      |                  ^~~~~~~~~~~~~~~~~~~~
```
and many more. It turns out that while the definition of `__divtc3` is
guarded with `CRT_HAS_F128`, the `__compiler_rt_logbtf` and other
declarations use `CRT_HAS_128BIT && CRT_HAS_F128` as guard. This only
shows up with `gcc` since, as documented in Issue llvm#41838, `clang`
violates the SPARC psABI in not using 128-bit `long double`, so this
code path isn't used.

Fixed by changing the guards to match.

Tested on `sparcv9-sun-solaris2.11`.

(cherry picked from commit 63a7786)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

6 participants