Page MenuHomePhabricator

[HLSL] Add groupshare address space.
ClosedPublic

Authored by python3kgae on Oct 2 2022, 11:58 PM.

Details

Summary

Added keyword, LangAS and TypeAttrbute for groupshared.

Tanslate it to LangAS with asHLSLLangAS.

Make sure it translated into address space 3 for DirectX target.

Diff Detail

Event Timeline

python3kgae created this revision.Oct 2 2022, 11:58 PM
Herald added a project: Restricted Project. · View Herald Transcript
python3kgae requested review of this revision.Oct 2 2022, 11:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2022, 11:58 PM

Fix test fail caused by max address space change.

Change Category to DocCatVariable.

At a high-level: can you make sure that functional changes all have corresponding test coverage?

clang/include/clang/Basic/Attr.td
4069

So this is being exposed as both a keyword and an attribute? Why? (No tests are using it as an attribute.)

clang/include/clang/Basic/DiagnosticSemaKinds.td
11718–11719

It looks like err_opencl_return_value_with_address_space could be modified to make this work. (The existing phrasing of 'return value' is strange and I'd be fine changing that to 'return type' instead.)

clang/lib/Basic/Targets/AMDGPU.cpp
59
83
clang/lib/Basic/Targets/DirectX.h
44
clang/lib/Basic/Targets/NVPTX.h
46
clang/lib/Basic/Targets/SPIR.h
46
76
clang/lib/Basic/Targets/TCE.h
53
clang/lib/Basic/Targets/X86.h
47
clang/lib/Parse/ParseExprCXX.cpp
1408

Do you expect to add more address spaces for HLSL? Do we want a helper function here for isHLSLQualifier() instead?

Also, there's no test coverage for this change.

clang/lib/Sema/SemaDecl.cpp
15002–15006

Why is this being done from ActOnStartOfFunctionDef() instead of CheckFunctionReturnType()?

We're missing test coverage for specifying this on a function declaration, a friend function declaration, or a member function (declaration or definition).

What about non-function situations like conversion operators? Can you write operator groupshared int() const; ?

python3kgae marked 12 inline comments as done.

Code cleanup to match comments.
Reuse opencl error.
Fix typo.

clang/include/clang/Basic/Attr.td
4069

It should be only keyword. I misunderstood what the Clang<> means here :(

clang/lib/Parse/ParseExprCXX.cpp
1408

Added isHLSLQualifier.
Maybe not other address space in a short term but will need other qualifiers for sure. :)

HLSL doesn't support lambda at this moment.
Should I just remove this and add it back when lambda is supported?

clang/lib/Sema/SemaDecl.cpp
15002–15006

Moved to ActOnFunctionDeclarator following err_opencl_return_value_with_address_space.
Also added tests.
operator overload is not supported in current version of hlsl though.

aaron.ballman added inline comments.Oct 12 2022, 11:36 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
10304–10305

I think we can drop the opencl and hlsl from the name at this point.

11718

Looks like this change can be reverted out.

clang/lib/Parse/ParseDecl.cpp
4344

It took me a while to convince myself that this was correct -- the token is consumed at the end of the while loop.

5844

Same thing here -- the end of the while loop is what consumes the token, so this is also correct.

clang/lib/Parse/ParseExprCXX.cpp
1410

And this consume is correct because the parse function doesn't consume any tokens.

clang/test/SemaHLSL/group_shared.hlsl
58

Some other Sema test cases that would be useful:

// Do we reject it on a return type in a function pointer?
groupshared void (*fp)();
// What about on parameters in a function pointer?
void (*fp2)(groupshared float);
// Trailing return types?
auto func() -> groupshared void;
// What about in a novel qualifier order?
void groupshared f();

struct S {
  // Do we reject it as a function qualifier on a member function?
  void f() groupshared;
};

// Does it impact size or alignment?
static_assert(sizeof(float) == sizeof(groupshared float));
static_assert(alignof(double) == alignof(groupshared double));

// Does it impact type identity for templates?
template <typename Ty>
struct S {
  static constexpr bool value = false;
};

template <>
struct S<groupshared float> {
  static constexpr bool value = true;
};
static_assert(!S<float>::value);
static_assert(S<groupshared float>::value);

// Can you overload based on the qualifier?
void func(float f) {}
void func(groupshared float f) {} // Error on redefinition?

(Note, I'm taking guesses on some of these test cases; if I've guessed wrong, feel free to correct me!)

I'd also expect tests to show up in ParserHLSL testing the parsing behavior:

// Do we accept novel qualifier orders?
extern groupshared float f;
extern float groupshared f; // Ok, redeclaration?

// Can we parse it on a lambda? Note, be sure to add more lambda tests with 
// more complex parsing situations to make sure you're happy with the parsing
// behavior.
[]() groupshared {};

// Can we still parse attributes written on the type?
float groupshared [[]] i = 12;
beanz added inline comments.Oct 12 2022, 3:13 PM
clang/include/clang/Basic/Attr.td
4070

Shouldn't this have let Subjects = SubjectList<[Var]>;?

I don't think we support groupshared on anything except variable declarations.

python3kgae marked 2 inline comments as done.

Add SubjectList<[Var]> and add tests.

python3kgae marked 2 inline comments as done.Oct 12 2022, 3:50 PM
python3kgae added inline comments.
clang/test/SemaHLSL/group_shared.hlsl
58

Unfortunately, things like pointer and lambda are not supported in HLSL yet.
So some tests will not work.

beanz added inline comments.Oct 12 2022, 4:20 PM
clang/test/ParserHLSL/group_shared.hlsl
12

What happens to this if you set the language standard to hlsl 202x? I setup 202x in Clang to be C++11-based since I think that is the direction 202x will take (still a lot unsure about the exact features).

python3kgae marked 2 inline comments as done.Oct 12 2022, 11:54 PM
python3kgae added inline comments.
clang/test/ParserHLSL/group_shared.hlsl
12

hlsl 202x works!
I'll need to add more tests for hlsl 202x since lambda compiles.

python3kgae marked an inline comment as done.

Add hlsl202x test for C++ 11 features like lambda.

aaron.ballman added inline comments.Oct 19 2022, 12:12 PM
clang/lib/Sema/SemaDecl.cpp
10131
clang/lib/Sema/SemaLambda.cpp
977
clang/test/ParserHLSL/group_shared_202x.hlsl
6–7

One test I think we need in SemaCXX for this is what happens when the lambda return type (or function return type) is deduced. e.g.,

extern groupshared float f;

auto func() {
  return f;
}

void other() {
  [&]() { return f; };
}

Does use of return f cause us to deduce a return type that's annotated with groupshared or does that get stripped off thanks to lvalue to rvalue conversions and so we deduce float?

Update comments and add more test cases.

python3kgae marked 3 inline comments as done.Oct 19 2022, 2:29 PM
python3kgae added inline comments.
clang/test/ParserHLSL/group_shared_202x.hlsl
6–7

groupshared is stripped because of lvalue to rvalue conversions.

And even hlsl202x not enable CXX14 :( .

aaron.ballman accepted this revision.Oct 20 2022, 4:58 AM

LGTM

clang/test/ParserHLSL/group_shared_202x.hlsl
6–7

Excellent, thank you!

This revision is now accepted and ready to land.Oct 20 2022, 4:58 AM
This revision was automatically updated to reflect the committed changes.
python3kgae marked an inline comment as done.