Skip to content

Refactor LLVM easyblock and install OpenMP library symlinks #3815

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

Merged
merged 7 commits into from
Jul 2, 2025

Conversation

Flamefire
Copy link
Contributor

Based on #3812, see reduced changeset / diff to that

Only functional change: Install the OpenMP alias symlinks which we had before in the Clang easyblock. Doesn't hurt and avoids failures for programs expecting the alternative library names.

The rest are mostly small refactorings pulled out from #3755 to contain non-functional or minor changes.

Flamefire added 5 commits July 2, 2025 12:49
- Use f-Strings
- Update comments
- Use list append/extend
`LooseVersion(self.version)` in `sanity_check_step` can be done only once.
Comparisons of a LooseVersion to a string already converts the string.
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@Flamefire
Copy link
Contributor Author

Test report by @Flamefire

Overview of tested easyconfigs (in order)

Build succeeded for 2 out of 3 (3 easyconfigs in total)
n1612 - Linux RHEL 8.9 (Ootpa), x86_64, Intel(R) Xeon(R) Platinum 8470 (sapphirerapids), Python 3.9.18
See https://gist.github.com/Flamefire/31d93d58ce561bb256fb8a7c31a4be4a for a full test report.

@boegel
Copy link
Member

boegel commented Jul 2, 2025

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS LLVM-16.0.6-GCCcore-12.3.0.eb
  • SUCCESS LLVM-20.1.5-GCCcore-13.3.0.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
node3521.doduo.os - Linux RHEL 9.4, x86_64, AMD EPYC 7552 48-Core Processor (zen2), Python 3.9.18
See https://gist.github.com/boegel/b7ce092b10d4c5e655d51ab9f019d58c for a full test report.

@Flamefire
Copy link
Contributor Author

Test report by @Flamefire

Overview of tested easyconfigs (in order)

  • SUCCESS LLVM-19.1.7-GCCcore-13.3.0.eb
  • SUCCESS LLVM-20.1.5-GCCcore-13.3.0.eb
  • SUCCESS LLVM-18.1.8-GCCcore-13.3.0.eb

Build succeeded for 3 out of 3 (3 easyconfigs in total)
c71 - Linux AlmaLinux 9.4, x86_64, AMD EPYC 9334 32-Core Processor (zen4), 4 x NVIDIA NVIDIA H100, 560.35.03, Python 3.9.18
See https://gist.github.com/Flamefire/1d62028e5f2d41842416a2a43161fc76 for a full test report.

@boegel boegel merged commit 737b99a into easybuilders:develop Jul 2, 2025
17 checks passed
@boegel
Copy link
Member

boegel commented Jul 2, 2025

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS LLVM-18.1.8-GCCcore-13.3.0.eb
  • SUCCESS LLVM-19.1.7-GCCcore-13.3.0.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
node4210.shinx.os - Linux RHEL 9.4, x86_64, AMD EPYC 9654 96-Core Processor (zen4), Python 3.9.18
See https://gist.github.com/boegel/ac9720c0c0ca9fefa6982be6d110215c for a full test report.

@Crivella
Copy link
Contributor

Crivella commented Jul 2, 2025

@boegel @Flamefire
If i recall correctly we kept the aliases deliberately off to avoid bad interactions with other GCCcore+OpenMP software.
While it is ~possible for one of the 2 libraries to work with the other it is not bi-drectional and should be carefully tested before

At the very least until we have it tested or we have a sanity check to ensure GCCcore ECs do not ship software compiled with OpenMP i would keep them off

@Crivella
Copy link
Contributor

Crivella commented Jul 2, 2025

At the very least until we have it tested or we have a sanity check to ensure GCCcore ECs do not ship software compiled with OpenMP i would keep them off

@ocaisa i'm not sure if we discussed about this in an issue?

@Flamefire
Copy link
Contributor Author

At the very least until we have it tested or we have a sanity check to ensure GCCcore ECs do not ship software compiled with OpenMP i would keep them off

@ocaisa i'm not sure if we discussed about this in an issue?

I discussed this with @Thyre at #3755 (comment) where the missing libs/symlinks were the only difference of using the Clang vs the LLVM easyblock, so we assumed it was just missed.

@Flamefire Flamefire deleted the llvm-refactor branch July 3, 2025 06:41
@Crivella
Copy link
Contributor

Crivella commented Jul 3, 2025

As far as i remember from previous discussion the problems is when using LLVM as the base of a toolchain that reuses GCCcore stuff.
The risk, particularly without rpathing is the dynamic-linker picking up the wrong libomp vs libgomp when mixing libraries from GCCcore and LLVM.

@ocaisa
Copy link
Member

ocaisa commented Jul 3, 2025

The original discussion around this is at easybuilders/easybuild-framework#4535

@Flamefire
Copy link
Contributor Author

Thanks for the link!

So from what I understood: With Clang we wanted the symlink because it is a compiler, but with LLVM we don't?

I guess that means we should add an option for this to the easyblock such that clang easyconfigs continue to have it, see easybuilders/easybuild-easyconfigs#23055

@Crivella
Copy link
Contributor

Crivella commented Jul 3, 2025

LLVM is also going to act as a compiler for the toolchain, the problem will be the risk of mixing GCC with LLVM openmp libraries (since we decided to build the new LLVM toolchain on top of GCCcore instead of starting everything from scratch)

Also wondering what packages is hard using/checking the name of the OMP library instead of using -fopenmp and let the compiler deal with it

@Thyre
Copy link
Contributor

Thyre commented Jul 3, 2025

Also wondering what packages is hard using/checking the name of the OMP library instead of using -fopenmp and let the compiler deal with it

One of the worst offenders is CMake itself, which can directly link the OpenMP libraries instead of using the compiler flags.
This frequently breaks cases where users instrument their applications with Score-P, as we (Score-P) fail to identify OpenMP from being used.

However, I don't know how they determine the OpenMP library to link.

@Crivella
Copy link
Contributor

Crivella commented Jul 3, 2025

Shouldnt't CMake pick up the correct libraries for clang/flang?

I think the risk here is always running something built with libgomp using an aliased version of libomp5 instead of libgomp

@Flamefire
Copy link
Contributor Author

Shouldnt't CMake pick up the correct libraries for clang/flang?

Looks like CMake uses -fopenmp and extracts the libraries from that. The logic is a bit involved: https://github.com/Kitware/CMake/blob/master/Modules/FindOpenMP.cmake

So IMO CMake should be using the compiler libs as it queries the compiler first to see which libs are required and then hard-codes them to the linker invocation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants