This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][AMDGPU] More detail in AMDGPU kernel launch info
ClosedPublic

Authored by jplehr on Feb 21 2023, 1:57 PM.

Details

Summary

Makes the info that is printed for kernel launches configurable for
different plugins. Adds all machinery to print the detailed launch
info that the current AMD plugin provides and includes e.g. register
spill counts.

The files msgpack.cpp, msgpack.def, and msgpack.h are copied from the old plugin
and are untouched. The contents of UtilitiesHSA.cpp and .h are copied together from
various files from the old plugin. The code was originally written by
Jon Chesterfield. I updated the function and type names visible to the outside, i.e.
in headers, to respect the LLVM conventions.

Diff Detail

Event Timeline

jplehr created this revision.Feb 21 2023, 1:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2023, 1:57 PM
jplehr requested review of this revision.Feb 21 2023, 1:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2023, 1:57 PM
jplehr updated this revision to Diff 499817.Feb 23 2023, 5:31 AM

Rewrite using what is available upstream to read the ELF metdata for AMDGPU.
Removes the copied-over code from the old plugin.

Some initial nits. I wonder if we could move this code somewhere common, since both llvm-readobj and here wants to use it now. I don't know much about the actual msgpack format, maybe @JonChesterfield can chime in.

openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
2230–2232

nit: remove braces.

2609–2610

nit. prefer early exit.

openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h
153–154

Probably not needed. We only support the existing llvm::object::ELF64LE alias, which is exactly what you have here. The Note alias is only used a single time so it's probably fineto just use ELF64LE::Note.

168–170

nit. no braces around single line block here and below

272
272–274

I'd guess that this should be an error. If we fail to parse the code object something is probably wrong.

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp
254–257

Thanks Joseph.
My impression about the llvm-readobj was that they have quite some machinery around the printing. However, I believe I understood that the actual printing is mainly a conversion of the MsgPackDocument into a YAML and then print that. So I think that there is not too much in common.

jplehr added inline comments.Feb 23 2023, 6:14 AM
openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h
272–274

I'm not sure if it should be an error, but if it is not a map, we should probably skip it (and maybe warn or so). I'm not familiar enough with the msgpack format.

Also, is it possible to write a test for this?

Am I right to assume you mainly asking about the code that obtains the metadata for the different kernels in the image?
Without executing a OpenMP target offload program on an AMDGPU, I see mostly one option: Construct a msgpack from a textual representation (e.g., YAML) and query that for the relevant information. This should be somewhat straight forward to do in Google Test. But, it would not automatically react to changes in the metadata produced in the backend. For that, we probably would need to compile and run something on an AMDGPU to test the output.

Am I right to assume you mainly asking about the code that obtains the metadata for the different kernels in the image?
Without executing a OpenMP target offload program on an AMDGPU, I see mostly one option: Construct a msgpack from a textual representation (e.g., YAML) and query that for the relevant information. This should be somewhat straight forward to do in Google Test. But, it would not automatically react to changes in the metadata produced in the backend. For that, we probably would need to compile and run something on an AMDGPU to test the output.

Shouldn't this get populated automatically if you compile an OpenMP program? We don't need to check. We have an existing info.c test that you could copy and make AMD only.

jhuber6 added inline comments.Feb 23 2023, 8:29 AM
openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
377

Doesn't this crash if the identifier is not present? Should it be a std::optional or something?

Am I right to assume you mainly asking about the code that obtains the metadata for the different kernels in the image?
Without executing a OpenMP target offload program on an AMDGPU, I see mostly one option: Construct a msgpack from a textual representation (e.g., YAML) and query that for the relevant information. This should be somewhat straight forward to do in Google Test. But, it would not automatically react to changes in the metadata produced in the backend. For that, we probably would need to compile and run something on an AMDGPU to test the output.

Shouldn't this get populated automatically if you compile an OpenMP program? We don't need to check. We have an existing info.c test that you could copy and make AMD only.

Yes. Apologies for me not yet being aware of the different test possibilities.
So, if we can compile / run a simple offload program to check if things get populated that would probably be even easier and account for any changes in the metadata.

jplehr added inline comments.Feb 23 2023, 8:41 AM
openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
377

Good catch. Yeah, I think this should be an Expected<T> or so, as, should we not have an entry for the identifier in the map, then we did not find info for it in the image.
This would more or less mean that there is something weird in the image AFAICT as there was no metadata for the kernel with that identifier.
So, we would return an Error in the plugin in that case.

jdoerfert added inline comments.Feb 23 2023, 11:58 AM
openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h
224

???

317

pass it in instead of copying it out.

319

no llvm:: needed, right?

jplehr updated this revision to Diff 499945.Feb 23 2023, 12:14 PM
  • Removed: curlies for single statement blocks, unncessary llvm:: qualification, leftover empty if, unneccessary using ELFT statements
  • Added check if kernel name has associated metadata and changed return type to Expected<T> to allow for error propagation
jdoerfert added inline comments.Feb 23 2023, 1:18 PM
openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
444

I doubt this should be a fatal error. We should recover instead.

openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h
202
jplehr updated this revision to Diff 500203.Feb 24 2023, 7:39 AM
jplehr marked 5 inline comments as done.

Addressed more reviewer comments and added test

LG overall, some final nits.

openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
444–445

Since we're ignoring this, it would be best to use std::optional. But just for reference, you would need to use consumeError here to actually ignore it. You could go ahead and make the KernelInfo variable an optional as well, so we don't need to bother with a zero fill or anything.

openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h
162

Nit, but these probably don't need to be their own variables since the getters are self-explanatory and used only once.

237

Default constructor should be fine here.

openmp/libomptarget/test/offloading/info.c
1

This should stay generic.

jplehr updated this revision to Diff 500821.Feb 27 2023, 8:54 AM
jplehr marked 4 inline comments as done.

Addressed nits and rebased. Please see if the changes to the test case are acceptable.

jhuber6 added inline comments.Feb 27 2023, 9:02 AM
openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
382

nit.

490

I still think this should be an optional. Then it's a direct assignment from the table lookup. The only thing you need to change is an if when you print it. I'd go ahead and just emit a secondary line for this extra info anyway. That way the first line is still generic between the plugins and this is more info.

jplehr updated this revision to Diff 500898.Feb 27 2023, 1:46 PM

Reworked implementation to always print standard info.
Introduced optional and refactored test.

jhuber6 accepted this revision.Feb 27 2023, 1:51 PM

LG, few final nits about formatting that should be addressed.

openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
2635

Comment above line.

openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h
139

these should all use the LLVM naming style.

This revision is now accepted and ready to land.Feb 27 2023, 1:51 PM
jplehr updated this revision to Diff 500908.Feb 27 2023, 2:10 PM
jplehr marked an inline comment as done.

Addresssed final nits

This revision was automatically updated to reflect the committed changes.