Page MenuHomePhabricator

[libomptarget][WIP] Patch amdgpu DeviceRTL until it compiles
AbandonedPublic

Authored by JonChesterfield on Oct 18 2021, 7:18 AM.

Details

Reviewers
jdoerfert
Summary

Requires cmake changes in D111983, D111987

Mixture of typo fixes, stub implementations and a workaround for
runtime values for memory ordering

Diff Detail

Event Timeline

JonChesterfield requested review of this revision.Oct 18 2021, 7:18 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 18 2021, 7:18 AM
clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
255

Naming is somewhat inconsistent. I think 'amdgcn' refers to 'graphics core next', but the gfx10 cards are branded 'radeon dna'. Changing to a new library seems a relatively good point to rename it.

openmp/libomptarget/DeviceRTL/include/Synchronization.h
48

^ would like to call this 'load'

openmp/libomptarget/DeviceRTL/src/Configuration.cpp
23

What is this? Symbol is undefined on nvptx as far as I can tell. Amdgpu fails to link without a definition.

openmp/libomptarget/DeviceRTL/src/Synchronization.cpp
87

Suggestion offline is that delaying the check in clang for these builtins until template instantiation would make them usable from a function template, worthwhile usability improvement

JonChesterfield planned changes to this revision.Oct 19 2021, 12:16 AM

Needs manual rebase on trunk + clang patch to delay checking the arguments to amdgcn intrinsics until template instantiation time

openmp/libomptarget/DeviceRTL/src/Synchronization.cpp
92

The assert in clang was unrelated to amdgpu, posted a diff at D112159 with a fix.

The other atomic builtins codegen with a switch on order when passed a variable. That seems like the right thing to do for the amdgpu-specific atomic builtins.

JonChesterfield abandoned this revision.Oct 21 2021, 6:44 AM

Going to discard this and recreate as a new patch which includes the two cmake ones (D111983 and D111987)