Page MenuHomePhabricator

Enable kmpc_atomic functions for arm64
ClosedPublic

Authored by branh on Dec 1 2022, 1:14 PM.

Details

Summary

Define the same kmpc_atomic functions for arm and arm64 that are defined for x86 and x64.

Diff Detail

Event Timeline

branh created this revision.Dec 1 2022, 1:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 1:14 PM
branh requested review of this revision.Dec 1 2022, 1:14 PM
Herald added a project: Restricted Project. · View Herald Transcript
mstorsjo added inline comments.Dec 1 2022, 1:34 PM
openmp/runtime/src/kmp_atomic.h
1033

Why are these functions only declared on x86? These functions are among the exported ones. Since they aren't declared here (within an extern "C" {...} block, they end up with mangled C++ names. When linking in MSVC mode, the linker still manages to map the undecorated C symbol name to the C++ symbol and exporting that, but in mingw mode, it doesn't automatically find a matching itanium mangled symbol.

1341

The same goes for this ifdef block.

natgla added a subscriber: natgla.Dec 1 2022, 1:36 PM

Hmm, that’s a good point. However I don’t get any warnings for these when I build with Clang though…

If these are functions that are supposed to be called from user C code, then they probably don’t make much sense if they return a C-incompatible type. If they are supposed to be called from compiler generated code which handles the return type correctly anyway (or only ever accessed from C++ code), then it’s probably ok to include them and just silence that warning with suitable mechanisms.

// Martin

branh updated this revision to Diff 479777.Dec 2 2022, 4:18 PM

Updated based on Martin's feedback.

Thanks, this version builds successfully for me - but I presume this configuration produces warnings when built with MSVC?

(Btw, when uploading diffs, keep in mind that Phabricator works best if you include extra context, e.g. git diff -U999.)

mstorsjo accepted this revision.Dec 5 2022, 12:34 PM

It seems like Bran's replies via email doesn't make it into Phabricator.

Anyway...:

It produces warnings when built with MSVC, but I think you're right that this is probably okay. The warnings can be suppressed in the command line arguments for the build. It looks like they already occur in a few other places, as we've already suppressed them in our build pipeline.

Ok, with that in mind, this patch looks ok to me.

This revision is now accepted and ready to land.Dec 5 2022, 12:34 PM
branh added a reviewer: natgla.Dec 5 2022, 3:42 PM
branh added a comment.Dec 5 2022, 4:47 PM

I don't have commit permissions. If the change is good, could someone please commit it?

If not, are there any additional tests I need to run?

Thank you.

I don't have commit permissions. If the change is good, could someone please commit it?

Sure - can you state your preferred git author line for the patch, i.e. Real Name <email@address>?

branh added a comment.Dec 7 2022, 10:27 AM

Preferred git author line: Bran Hagger <brhagger@microsoft.com>

This revision was automatically updated to reflect the committed changes.