Page MenuHomePhabricator

[AMDGPU] Late parsed / dependent arguments for AMDGPU kernel attributes
Needs RevisionPublic

Authored by AlexVlx on Nov 9 2017, 11:31 AM.

Details

Summary

This generalises AMDGPU kernel attributes by changing their
definition to accept possibly late-parsed / dependent expressions, and
not only literal values of integral type. This enables one to use, for example,
template meta-programming or constexpr functions to generate an
attribute value, based on compile-time information such as the size of a
type or a nested compiled-time constant property. This brings parity with
launch_bounds as implemented in CUDA.

Diff Detail

Event Timeline

AlexVlx created this revision.Nov 9 2017, 11:31 AM
AlexVlx added a project: Restricted Project.Nov 9 2017, 11:34 AM
AlexVlx added a reviewer: whchung.
AlexVlx retitled this revision from Late parsed / dependent arguments for AMDGPU kernel attributes to [AMDGPU] Late parsed / dependent arguments for AMDGPU kernel attributes.Nov 9 2017, 11:38 AM
kzhuravl edited edge metadata.
kzhuravl added a subscriber: cfe-commits.

Hi Alex, can you rebase on top of trunk (I think you brought in some extra changes from hcc branch) and upload a full diff?

include/clang/Basic/Attr.td
1328

Not in trunk.

1339

Not in trunk.

1361–1370

Not in trunk.

aaron.ballman added inline comments.Nov 9 2017, 12:07 PM
include/clang/Basic/Attr.td
1329

Please ensure that the attribute documentation properly reflects the changes you've made here. At the very least, I would expect the docs to mention that these can now be in dependent contexts.

The same applies for the other attributes.

1368

Since you're changing the definition of the attribute, it would also be nice to add some documentation to AttrDocs.td for this.

lib/CodeGen/TargetInfo.cpp
7665–7666

This should be a static function rather than in an inline namespace.

7671

If there is no expression given, why should this return an APSInt for 0? This seems more like something you would assert.

lib/Sema/SemaDeclAttr.cpp
5583–5584

static function instead, please.

5586

We usually put the Sema object first in the parameter list in this file, don't we?

5587–5588

These variables should not be lower-case per our usual coding conventions.

5588

Please spell the type out explicitly.

AlexVlx updated this revision to Diff 122318.Nov 9 2017, 1:11 PM
AlexVlx updated this revision to Diff 122323.Nov 9 2017, 1:28 PM
AlexVlx marked 7 inline comments as done.
AlexVlx marked 2 inline comments as done.Nov 9 2017, 1:36 PM
AlexVlx added inline comments.
include/clang/Basic/Attr.td
1328

Done (hopefully). Apologies for that.

1339

Done (hopefully). Apologies for that.

1361–1370

Done (hopefully). Apologies for that.

lib/CodeGen/TargetInfo.cpp
7665–7666

Done.

7671

Hello Aaron, thank you for your comment. The initial premise was that it could be possible for a pass null, for cases where an user would not have specified an optional argument (e.g. for MaxWavesPerEU). This would've put the actual check further away from the actual attribute handling logic, making it somewhat tidier. However, thinking about it some more, from the specification of the attributes where this would have applied this can be done upstream in such a way as to ensure that all expressions describing the arguments passed to an attribute are non-null, so I've switched it over to follow your suggestion.

AlexVlx marked an inline comment as done.Nov 9 2017, 1:36 PM

Hi Alex, can you rebase on top of trunk (I think you brought in some extra changes from hcc branch) and upload a full diff?

Hello Konstantine - apologies about that, hopefully it's correct now. Thank you.

AlexVlx updated this revision to Diff 122327.Nov 9 2017, 1:49 PM
aaron.ballman added inline comments.Nov 9 2017, 7:33 PM
include/clang/Basic/AttrDocs.td
1502–1503

Slight wording tweak:

"A type or non-type dependent integral value..."

drop the "be they type or non-type"

Same comment applies below.

lib/CodeGen/TargetInfo.cpp
7665

The curly brace should bind to the end of the function parameter list. I'd just run the whole patch through clang-format and then fix up anything that goes wrong in the .td files.

7669

Are you expecting this to always succeed, or are you relying on the initialized value in the case where this fails?

lib/Sema/SemaDeclAttr.cpp
5473

Don't use auto here either, just use unsigned. Also i doesn't match our coding standard.

5474
  • binds to E not to Expr
5484

This still returns true even if E is null; is that intended?

AlexVlx updated this revision to Diff 123577.Nov 20 2017, 6:34 AM

Apologies for the delayed update. The new version tries to deal with two issues:

  1. the non-compliant style identified by Aaron in various spots, hopefully this is correct now;
  2. there was an issue with the existing logic, which only came up with additional testing:
    • for AMDGPUFlatWorkGroupSizeAttr and AMDGPUWavesPerEU, it is valid for the user to have passed an integer literal for the minimum and a dependent expression for the maximum;
    • in this case, iff the expression for the maximum could not be evaluated in Sema, then a spurious error case would obtain in which the minimum would signalled as greater than the maximum, which would have been pegged at 0;
    • this is fixed by having the maximum be at least equal to the minimum before any attempt of evaluating it is made - iff the actual user passed arguments to the attribute violate its invariants the error is signalled.
    • the relevant changes are on lines 5495 and 5529 in SemaDeclAttr.cpp.
aaron.ballman requested changes to this revision.Nov 27 2017, 5:52 AM

Please add test cases for this new functionality.

lib/CodeGen/TargetInfo.cpp
7665

This is still a bit off -- the & should bind to Ctx.

7665

You should add a message to your assertion in case it is triggered. e.g., assert(E && "attempting to get a constexpr int out of a null Expr"); or something along those lines.

7669

This question still applies.

lib/Sema/SemaDeclAttr.cpp
5473

Only partially fixed; the naming convention is still not being followed.

This revision now requires changes to proceed.Nov 27 2017, 5:52 AM