This is an archive of the discontinued LLVM Phabricator instance.

[nfc][libomptarget] Reorganise support header
ClosedPublic

Authored by JonChesterfield on Oct 30 2019, 7:53 PM.

Details

Summary

[nfc][libomptarget] Reorganise support header

All functions defined in support implementation are now declared in support.h
Reordered functions in support implementation to match the sequence in support.h
Added include guards to support.h
Added #include interface to support.h to provide kmp_Ident declaration
Move supporti.h to support.cu and s/INLINE/EXTERN/g
Add remaining includes to support.cu

A minor side effect is to change the name mangling of the support functions to
extern "C". If this matters another macro along the lines of INLINE/EXTERN
can be added - perhaps DEVICE as that's the obvious implementation.

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2019, 7:53 PM
jdoerfert accepted this revision.Oct 30 2019, 10:47 PM

There seem to be unrelated moves but other than that this looks good to me. Consider not moving stuff if it's not necessary and also using a different header guard name.

LGTM.

openmp/libomptarget/deviceRTLs/nvptx/src/support.h
14

The device RTL is not consistent here (__XXX and _XXX are used) and it is different from LLVM. I'd vote for LLVM style, thus OMPTARGET_SUPPORT_H

This revision is now accepted and ready to land.Oct 30 2019, 10:47 PM
JonChesterfield marked an inline comment as done.
  • Review comments, retrieve inline annotation and mangled symbols
JonChesterfield added a comment.EditedOct 31 2019, 4:45 AM

Using EXTERN means dropping the __always_inline__ annotation on the functions. That was unintentional, changed back to INLINE in the implementation.

Also introduced DEVICE as an annotation for the non-inline header functions. We don't currently restrict the symbols exposed from the library and names like PadBytes are generic enough that I can imagine them colliding with application symbols. The mangled names are known to not collide, to the extent that no-one has reported such a collision.

openmp/libomptarget/deviceRTLs/nvptx/src/support.h
14

Yeah, I'd noticed the variation. I like your suggestion, will change it.

  • revert code motion

Reverted code motion, which decreases the diff size considerably.

jdoerfert accepted this revision.Oct 31 2019, 9:21 AM

LGTM. Thx!

This revision was automatically updated to reflect the committed changes.