Page MenuHomePhabricator

AMDGPU: Emit runtime metadata version 2 as YAML
ClosedPublic

Authored by yaxunl on Sep 28 2016, 2:29 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

yaxunl updated this revision to Diff 72901.Sep 28 2016, 2:29 PM
yaxunl retitled this revision from to AMDGPU: Emit runtime metadata version 2 as YAML.
yaxunl updated this object.
yaxunl added a subscriber: llvm-commits.
arsenm added inline comments.Oct 2 2016, 9:08 PM
lib/Target/AMDGPU/AMDGPURuntimeMDYamlMapping.cpp
169–174 ↗(On Diff #72901)

Braces

177–180 ↗(On Diff #72901)

Braces

lib/Target/AMDGPU/AMDGPURuntimeMDYamlMapping.h
28 ↗(On Diff #72901)

probably shouldn't use yaml namespace unless it's in the public yaml utilities, or is that necessary for the type traits?

test/CodeGen/AMDGPU/runtime-metadata.ll
192 ↗(On Diff #72901)

Can you use some combination of llvm utilities to extract the section and dump the yaml so you can write more readable checks for the specific fields?

yaxunl updated this revision to Diff 78053.Nov 15 2016, 12:53 PM
yaxunl marked 4 inline comments as done.

Bring the patch up to date and revised by Matt's comments.

Added support of printing AMDGPU runtime metadata to llvm-readobj and use it for testing.

Refactored target streamer to only emit runtime metadata in ELF target streamer.

yaxunl updated this revision to Diff 80102.Dec 2 2016, 11:05 AM
yaxunl edited edge metadata.

Revised by Nikolay's comments.

Moved the in-memory representation of runtime metadata to AMDGPURuntimeMetadata.h to share with runtime.
Added function Program::Metadata::fromYAML for parsing YAML string to the the in-memory representation of runtime metadata.
Added options -amdgpu-dump-rtmd for dumping the runtime metadata as YAML string and -amdgpu-check-rtmd-parser for testing the parser.

nhaustov added inline comments.Dec 5 2016, 6:11 AM
lib/Target/AMDGPU/AMDGPURuntimeMetadata.h
111 ↗(On Diff #80102)

Can 'None' be renamed to something less generic? I'm getting conflict with X11 headers define :(
/srv/dk/lnx/xfree86/7.4_64a/include/X11/X.h:120:30: error: expected identifier before numeric constant
#define None 0L /* universal null resource or null atom */

^

../../../../../../compiler/llvm/lib/Target/AMDGPU/AMDGPURuntimeMetadata.h:111:7: note: in expansion of macro 'None'

None       = 0,
^
lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMD.cpp
23 ↗(On Diff #80102)

Also needs include of "llvm/ADT/StringSwitch.h".

yaxunl updated this revision to Diff 80273.Dec 5 2016, 8:21 AM
yaxunl edited edge metadata.

Changed enum value name None to AccNone to avoid conflict with macro defined in X11 header file.
Changed some macro definitions to const variables to avoid polluting global name space.

yaxunl updated this revision to Diff 80277.Dec 5 2016, 9:06 AM
yaxunl edited edge metadata.

Add constructor of Program::Metadata for constructing from YAML string.

nhaustov accepted this revision.Dec 8 2016, 3:30 AM
nhaustov edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Dec 8 2016, 3:30 AM
SamWot added a subscriber: SamWot.Dec 8 2016, 7:41 AM

I applied this patch to my repository, but tests aren't working. It seams that llc generate note for metadata, but metadata itself is empty.

I'm currently working on adding support for metadata in assembler.

yaxunl added a comment.Dec 8 2016, 7:46 AM

I applied this patch to my repository, but tests aren't working. It seams that llc generate note for metadata, but metadata itself is empty.

I'm currently working on adding support for metadata in assembler.

The tests are passing in llvm trunk. Are you using llvm trunk with the patch?

How do you plan to represent the metadata as YAML string in assembly?

SamWot added a comment.Dec 9 2016, 2:01 AM

I applied this patch to my repository, but tests aren't working. It seams that llc generate note for metadata, but metadata itself is empty.

I'm currently working on adding support for metadata in assembler.

The tests are passing in llvm trunk. Are you using llvm trunk with the patch?

How do you plan to represent the metadata as YAML string in assembly?

Yes, I tried with llvm trunk. Did you run tests on windows or linux? For me tests are passing okay on linux but on windows they fail even with latest changes from trunk.

I want to create assembler directives .hsa_code_object_metadata/.end_hsa_code_object_metadata. Between them user can put YAML string that would be directly put to the generated note. E.g.:

.hsa_code_object_metadata
    {
        amd.MDVersion: [ 2, 0 ]
    }
.end_hsa_code_object_metadata
yaxunl added a comment.Dec 9 2016, 7:33 AM

Yes, I tried with llvm trunk. Did you run tests on windows or linux? For me tests are passing okay on linux but on windows they fail even with latest changes from trunk.

I only tested it on linux. I will try windows. Thanks.

Yes, I tried with llvm trunk. Did you run tests on windows or linux? For me tests are passing okay on linux but on windows they fail even with latest changes from trunk.

The failure on windows was due to output buffer not flushed in Program::Metadata::toYAML. The fix is to return Stream.str(). Thanks for catching this failure.

This revision was automatically updated to reflect the committed changes.
jlebar added a subscriber: jlebar.EditedDec 14 2016, 3:35 PM

The test here seems to be leaving a compiled object file at test/CodeGen/AMDGPU/runtime-metadata.o in my source tree.

I've submitted rL289742 to fix this, but please have a look to make sure that this is the right fix for your purposes.

The test here seems to be leaving a compiled object file at test/CodeGen/AMDGPU/runtime-metadata.o in my source tree.

I've submitted rL289742 to fix this, but please have a look to make sure that this is the right fix for your purposes.

Sorry my fault. I think this should work.