This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Expose flat work group size, register and wave control attributes
ClosedPublic

Authored by kzhuravl on Sep 13 2016, 9:41 AM.

Details

Summary

attribute((amdgpu_flat_work_group_size(<min>, <max>))) - request minimum and maximum flat work group size
attribute((amdgpu_waves_per_eu(<min>[, <max>]))) - request minimum and/or maximum waves per execution unit

Diff Detail

Event Timeline

kzhuravl updated this revision to Diff 71186.Sep 13 2016, 9:41 AM
kzhuravl retitled this revision from to [AMDGPU] Expose flat work group size, register and wave control attributes.
kzhuravl updated this object.
kzhuravl added reviewers: arsenm, aaron.ballman.
kzhuravl added subscribers: yaxunl, kanarayan, cfe-commits.
kzhuravl updated this revision to Diff 71311.Sep 14 2016, 3:25 AM
kzhuravl updated this object.
kzhuravl edited edge metadata.

Update docs in AttrDocs.td

kzhuravl updated this revision to Diff 71382.Sep 14 2016, 10:10 AM

Fix minor typos

aaron.ballman requested changes to this revision.Sep 14 2016, 10:46 AM
aaron.ballman edited edge metadata.
aaron.ballman added inline comments.
include/clang/Basic/Attr.td
1053–1057

This FIXME is now detached from what "this" is referring to. Can you clarify by mentioning that the Subjects list is what should be modified?

1067

Looking at the documentation, are you sure this should be a VariadicUnsignedArgument? It seems like this should be an UnsignedArgument with the optional bit set. Or can you pass multiple Max values?

include/clang/Basic/AttrDocs.td
1138

There's an extra space between "can" and "hide".

1144

Comma after "executed".

1147

An error instead of The error.

include/clang/Basic/DiagnosticSemaKinds.td
2385

I think this should say the attribute argument is invalid, rather than the attribute parameter.

lib/CodeGen/TargetInfo.cpp
6958

Should be const auto *Attr.

6967–6969

Elide braces?

6972

const auto * here as well.

6983–6985

Elide braces?

6993

const auto * here as well.

lib/Sema/SemaDeclAttr.cpp
4947

I don't think this case is required (here, or elsewhere). getArgAsExpr() already returns an Expr *.

4959–4960

Please include this text in the diagnostic rather than hard-coding it here. You can use %select to differentiate between different text snippets in the diagnostic, and the diagnostic doesn't need to continue to try to be generic. Same comment applies elsewhere.

4988

This will change if you use the optional bit rather than a variadic argument, but this silently eats attributes that have more than two arguments, which is not a good user experience.

6039–6043

This list is getting to the point where we really need to start handling this in Attr.td soon. Are you planning to work on more AMDGPU attributes in the near future?

test/SemaOpenCL/amdgpu-attrs.cl
53

I'd like to see a test that shows amdgpu_waves_per_eu with > 2 arguments to make sure it's handled appropriately.

This revision now requires changes to proceed.Sep 14 2016, 10:46 AM
kzhuravl updated this revision to Diff 71449.Sep 14 2016, 3:06 PM
kzhuravl edited edge metadata.
kzhuravl marked 15 inline comments as done.

Address review feedback

kzhuravl added inline comments.Sep 14 2016, 3:08 PM
include/clang/Basic/Attr.td
1067

You are right. Switched to UnsignedArgument since only one Max is allowed. Thanks.

lib/Sema/SemaDeclAttr.cpp
6039–6043

I agree, and yes, few more attributes will need to be added in the near future. Would it be ok if I change it to start handling in Attr.td after this change, but before other attributes are added?

aaron.ballman added inline comments.Sep 15 2016, 2:20 PM
lib/Sema/SemaDeclAttr.cpp
4967

Is it okay to supply 0, 0 as the min, max arguments?

4997

Is it okay to supply 0, 0 as the min, max arguments?

6039–6043

Yes, totally fine to be a follow-up patch. I was hoping it would look something like (we can bikeshed the name):

def SomeAttr {
  /* Blah */
}

def SomeOtherAttr {
  let RequiredCompanionAttributes = [SomeAttr];
}
kzhuravl updated this revision to Diff 71970.Sep 20 2016, 1:05 PM
kzhuravl edited edge metadata.

Mention 0, 0 case in the docs.

kzhuravl added inline comments.Sep 20 2016, 1:05 PM
lib/Sema/SemaDeclAttr.cpp
4967

Yes, I mentioned 0, 0 case in the docs.

4997

Yes, I mentioned 0, 0 case in the docs.

6039–6043

This seems like a good start. Thanks :)

aaron.ballman accepted this revision.Sep 21 2016, 6:15 AM
aaron.ballman edited edge metadata.

LGTM, thank you!

This revision is now accepted and ready to land.Sep 21 2016, 6:15 AM

Thanks for the review Aaron!

Tom, would you be able to do a final glance over?

tstellarAMD accepted this revision.Sep 22 2016, 6:41 PM
tstellarAMD edited edge metadata.

LGTM.

This revision was automatically updated to reflect the committed changes.
test/SemaOpenCL/amdgpu-attrs.cl