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

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

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

llvmbot
Copy link
Member

@llvmbot llvmbot commented Aug 3, 2024

Backport 63a7786

Requested by: @rorth

@llvmbot llvmbot added this to the LLVM 19.X Release milestone Aug 3, 2024
@llvmbot
Copy link
Member Author

llvmbot commented Aug 3, 2024

@arichardson What do you think about merging this PR to the release branch?

Copy link
Contributor

@s-barannikov s-barannikov left a comment

Choose a reason for hiding this comment

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

@tru
Copy link
Collaborator

tru commented Sep 1, 2024

Any news on this? Should it be withdrawn considering there are issues with it?

@rorth
Copy link
Collaborator

rorth commented Sep 2, 2024

It's difficult: on one hand it fixes a Solaris/SPARC build failure. On the other, it's said to cause problems for an out-of-tree z/OS port. Unfortunately, the developers refuse to publish their code, so it's almost impossible to reason about that code.

@tru
Copy link
Collaborator

tru commented Sep 3, 2024

That does sound like it should be acceptable to merge if it's only blocking a out-of-tree implementation, since we don't officially support that config in that case. There is also the question as if we need to backport this - since if the main complaint for it not going into main is because of a external port is breaking might not be relevant in this case.

@s-barannikov @zibi2 @perry-ca

@perry-ca
Copy link
Contributor

perry-ca commented Sep 3, 2024

I have concerns about this change just thinking about this from the Solaris point of view. A couple questions:

  • from what I've learnt from Rainer, the Sparc 32-bit mode doesn't support long double. In that case they should change the CMakefile file so none of the TF functions are compiled.
  • why are you compiling the multc3 function and not the divtc3? I would expect that if the compiler can generate one, it would be able to generate the other.

It's difficult: on one hand it fixes a Solaris/SPARC build failure. On the other, it's said to cause problems for an out-of-tree z/OS port. Unfortunately, the developers refuse to publish their code, so it's almost impossible to reason about that code.

I take exception to the statement "the developers refuse to publish". The z/OS code is in the process of being published. No one has refused to publish code and we have worked with other contributors to compiler-rt to ensure the code builds on all platforms while we get the z/OS changes upstreamed. This change will need to be undone and a proper fix applied for Sparc when we enable the z/OS buildbots. I have asked @rorth to review #82789 and provide any changes required for Sparc. I haven't heard back yet.

For z/OS, I am ok with this change in the 19.x release since we only depend on trunk and 18.x. We do need to find a proper solution for Sparc in trunk.

@tru
Copy link
Collaborator

tru commented Sep 10, 2024

I'll merge this for 19.x then. Hopefully when 20 rolls around it will work out of the box.

…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)
@tru tru merged commit f64404e into llvm:release/19.x Sep 10, 2024
3 of 5 checks passed
Copy link

@rorth (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

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.

5 participants