Page MenuHomePhabricator

[AMDGPU] Add metadata for runtime
ClosedPublic

Authored by yaxunl on Jun 29 2016, 11:13 AM.

Details

Summary

Added emitting metadata to elf for runtime.

Runtime requires certain information (metadata) about kernels to be able to execute and query them. Such information is emitted to an elf section as a key-value pair stream.

Diff Detail

Repository
rL LLVM

Event Timeline

yaxunl updated this revision to Diff 62248.Jun 29 2016, 11:13 AM
yaxunl retitled this revision from to [AMDGPU] Add metadata for OpenCL runtime.
yaxunl updated this object.
yaxunl added reviewers: tstellarAMD, arsenm.
yaxunl added a subscriber: llvm-commits.
arsenm edited edge metadata.Jun 29 2016, 12:23 PM

Needs more tests. I would like the full set of types to be tested, particularly weird ones like 3-vectors. Other non-OpenCL types like i1 or a random other bitwidth should also be tested. A few tests that use multiple arguments also would be useful

lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
46 ↗(On Diff #62248)

Should not use macro naming convention

724 ↗(On Diff #62248)

Spaces around :

739–744 ↗(On Diff #62248)

You could also switch over the TypeID

758 ↗(On Diff #62248)

This is reachable with valid IR. How about returning an empty string, or i + bitwidth?

776 ↗(On Diff #62248)

Missing space before {

777–781 ↗(On Diff #62248)

These enum should be moved to a header

831–849 ↗(On Diff #62248)

StringSwitch

888–897 ↗(On Diff #62248)

You can use a StringSwitch

901 ↗(On Diff #62248)

Extra space before (

902–911 ↗(On Diff #62248)

Another StringSwitch. Also llvm_unreachable should be avoided

925–935 ↗(On Diff #62248)

Can you move this into the Flag constructor and pass in the function?

936–937 ↗(On Diff #62248)

It seems odd to specify 0 as the address pace for no address space since it's private. -1?

942 ↗(On Diff #62248)

Use getTypeAllocSize. Make sure there are tests for 3 vectors

951–952 ↗(On Diff #62248)

Unchecked dyn_cast. Use cast or do th enull check

980–981 ↗(On Diff #62248)

Unchecked dyn_extracts

lib/Target/AMDGPU/AMDGPUAsmPrinter.h
120–122 ↗(On Diff #62248)

These should start with lowercase e

test/CodeGen/AMDGPU/kernel-attributes.ll
6 ↗(On Diff #62248)

Spaces after the :s

If you can come up with more descriptive names for the test functions that includes the argument type they test it would be helpful

yaxunl updated this revision to Diff 62432.Jun 30 2016, 3:23 PM
yaxunl edited edge metadata.
yaxunl marked 14 inline comments as done.

Revised as Matt suggested.

Needs more tests. I would like the full set of types to be tested, particularly weird ones like 3-vectors. Other non-OpenCL types like i1 or a random other bitwidth should also be tested. A few tests that use multiple arguments also would be useful

I added more tests. bool is not allowed as kernel argument, so I did not add test for bool.

lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
758 ↗(On Diff #62248)

I will return i+bitwidth

831–849 ↗(On Diff #62248)

There is else if condition not using string, and StringSwitch.Cases only allow 5 strings maximum.

936–937 ↗(On Diff #62248)

This bit fiels has only 2 bits, so the maximum value is 3, which will be the same as local addr space. This value should be ignored if the argument is not pointer, so it should not matter.

yaxunl updated this revision to Diff 62746.Jul 5 2016, 7:12 AM

Added version number. Renamed and updated test.

yaxunl updated this revision to Diff 63522.Jul 11 2016, 9:05 AM

Add kernel name to the metadata.

yaxunl updated this revision to Diff 63829.Jul 13 2016, 11:21 AM

Updated to use the new key-value pair format.

yaxunl retitled this revision from [AMDGPU] Add metadata for OpenCL runtime to [AMDGPU] Add metadata for runtime.Jul 13 2016, 11:23 AM
yaxunl updated this object.
nhaustov requested changes to this revision.Jul 14 2016, 5:58 AM
nhaustov added a reviewer: nhaustov.
nhaustov added a subscriber: nhaustov.
nhaustov added inline comments.
lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
927 ↗(On Diff #63829)

DataType enum is defined as char, but here 2 bytes are used.

lib/Target/AMDGPU/AMDGPURuntimeMetadata.h
98 ↗(On Diff #63829)

Are these OpenCL versions? Enum here seems to be overly restricting. Can this field just be a short integer?

115 ↗(On Diff #63829)

Change to ValueType?

130 ↗(On Diff #63829)

This can be now removed.

This revision now requires changes to proceed.Jul 14 2016, 5:58 AM
yaxunl added inline comments.Jul 14 2016, 8:34 AM
lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
927 ↗(On Diff #63829)

Data type is now defined as 2 bytes since 1 byte may not be enough for all runtimes.

lib/Target/AMDGPU/AMDGPURuntimeMetadata.h
98 ↗(On Diff #63829)

These version numbers follow Clang convention for OpenCL, but they could be used for other languages. We need to have an agreement between compiler and runtime what value is used for certain version. Otherwise, how runtime knows OpenCL version 1.0 is represented by value 100 instead of 10?

130 ↗(On Diff #63829)

Will do.

yaxunl added inline comments.Jul 14 2016, 8:45 AM
lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
927 ↗(On Diff #63829)

I will update the header to make it short.

yaxunl updated this revision to Diff 63991.Jul 14 2016, 9:10 AM
yaxunl edited edge metadata.
yaxunl marked an inline comment as done.

Revised by Nikolay's comments.

yaxunl updated this revision to Diff 63993.Jul 14 2016, 9:37 AM
yaxunl edited edge metadata.

Removed unused includes and fixed typos in AMDGPURuntimeMetadata.h

arsenm added inline comments.Jul 14 2016, 9:50 AM
lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
800 ↗(On Diff #63993)

Twine('u')

823–826 ↗(On Diff #63993)

dyn_cast and use casted pointer

831–849 ↗(On Diff #63993)

There's no maximum , only when trying to use all strings on a single Cases call

863 ↗(On Diff #63993)

Spaces around :

946–948 ↗(On Diff #63993)

dyn_cast to PointerType and PT->getAddressSpace()

955–957 ↗(On Diff #63993)

These can be moved such that if (auto RWGS = ..)

lib/Target/AMDGPU/AMDGPURuntimeMetadata.h
1 ↗(On Diff #63993)

Missing C++ mode comment

34 ↗(On Diff #63993)

Pointless comment

50 ↗(On Diff #63993)

global should not defined in a header

88 ↗(On Diff #63993)

Specify the signedness, uint8_t

88–93 ↗(On Diff #63993)

should we re-use dwarf language IDs + extensions or something instead of inventing a new enum? Maybe an unknown value for 0 or -1?

95 ↗(On Diff #63993)

Specify signedness and size, uint16_t

104 ↗(On Diff #63993)

Ditto for the rest of the enum types

test/CodeGen/AMDGPU/runtime-metadata.ll
718 ↗(On Diff #63993)

Are these supposed to be missing the address space?

722 ↗(On Diff #63993)

A couple more tests with pointers to pointers, struct with a pointer, vectors of pointers, and an unknown string for a builtin type name

yaxunl marked 25 inline comments as done.Jul 14 2016, 11:55 AM
yaxunl added inline comments.
lib/Target/AMDGPU/AMDGPURuntimeMetadata.h
50 ↗(On Diff #63993)

const global variable in C++ has internal linkage, so should be fine in header.

88–93 ↗(On Diff #63993)

This key is intended to indicate the required language spec supported by the runtime, not necessarily the original source language. For example, we can use an LLVM assembly to create a binary which can be executed on OpenCL 1.2 runtime, then we can add a key OpenCL_C to this binary, so that the runtime knows whether it can be executed.

Maybe these keys should be renamed as RuntimeSpec and RuntimeSpecVersion?

test/CodeGen/AMDGPU/runtime-metadata.ll
718 ↗(On Diff #63993)

No. address space is represented separately in kernel_arg_addr_space metaata.

yaxunl updated this revision to Diff 64045.Jul 14 2016, 2:26 PM
yaxunl marked 3 inline comments as done.

Revised by Matt's comments.

nhaustov accepted this revision.Jul 15 2016, 3:27 AM
nhaustov edited edge metadata.
nhaustov added inline comments.
lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
924 ↗(On Diff #64045)

Trailing whitespace

This revision is now accepted and ready to land.Jul 15 2016, 3:27 AM
This revision was automatically updated to reflect the committed changes.
vitalybuka reopened this revision.Jul 15 2016, 12:22 PM

reverted

This revision is now accepted and ready to land.Jul 15 2016, 12:22 PM
yaxunl closed this revision.Jul 29 2016, 11:06 AM

The bug has been fixed and re-committed by r275676.