This is an archive of the discontinued LLVM Phabricator instance.

Add attributes for AMD GPU Tools
AbandonedPublic

Authored by kzhuravl-AMD on Mar 1 2016, 6:09 AM.

Details

Summary

Add following kernel function attributes for AMD GPU Tools (debugger, profiler):

  • amdgpu_tools_insert_nops - insert two nop instructions for each high level source statement
  • amdgpu_tools_num_reserved_vgpr(<num>) - reserve <num> vector registers and do not use throughout kernel execution
  • amdgpu_tools_num_reserved_sgpr(<num>) - reserve <num> scalar registers and do not use throughout kernel execution

Also add similar options that cause to insert attributes for each kernel, options take precedence

+ Updated docs, added tests

Diff Detail

Event Timeline

kzhuravl-AMD retitled this revision from to Add attributes for AMD GPU Tools.
kzhuravl-AMD updated this object.
kzhuravl-AMD added a subscriber: cfe-commits.
arsenm added inline comments.Mar 1 2016, 9:50 AM
include/clang/Basic/Attr.td
993–998

I didn't envision these as being user code facing attributes, and only the function emission would add them. Is there a use for these being used manually?

include/clang/Driver/Options.td
355–365

These are user facing options, not cc1 flags? I wouldn't expect these to be exposed to users, and they would just be implied by -g. Is there a need for this? Even if there is some need for these, I don't see anything testing for these with only -g

lib/CodeGen/CGCall.cpp
1601

I've been trying to switch to consistently using the '-' separates words conventions most function attributes use.

test/CodeGenOpenCL/amdgpu-tools-attrs.cl
2

I all of the run lines should use amdhsa triple

aaron.ballman edited edge metadata.Mar 1 2016, 12:26 PM

Some quick thoughts below.

include/clang/Basic/Attr.td
997

I don't see any code that checks whether the function is actually a kernel function, nor sema tests that diagnose if this is attached to a non-kernel function.

It may make sense to make a SubsetSubject that does the checking for you, and use that in all of these attributes.

include/clang/Basic/AttrDocs.td
943

This reads a bit strangely, can it be "debuggers" and "profilers" (plural)?

951

"supports the __attribute__" (insert the).

952

"it causes" (insert it).

954

"before the first ISA" (insert the, capitalize acronym).
"of the high level" (insert the).

955

"after the last ISA" (insert the, capitalize acronym).
"of the high level" (insert the).

959

"function in module" -> "function in the translation unit"? At least insert "the", but uncertain what you mean by "module", which I assume to be a translation unit.

Also add "the" before the command line option.

964

Same sentence comments apply to the following attributes as above.

test/SemaOpenCL/amdgpu-tools-attrs.cl
6

You can make the struct empty in these tests, but more importantly, it would be good to see a test that ensures the following diagnoses:

__attribute__((amdgpu_tools_insert_nops)) void not_a_kernel_func() {}
kzhuravl-AMD marked 10 inline comments as done.Mar 2 2016, 11:16 AM

Review Feedback

include/clang/Basic/Attr.td
993–998

Tools team expressed a desire to have the ability to specify them manually as well.

include/clang/Driver/Options.td
355–365

These should be independent of -g, user facing options. One of the use cases is profiler, where debug information is needed (-g), but no registers need to be reserved, no nops inserted.

lib/CodeGen/CGCall.cpp
1601

Do you mean use '-' instead of '_' in attribute spellings?

kzhuravl-AMD edited edge metadata.

Review Feedback - Updated diff

arsenm added inline comments.Mar 3 2016, 5:29 PM
lib/CodeGen/CGCall.cpp
1601

Yes

tstellarAMD added inline comments.Mar 7 2016, 5:05 AM
include/clang/Basic/AttrDocs.td
969–973

What does the value of this attribute mean? If I pass 16, does it reserve VGPRS 0 - 15 or
VGPRS 240 - 255 ? Can you clarify this in the documentation?

kzhuravl-AMD marked 2 inline comments as done.Mar 7 2016, 12:54 PM

Review Feedback - Updated diff

aaron.ballman added inline comments.Mar 8 2016, 12:13 PM
lib/Sema/SemaDeclAttr.cpp
5653

All of these should call D->setInvalidDecl(), similar to the above kernel attributes.

Aaron's Review Comments

kzhuravl-AMD marked an inline comment as done.Mar 8 2016, 1:45 PM
aaron.ballman accepted this revision.Mar 8 2016, 3:01 PM
aaron.ballman edited edge metadata.

Attribute side of things LGTM, but I don't have the expertise to comment on the OpenCL functionality itself.

This revision is now accepted and ready to land.Mar 8 2016, 3:01 PM
kzhuravl-AMD abandoned this revision.Mar 17 2016, 10:54 AM

After recent discussions we decided to use target specific options instead