This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Changes to enable MSVC ARM64 build of libomp
ClosedPublic

Authored by pulidocr on Apr 23 2021, 9:00 AM.

Details

Summary

This is the first in a series of changes to the OpenMP runtime that have been done internally by Microsoft. This patch makes the necessary changes to enable libomp.dll to build with the MSVC compiler targeting ARM64.

Diff Detail

Event Timeline

pulidocr created this revision.Apr 23 2021, 9:00 AM
pulidocr requested review of this revision.Apr 23 2021, 9:00 AM
Herald added a project: Restricted Project. · View Herald Transcript
jdoerfert added a subscriber: AndreyChurbanov.

@AndreyChurbanov is someone you want to have as reviewer on all these patches, you can also include others I added here.

natgla added a subscriber: natgla.Apr 27 2021, 9:52 PM
JonChesterfield added a comment.EditedApr 28 2021, 7:36 AM

Exciting! Thank you.

If I understand correctly, we presently have neither windows nor aarch64 working. Doing both of those at the same time sounds hazardous. Mixes a variety of failure modes.

We could enable windows on x86-64, which will hammer out things like uses of dlopen. That can probably be tested via a virtual machine running on a linux host, or maybe on Azure.

We could enable linux on aarch64, which may be almost a no-op. That can probably be tested via qemu for people who don't have an aarch64 box, or maybe on AWS.

If we can get linux/aarch64 passing tests, and windows/x64 passing tests, there's a much reduced cross section for bugs when trying windows/aarch64.

edit: there's also the host compiler split, building this with clang / msvc / other, which probably induces a different axis of bugs

We talked about this in our OpenMP in LLVM meeting (https://docs.google.com/document/d/1Tz8WFN13n7yJ-SCE0Qjqf9LmjGUw0dWO9Ts1ss4YOdg/edit?usp=sharing) today and we are looking for people and ways to test and review this.
It might be good if you could join us next Wednesday and give a short talk about the overall plan, what patches we can expect, buildbots, things like that.

@jdoerfert : yes, it's a great idea, I can join next Wednesday. From the agenda I didn't know you'd like to discuss this today. Hopefully Chris will be able to come too, but I'll let him tell for himself

@jdoerfert @natgla Sounds great, I'd also like to attend next meeting.

If I understand correctly, we presently have neither windows nor aarch64 working.

I'm confused about this statement. Does this mean that libomp is not available for Windows at all, for any architecture? I'm confused because I see libomp.dll as a part of the pre-built Windows binaries that users can download, for at least for x64 which I have installed.

@jdoerfert @natgla Sounds great, I'd also like to attend next meeting.

If I understand correctly, we presently have neither windows nor aarch64 working.

I'm confused about this statement. Does this mean that libomp is not available for Windows at all, for any architecture? I'm confused because I see libomp.dll as a part of the pre-built Windows binaries that users can download, for at least for x64 which I have installed.

That was my understanding, but not surprised to be wrong. I'm almost certain libomptarget doesn't work on windows. Libomp.dll suggests that part does build for windows, so given that information I'd then assume it passes tests too.

This patch does however read like two orthogonal sets of fixes, some for windows (which might mean msvc? Code says windows, description says msvc), some for aarch64.

I'm confused about this statement. Does this mean that libomp is not available for Windows at all, for any architecture? I'm confused because I see libomp.dll as a part of the pre-built Windows binaries that users can download, for at least for x64 which I have installed.

I think this applies to the libomptarget library.

libomp work on Windows, I used it successfully myself. When Intel donated the library, Windows was supported from the beginning. The only problem I had is that the clang driver incorrectly wants to link to libomp.lib, but only a libomp.dll.lib is built. I don't know the status of AARCH64 though.

AndreyChurbanov accepted this revision.May 7 2021, 1:01 PM

LGTM, though I cannot test Aarch64 functionality.

This revision is now accepted and ready to land.May 7 2021, 1:01 PM

Thanks. Do I need to worry about that one clang-tidy message, or do anything else before committing?

Thanks. Do I need to worry about that one clang-tidy message, or do anything else before committing?

I think clang-tidy fails to cope with tricky macros, the symbol is declared and defined for x86 and x86_64.
Looks like we can safely skip the warning.

Apologies, I don't have commit access. Can someone commit on my behalf? Thanks in advance.

This revision was landed with ongoing or failed builds.May 11 2021, 1:04 PM
This revision was automatically updated to reflect the committed changes.