This is an archive of the discontinued LLVM Phabricator instance.

[openmp][amdgpu] Make plugin robust to presence of explicit implicit arguments
ClosedPublic

Authored by JonChesterfield on Nov 19 2021, 12:52 PM.

Details

Summary

OpenMP (compiler) does not currently request any implicit kernel
arguments. OpenMP (runtime) allocates and initialises a reasonable guess at
the implicit kernel arguments anyway.

This change makes the plugin check the number of explicit arguments, instead
of all arguments, and puts the pointer to hostcall buffer in both the current
location and at the offset expected when implicit arguments are added to the
metadata by D113538.

This is intended to keep things running while fixing the oversight in the
compiler (in D113538). Once that patch lands, and a following one marks
openmp kernels that use printf such that the backend emits an args element
with the right type (instead of hidden_node), the over-allocation can be
removed and the hardcoded 8*e+3 offset replaced with one read from the
.offset of the corresponding metadata element.

Diff Detail

Event Timeline

JonChesterfield requested review of this revision.Nov 19 2021, 12:52 PM
Herald added a project: Restricted Project. · View Herald Transcript
arsenm added inline comments.Nov 19 2021, 12:54 PM
openmp/libomptarget/plugins/amdgpu/impl/system.cpp
487

probably shouldn't touch this line until it's deleted

openmp/libomptarget/plugins/amdgpu/impl/system.cpp
487

i missed clang-format on a previous patch, but not on this one. Doesn't seem worth going through and putting the whitespace back in

openmp/libomptarget/plugins/amdgpu/impl/system.cpp
474

There's some evidence suggesting clang won't emit this metadata (or the printf one), it may actually emit

{
".address_space" : "global",
".offset" : 24,
".size" : 8,
".value_kind" : "hidden_none",
},

as a placeholder to indicate it doesn't know the print/hostcall thing is needed

  • use positional encoding, clang isn't setting print or hostcall metadata
JonChesterfield edited the summary of this revision. (Show Details)Nov 19 2021, 3:18 PM
JonChesterfield edited the summary of this revision. (Show Details)

This makes my assert problem go away

Win for shotgun debugging. The hostcall init changes are inert in trunk so if this got through internal testing we can land it, then your fix, then delete the handling for the current clang output.

estewart08 accepted this revision.Nov 22 2021, 2:41 PM

LGTM, passed CI testing.

This revision is now accepted and ready to land.Nov 22 2021, 2:41 PM