This is an archive of the discontinued LLVM Phabricator instance.

[libomptarget] Implement target_impl for amdgcn
ClosedPublic

Authored by JonChesterfield on Nov 1 2019, 8:13 AM.

Details

Summary

[libomptarget] Implement target_impl for amdgcn

Smallest atomic addition for a new target. Implements enough of the amdgcn
specific code that some of the source files under nvptx/src could be compiled,
without modification, to run on amdgcn.

This foreshadows a work in progress patch to move said source out of nvptx/src.
Patch based on fork at https://github.com/ROCm-Developer-Tools/llvm-project

Event Timeline

JonChesterfield created this revision.Nov 1 2019, 8:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2019, 8:13 AM
ABataev added inline comments.Nov 1 2019, 8:21 AM
openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.h
23

__forceinline__?

81–82

Maybe ULL suffix instead of just L?

90

0xffffffffffffffffULL?

This is dead code, without any means of compiling it in tree. The clang patches aren't ready to go yet and will hopefully be replaced by the OpenMPIRBuilder.

I'd prefer to get the amdgcn deviceRTL in tree, preferably as incremental patches, so that I can use it while bringing up clang. It's not essential though. It's certainly reasonable to reject this patch as premature.

JonChesterfield marked 3 inline comments as done.
  • Prefer uint64_c to trailing suffix
JonChesterfield added inline comments.Nov 1 2019, 8:41 AM
openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.h
23

inline is sufficient when building with clang. It's possible the nvptx version should have an #ifdef that chooses between the two based on clang vs nvcc.

81–82

Nice spot. Don't really want the sign conversions here, fixed. Although to use UINT64_C instead of the suffix.

Strictly speaking that should probably be 32u (or UINT32_C(32), but that feels like a step too far) as well. Thoughts?

90

I'd prefer to use the UINT64_C notation. It's longer but it's harder to get wrong.

This revision is now accepted and ready to land.Nov 1 2019, 8:43 AM
This revision was automatically updated to reflect the committed changes.

I love it when a plan comes together