This is an archive of the discontinued LLVM Phabricator instance.

Revert "Revert "[libomptarget] Move resource id functions into target specific code, implement for amdgcn""
ClosedPublic

Authored by JonChesterfield on Dec 13 2019, 6:15 PM.

Details

Summary

This reverts commit dd8a7fcdd73dd63529b81bf9f72c7529dfe99ec3.

Alexey reports undefined symbols for the new inline functions defined in target_impl.h
This does not reproduce for me for nvptx, or amdgcn, under release or debug builds.

I believe the patch is fine, based on:

  • the semantics of an inline function in C++ (the cuda INLINE functions end up as linkonce_odr in IR), which are only legal to drop if they have no uses
  • the code generated from a debug build of clang 9 does not show these undef symbols
  • the tests pass
  • the code is trivial

To progress from here I either need:

  • A tie break - someone to play the role of CI in determining whether the patch works
  • Alexey to provide sufficient information about his build for me to reproduce the failure
  • Alexey to debug why the symbols are disappearing for him and report back

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2019, 6:15 PM

One would get missing symbols if objects compiled against the previous support.h were linked to ones compiled against the files here, e.g. if one attempted an incremental build of deviceRTL despite the dependency graph changing between revisions.

One would get missing symbols if objects compiled against the previous support.h were linked to ones compiled against the files here, e.g. if one attempted an incremental build of deviceRTL despite the dependency graph changing between revisions.

Let's wait till @ABataev comes back with the system setup that caused this and the names of the symbols that were missing.

Ping. I'm blocked until this is resolved.

Ping. I'm blocked until this is resolved.

Yes, it works, sorry for the delay.

Ping. I'm blocked until this is resolved.

Yes, it works, sorry for the delay.

That's a considerable relief, thanks. Will land this.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 16 2019, 8:16 AM
This revision was automatically updated to reflect the committed changes.