This is an archive of the discontinued LLVM Phabricator instance.

[libc++][VE] Support futex system call on VE
AbandonedPublic

Authored by kaz7 on Jan 18 2021, 4:48 AM.

Details

Reviewers
ldionne
simoll
k-ishizaka
Group Reviewers
Restricted Project
Summary

VE architecture doesn't distribute linux/futex.h. This patch modifies
linux implementation of atomic.cpp to to handle VE without linux/futex.h,
and to show erros if either linux/futex.h or special handling is not
implemented.

Diff Detail

Event Timeline

kaz7 created this revision.Jan 18 2021, 4:48 AM
kaz7 requested review of this revision.Jan 18 2021, 4:48 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 18 2021, 4:48 AM
ldionne requested changes to this revision.Jan 18 2021, 8:56 AM

Thanks for your contribution. However, it would be useful to have more information before going forward. The goal is to avoid adding random bits of complexity to the library for a system that isn't officially supported and maintained.

  • What is VEOS?
  • Is this part of an effort to port libc++ to that OS?
  • What parts of the library are working on the OS and what parts are not working? Do you plan to subset the library using knobs like LIBCXX_ENABLE_LOCALIZATION=OFF?
  • Are you able to provide CI for that OS?

Generally speaking, more information about what prompts this patch is welcome.

This revision now requires changes to proceed.Jan 18 2021, 8:56 AM
kaz7 added a comment.Jan 18 2021, 4:34 PM

Thanks for your contribution. However, it would be useful to have more information before going forward. The goal is to avoid adding random bits of complexity to the library for a system that isn't officially supported and maintained.

  • What is VEOS?
  • Is this part of an effort to port libc++ to that OS?
  • What parts of the library are working on the OS and what parts are not working? Do you plan to subset the library using knobs like LIBCXX_ENABLE_LOCALIZATION=OFF?
  • Are you able to provide CI for that OS?

Generally speaking, more information about what prompts this patch is welcome.

Thank you for comments and sorry for missing information. VE is a NEC SX Aurora vector engine. We ported llvm/clang/libraries to VE. We are upstreaming this port following the RFC, https://lists.llvm.org/pipermail/llvm-dev/2019-April/131580.html. VE is implemented as an accelerator. Ported llvm runs on x86 and generates binary programs for VE as a cross-compiler. At the moment, VE is an experimental target. We've upstreamed llvm/clang. Now we are upstreaming libraries like builtin/libunwind/libcxx in order to run llvm-test-suite. After that, we will try to make this VE as an official target in llvm with test-suite results.

  • The VEOS is an OS running on this accelerator. The VEOS is a linux subset, but almost all features are implemented.
  • This is part of an effort to port not only libc++ but also clang/llvm. VE has it's own proprietary c++ compiler and c++ library. We ports llvm/clang/libc++ to provide not a proprietary compiler and libraries for VE.
  • As far as I know, whole part of libc++ is working on VEOS well through libc++ test suite. Localization is working fine also.
  • Yes, we will provide CI to make VE as an official target of llvm.
kaz7 updated this revision to Diff 391810.Dec 3 2021, 9:56 PM

Rebase to the latest main. VE is now an official target of llvm.
I would like to support libcxx for VE also.

kaz7 updated this revision to Diff 391811.Dec 3 2021, 9:59 PM

Change the way to handle linux/futex.h to more generic way
for ease of understanding and modifying in future.

kaz7 retitled this revision from [VE] Define FUTEX values to [libc++][VE] Support futex system call on VE.Dec 3 2021, 10:00 PM
kaz7 edited the summary of this revision. (Show Details)
kaz7 updated this revision to Diff 391812.Dec 3 2021, 10:04 PM

Fix typo.

ldionne requested changes to this revision.Dec 7 2021, 3:53 PM
ldionne added inline comments.
libcxx/src/atomic.cpp
19–21

You should be able to remove all the CMake changes and replace this by

#if __has_include(<linux/futex.h>)
#  include <linux/futex.h>
#endif
69

This is incredibly brittle and not self-documenting. Is there no way that VE can implement linux/futex.h? After all, it does provide the SYS_futex syscall and it does support FUTEX_WAIT_PRIVATE, only not with that macro name. This seems to me like a classic "workaround for something missing on the platform side", and usually it's better to round those rough edges on the platform side instead of working around them in a bunch of places (like here).

This revision now requires changes to proceed.Dec 7 2021, 3:53 PM

Also, are you able to provide a builder that would run CI on VE?

kaz7 updated this revision to Diff 392710.Dec 8 2021, 4:22 AM

Modifies code following suggestions.

kaz7 marked an inline comment as done.Dec 8 2021, 4:26 AM

Also, are you able to provide a builder that would run CI on VE?

Yes. We have CI at https://lab.llvm.org/buildbot/#/builders/91. It runs builtin's tests at the moment. We will run libcxxabi tests, libcxx tests, and more soon.

libcxx/src/atomic.cpp
19–21

Thank you for suggestion. I modified code.

69

I understand how it looks like, but it is simply difficult to let OS peeople add linux/futex.h to their distribution. I tried, but it won't work. I can try it again and again...

kaz7 planned changes to this revision.Dec 8 2021, 4:27 AM
kaz7 edited the summary of this revision. (Show Details)
kaz7 marked an inline comment as done.

At least, I can update code by self-documenting.

Also, are you able to provide a builder that would run CI on VE?

Yes. We have CI at https://lab.llvm.org/buildbot/#/builders/91. It runs builtin's tests at the moment. We will run libcxxabi tests, libcxx tests, and more soon.

Libc++ uses pre-commit CI using BuildKite, as documented here: https://libcxx.llvm.org/AddingNewCIJobs.html#adding-new-ci-jobs

Can you please join our Discord Channel (https://discord.gg/jzUbyP26tQ) and reach out to me (@ldionne) there? I'll help you set it up, it's really simple. If you want, the same host will be able to run both pre-commit CI using BuildKite and the BuildBot setup, although I would recommend just running BuildKite since that's the only thing we look at.

kaz7 updated this revision to Diff 392799.Dec 8 2021, 8:42 AM

Update implementation as suggested.

kaz7 added a comment.Dec 8 2021, 8:47 AM

Also, are you able to provide a builder that would run CI on VE?

Yes. We have CI at https://lab.llvm.org/buildbot/#/builders/91. It runs builtin's tests at the moment. We will run libcxxabi tests, libcxx tests, and more soon.

Libc++ uses pre-commit CI using BuildKite, as documented here: https://libcxx.llvm.org/AddingNewCIJobs.html#adding-new-ci-jobs

Can you please join our Discord Channel (https://discord.gg/jzUbyP26tQ) and reach out to me (@ldionne) there? I'll help you set it up, it's really simple. If you want, the same host will be able to run both pre-commit CI using BuildKite and the BuildBot setup, although I would recommend just running BuildKite since that's the only thing we look at.

Thank you. I definitely want to do that. But, it's 1:44 AM here, Japan, and @simoll who are operating our buildbot lives in Europe. Let's organize meeting soon.

Thank you. I definitely want to do that. But, it's 1:44 AM here, Japan, and @simoll who are operating our buildbot lives in Europe. Let's organize meeting soon.

No hurries at all -- sorry if that wasn't clear :-).

Just ping me whenever suits you and we can take a look together.

libcxx/src/atomic.cpp
69

Can you ask these people to come here to see what they're asking us to do?

I'm sure they would rather provide linux/futex.h than have us hardcode magic numbers in our implementation.

kaz7 abandoned this revision.Dec 9 2021, 2:50 AM

I close this issue since the linux/futex.h for VE is available in the recent VE dstribution for RHEL8. The distribution for RHEL7 is simply not updated yet. I found that when I chatted about futex.h with colleageus in OS team. Thank you for your suggestion. ;-)

On the other hand, we also want to run pre-commit CI using BuildKite. So, we will definitely contact you about it soon. Thank you for your help.