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

S390x compatibility fixes #109

Merged
merged 4 commits into from
May 18, 2024

Conversation

jchecahi
Copy link

@jchecahi jchecahi commented Mar 6, 2024

With the new support (LLVM-18) for libomp and lld in s390x there are now tests that are unexpectedly passing (marked as failure), others that are not being run. The summary of changes in this PR is as follows:

  • LIBOMP presence is detected at cmake configuration step similarly to how all the binaries are detected, using cmake's find_library.
  • libomp feature added in lit.site.cfg. This is enabled depending on LIBOMP provided from cmake configuration.
  • libomp related tests are now have REQUIRES: libomp lit directive. These tests will now be marked UNSUPPORTED in s390x/LLVM < 18, and will run normally on LLVM >= 18
  • whole-toolchain testcases now run on s390x due to lld being supported. These tests are expected to fail, as building with -rtlib=compiler-rt will fail due to unsupported builtins library in the arch. Added XFAIL: s390x on them.

Additionally, fedora workflow in GH actions has been updated with latest supported versions.

@jchecahi
Copy link
Author

jchecahi commented Mar 6, 2024

Running this branch on a s390x system running latest Fedora Rawhide and LLVM 18.1.0~rc4 shows no failures:

# lit --show-xfail --show-unsupported -v tests
-- Testing: 51 tests, 2 workers --      
PASS: LLVM regression suite :: plugins_newpm.txt (1 of 51)
[...]
PASS: LLVM regression suite :: scanbuild_py.cpp (51 of 51)
********************
Unsupported Tests (1):
  LLVM regression suite :: test_hwasan.c

********************
Expectedly Failed Tests (5):
  LLVM regression suite :: lto_and_cfi.c
  LLVM regression suite :: lto_and_cross_dso_cfi.c
  LLVM regression suite :: unwind.cpp
  LLVM regression suite :: whole-toolchain.c
  LLVM regression suite :: whole-toolchain.cpp


Testing Time: 12.03s

Total Discovered Tests: 51
  Unsupported      :  1 (1.96%)
  Passed           : 45 (88.24%)
  Expectedly Failed:  5 (9.80%)

@jchecahi
Copy link
Author

jchecahi commented Mar 6, 2024

@sylvestre Could you take a look at the cmake logic and double check if it's a good way to approach the libomp detection?
Also, it seems that the ubuntu runs that failed are unrelated to the changes in this PR. They've been there for a while.

Jesus Checa Hidalgo added 4 commits March 7, 2024 09:22
From LLVM 18, libomp is supported in s390x. Previously all the libomp related
tests were marked as XFAIL for s390x, which is no longer valid and marks
passing tests as failures. With the libomp detection and a new REQUIRE rule for
them the XFAIL is no longer needed.
Also document missing ENABLE_STATIC_LIBCXX and fix typo for
ENABLE_LIBUNWIND
With the new s390x support for lld these testcases run now but are
expected to fail as '-rtlib=compiler-rt' will fail due to unsupported
builtins in the arch
@jchecahi jchecahi force-pushed the s390x-compatibility-fixes branch from 408f804 to 73acffe Compare March 7, 2024 08:22
@jchecahi
Copy link
Author

jchecahi commented Mar 7, 2024

@nikic pointed out internally that detecting libomp using clang -fopenmp could mask real failures in a scenario where clang search path is broken, for example. I updated the PR using a different approach based on finding the library instead.

@jchecahi
Copy link
Author

jchecahi commented Apr 5, 2024

@sylvestre ping, pretty please? :)

Copy link
Contributor

@tuliom tuliom left a comment

Choose a reason for hiding this comment

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

While I'm not a maintainer of this project, I did take a look at this PR.
With this PR, no tests fail on s390x.
Nice catch fixing the missing documentation!

The tests failing on Ubuntu are unrelated to this PR.

@sylvestre sylvestre merged commit b6d1532 into opencollab:main May 18, 2024
12 of 15 checks passed
@sylvestre
Copy link
Collaborator

sorry, i missed your notif?!

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

Successfully merging this pull request may close these issues.

3 participants