[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae added a comment.

In D129883#3806552 , @kazu wrote:

> In D129883#3806507 , @python3kgae 
> wrote:
>
>> In D129883#3806393 , @kazu wrote:
>>
>>> This patch seems to cause several warnings:
>>>
>>>   clang/include/clang/AST/Decl.h:4696:15: error: 'getSourceRange' overrides 
>>> a member function but is not marked 'override' [-Werror,-Wsuggest-override]
>>>   clang/lib/Parse/ParseHLSL.cpp:76:20: error: unused variable 'Loc' 
>>> [-Werror,-Wunused-variable]
>>>   clang/lib/CodeGen/CGDecl.cpp:48:11: error: enumeration value 'HLSLBuffer' 
>>> not handled in switch [-Werror,-Wswitch]
>>>
>>> I could fix the first two, but I don't know what should be done for the 
>>> last one.  Would you mind taking a look?
>>>
>>> FWIW, I'm using clang-14.0.0.6 as the hot compiler with a cmake flag 
>>> `-DLLVM_ENABLE_WERROR=On`.
>>
>> Fixed with 
>> https://github.com/llvm/llvm-project/commit/a7e3de2450f5b62e7dfe8999443f15be5dfec0b0
>> Thanks for the help.
>> I will switch to clang in the future.
>
> Thank you so much for fixing this quickly!
>
> By the way, after your fix, I am getting:
>
>   clang/tools/libclang/CIndex.cpp:6648:11: error: enumeration value 
> 'HLSLBuffer' not handled in switch [-Werror,-Wswitch]
>
> Sorry for not listing all the warnings in my first message.  (I thought I 
> listed them all...) Should we add something like this?
>
>   case Decl::HLSLBuffer:
> return clang_getNullCursor();

Yeah. return clang_getNullCursor(); looks fine.
I'm running a full build now. Hope it can catch all the issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Kazu Hirata via Phabricator via cfe-commits]

kazu added a comment.

In D129883#3806507 , @python3kgae 
wrote:

> In D129883#3806393 , @kazu wrote:
>
>> This patch seems to cause several warnings:
>>
>>   clang/include/clang/AST/Decl.h:4696:15: error: 'getSourceRange' overrides 
>> a member function but is not marked 'override' [-Werror,-Wsuggest-override]
>>   clang/lib/Parse/ParseHLSL.cpp:76:20: error: unused variable 'Loc' 
>> [-Werror,-Wunused-variable]
>>   clang/lib/CodeGen/CGDecl.cpp:48:11: error: enumeration value 'HLSLBuffer' 
>> not handled in switch [-Werror,-Wswitch]
>>
>> I could fix the first two, but I don't know what should be done for the last 
>> one.  Would you mind taking a look?
>>
>> FWIW, I'm using clang-14.0.0.6 as the hot compiler with a cmake flag 
>> `-DLLVM_ENABLE_WERROR=On`.
>
> Fixed with 
> https://github.com/llvm/llvm-project/commit/a7e3de2450f5b62e7dfe8999443f15be5dfec0b0
> Thanks for the help.
> I will switch to clang in the future.

Thank you so much for fixing this quickly!

By the way, after your fix, I am getting:

  clang/tools/libclang/CIndex.cpp:6648:11: error: enumeration value 
'HLSLBuffer' not handled in switch [-Werror,-Wswitch]

Sorry for not listing all the warnings in my first message.  (I thought I 
listed them all...) Should we add something like this?

  case Decl::HLSLBuffer:
return clang_getNullCursor();


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae added a comment.

In D129883#3806393 , @kazu wrote:

> This patch seems to cause several warnings:
>
>   clang/include/clang/AST/Decl.h:4696:15: error: 'getSourceRange' overrides a 
> member function but is not marked 'override' [-Werror,-Wsuggest-override]
>   clang/lib/Parse/ParseHLSL.cpp:76:20: error: unused variable 'Loc' 
> [-Werror,-Wunused-variable]
>   clang/lib/CodeGen/CGDecl.cpp:48:11: error: enumeration value 'HLSLBuffer' 
> not handled in switch [-Werror,-Wswitch]
>
> I could fix the first two, but I don't know what should be done for the last 
> one.  Would you mind taking a look?
>
> FWIW, I'm using clang-14.0.0.6 as the hot compiler with a cmake flag 
> `-DLLVM_ENABLE_WERROR=On`.

Fixed with 
https://github.com/llvm/llvm-project/commit/a7e3de2450f5b62e7dfe8999443f15be5dfec0b0
Thanks for the help.
I will switch to clang in the future.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae added a comment.

In D129883#3806393 , @kazu wrote:

> This patch seems to cause several warnings:
>
>   clang/include/clang/AST/Decl.h:4696:15: error: 'getSourceRange' overrides a 
> member function but is not marked 'override' [-Werror,-Wsuggest-override]
>   clang/lib/Parse/ParseHLSL.cpp:76:20: error: unused variable 'Loc' 
> [-Werror,-Wunused-variable]
>   clang/lib/CodeGen/CGDecl.cpp:48:11: error: enumeration value 'HLSLBuffer' 
> not handled in switch [-Werror,-Wswitch]
>
> I could fix the first two, but I don't know what should be done for the last 
> one.  Would you mind taking a look?
>
> FWIW, I'm using clang-14.0.0.6 as the hot compiler with a cmake flag 
> `-DLLVM_ENABLE_WERROR=On`.

Working on it.
Building with clang now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Kazu Hirata via Phabricator via cfe-commits]

kazu added a comment.

This patch seems to cause several warnings:

  clang/include/clang/AST/Decl.h:4696:15: error: 'getSourceRange' overrides a 
member function but is not marked 'override' [-Werror,-Wsuggest-override]
  clang/lib/Parse/ParseHLSL.cpp:76:20: error: unused variable 'Loc' 
[-Werror,-Wunused-variable]
  clang/lib/CodeGen/CGDecl.cpp:48:11: error: enumeration value 'HLSLBuffer' not 
handled in switch [-Werror,-Wswitch]

I could fix the first two, but I don't know what should be done for the last 
one.  Would you mind taking a look?

FWIW, I'm using clang-14.0.0.6 as the hot compiler with a cmake flag 
`-DLLVM_ENABLE_WERROR=On`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG782ac2182c2b: [HLSL] Support cbuffer/tbuffer for hlsl. 
(authored by python3kgae).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/JSONNodeDumper.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/include/clang/Basic/DeclNodes.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Basic/IdentifierTable.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/IdentifierResolver.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaHLSL.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/ASTCommon.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/AST/HLSL/Inputs/empty.hlsl
  clang/test/AST/HLSL/ast-dump-comment-cbuffe-tbufferr.hlsl
  clang/test/AST/HLSL/cbuffer_tbuffer.hlsl
  clang/test/AST/HLSL/pch_hlsl_buffer.hlsl
  clang/test/ParserHLSL/cb_error.hlsl
  clang/test/ParserHLSL/invalid_inside_cb.hlsl
  clang/test/SemaHLSL/cb_error.hlsl

Index: clang/test/SemaHLSL/cb_error.hlsl
===
--- /dev/null
+++ clang/test/SemaHLSL/cb_error.hlsl
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// expected-note@+1 {{declared here}}
+cbuffer a {
+  int x;
+};
+
+int foo() {
+  // expected-error@+1 {{'a' does not refer to a value}}
+  return sizeof(a);
+}
+
+// expected-error@+1 {{expected unqualified-id}}
+template  cbuffer a { Ty f; };
+
+// For back-compat reason, it is OK for multiple cbuffer/tbuffer use same name in hlsl.
+// And these cbuffer name only used for reflection, cannot be removed.
+cbuffer A {
+  float A;
+}
+
+cbuffer A {
+  float b;
+}
+
+tbuffer A {
+  float a;
+}
+
+float bar() {
+  // cbuffer/tbuffer name will not conflict with other variables.
+  return A;
+}
+
+cbuffer a {
+  // expected-error@+2 {{unknown type name 'oh'}}
+  // expected-error@+1 {{expected ';' after top level declarator}}
+  oh no!
+  // expected-warning@+1 {{missing terminating ' character}}
+  this isn't even valid HLSL code
+  despite seeming totally reasonable
+  once you understand that HLSL
+  is so flaming weird.
+}
+
+tbuffer B {
+  // expected-error@+1 {{unknown type name 'flaot'}}
+  flaot f;
+}
Index: clang/test/ParserHLSL/invalid_inside_cb.hlsl
===
--- /dev/null
+++ clang/test/ParserHLSL/invalid_inside_cb.hlsl
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// template not allowed inside cbuffer.
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+template
+T foo(T t) { return t;}
+}
+
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+template
+struct S { float s;};
+}
+
+// typealias not allowed inside cbuffer.
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+// expected-warning@+1 {{alias declarations are a C++11 extension}}
+using F32 = float;
+}
Index: clang/test/ParserHLSL/cb_error.hlsl
===
--- /dev/null
+++ clang/test/ParserHLSL/cb_error.hlsl
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+cbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+cbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int cbuffer;
+// expected-error@+1 {{expected identifier}}
+cbuffer;
+
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+tbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+tbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int tbuffer;
+// expected-error@+1 {{expected identifier}}
+tbuffer;
+
+// expected-error@+1 {{expected unqualified-id}}
+cbuffer A {}, B{}
+
+// cbuffer inside namespace is supported.
+namespace N {
+  cbuffer A {
+float g;
+  }
+}
+
+cbuffer A {
+  // expected-error@+1 {{invalid 

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae updated this revision to Diff 461751.
python3kgae added a comment.

Rebase for the PCH fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/JSONNodeDumper.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/include/clang/Basic/DeclNodes.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Basic/IdentifierTable.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/IdentifierResolver.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaHLSL.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/ASTCommon.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/AST/HLSL/Inputs/empty.hlsl
  clang/test/AST/HLSL/ast-dump-comment-cbuffe-tbufferr.hlsl
  clang/test/AST/HLSL/cbuffer_tbuffer.hlsl
  clang/test/AST/HLSL/pch_hlsl_buffer.hlsl
  clang/test/ParserHLSL/cb_error.hlsl
  clang/test/ParserHLSL/invalid_inside_cb.hlsl
  clang/test/SemaHLSL/cb_error.hlsl

Index: clang/test/SemaHLSL/cb_error.hlsl
===
--- /dev/null
+++ clang/test/SemaHLSL/cb_error.hlsl
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// expected-note@+1 {{declared here}}
+cbuffer a {
+  int x;
+};
+
+int foo() {
+  // expected-error@+1 {{'a' does not refer to a value}}
+  return sizeof(a);
+}
+
+// expected-error@+1 {{expected unqualified-id}}
+template  cbuffer a { Ty f; };
+
+// For back-compat reason, it is OK for multiple cbuffer/tbuffer use same name in hlsl.
+// And these cbuffer name only used for reflection, cannot be removed.
+cbuffer A {
+  float A;
+}
+
+cbuffer A {
+  float b;
+}
+
+tbuffer A {
+  float a;
+}
+
+float bar() {
+  // cbuffer/tbuffer name will not conflict with other variables.
+  return A;
+}
+
+cbuffer a {
+  // expected-error@+2 {{unknown type name 'oh'}}
+  // expected-error@+1 {{expected ';' after top level declarator}}
+  oh no!
+  // expected-warning@+1 {{missing terminating ' character}}
+  this isn't even valid HLSL code
+  despite seeming totally reasonable
+  once you understand that HLSL
+  is so flaming weird.
+}
+
+tbuffer B {
+  // expected-error@+1 {{unknown type name 'flaot'}}
+  flaot f;
+}
Index: clang/test/ParserHLSL/invalid_inside_cb.hlsl
===
--- /dev/null
+++ clang/test/ParserHLSL/invalid_inside_cb.hlsl
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// template not allowed inside cbuffer.
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+template
+T foo(T t) { return t;}
+}
+
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+template
+struct S { float s;};
+}
+
+// typealias not allowed inside cbuffer.
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+// expected-warning@+1 {{alias declarations are a C++11 extension}}
+using F32 = float;
+}
Index: clang/test/ParserHLSL/cb_error.hlsl
===
--- /dev/null
+++ clang/test/ParserHLSL/cb_error.hlsl
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+cbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+cbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int cbuffer;
+// expected-error@+1 {{expected identifier}}
+cbuffer;
+
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+tbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+tbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int tbuffer;
+// expected-error@+1 {{expected identifier}}
+tbuffer;
+
+// expected-error@+1 {{expected unqualified-id}}
+cbuffer A {}, B{}
+
+// cbuffer inside namespace is supported.
+namespace N {
+  cbuffer A {
+float g;
+  }
+}
+
+cbuffer A {
+  // expected-error@+1 {{invalid declaration inside cbuffer}}
+  namespace N {
+  }
+}
+
+cbuffer A {
+  // expected-error@+1 {{invalid declaration inside cbuffer}}
+ 

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae added inline comments.



Comment at: clang/lib/Parse/ParseHLSL.cpp:79-80
+ParsedAttributes Attrs(AttrFactory);
+MaybeParseCXX11Attributes(Attrs);
+MaybeParseMicrosoftAttributes(Attrs);
+

beanz wrote:
> aaron.ballman wrote:
> > beanz wrote:
> > > aaron.ballman wrote:
> > > > beanz wrote:
> > > > > python3kgae wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Just double-checking, but this allows `[[]]` style attributes as 
> > > > > > > well as `[]` style attributes, but not `__attribute__` or 
> > > > > > > `__declspec` style attributes, is that intended?
> > > > > > That is what dxc currently support.
> > > > > > It may change in the future. But for now, only [[]] and [] are 
> > > > > > supported.
> > > > > Well... worth noting, HLSL doesn't actually support C++11 attributes, 
> > > > > but that is almost certainly going to change in the near future, so 
> > > > > we might as well support them from the start in Clang.
> > > > Ah, that is good to know @beanz -- we should think carefully about this 
> > > > situation because there are some tradeoffs to consider.
> > > > 
> > > > 1) It's pretty weird to support half of the Microsoft attribute syntax 
> > > > (and the half we barely have any attribute support for, at that). Is 
> > > > there a reason to not support `__declspec` as well? (For example, are 
> > > > there concerns about things like using those attributes to do dllexport 
> > > > or specify a COMDAT section, etc?) In fact, if we're going to support 
> > > > vendor attributes like `[[clang::overloadable]]`, it's a bit weird that 
> > > > we then prohibit the same attribute from being spelled 
> > > > `__attribute__((overloadable))`, is there a particular reason to not 
> > > > extend to all attributes?
> > > > 2) Supporting `[]` style attributes means that attribute order is 
> > > > important. We cannot use `MaybeParseAttributes()` to parse attribute 
> > > > specifiers in any order because `[]` causes ambiguities under some 
> > > > circumstances. So you're stuck with the order you have -- `[[]]` 
> > > > attributes first, `[]` attributes second. Is that ordering reasonable?
> > > > 
> > > > And for the patch itself -- there are no test cases demonstrating what 
> > > > happens when using attributes on things within one of these buffers. I 
> > > > expect many things to be quite reasonable, like using `[[deprecated]]`, 
> > > > but are the attributes which impact codegen reasonable as well? (Like 
> > > > naked functions, returns twice, disable tail calls, etc)
> > > @aaron.ballman I think those are all good questions. Generally HLSL has 
> > > used Microsoft attribute syntax, and I've started extending the Clang 
> > > support to be more robust, but still have more work to do.
> > > 
> > > More on this patch, I want to take a step back.
> > > 
> > > I think @python3kgae copied this code from DXC, but I don't think it is 
> > > ever used. I don't think we have any attributes in the language that are 
> > > valid with cbuffer or tbuffer  subjects. We certainly don't have any 
> > > attributes implemented in clang that would be valid on these targets.
> > > 
> > > That makes me think we should remove since it should be dead and 
> > > unreachable and untestable code.
> > > 
> > > Since these HLSL buffer decls are an older (although frequently used) 
> > > HLSL feature, I think our general preference is to not extend new feature 
> > > support to them, and instead to encourage users to use the newer buffer 
> > > types.
> > > 
> > > Does that sound reasonable?
> > > We certainly don't have any attributes implemented in clang that would be 
> > > valid on these targets.
> > 
> > Despite knowing nothing about HLSL, I feel like pushing back a little bit 
> > here: deprecated, nodiscard, maybe_unused, and many others seem like they'd 
> > not only be valid on the target but perhaps useful to users.
> > 
> > > Does that sound reasonable?
> > 
> > I'm totally fine with that approach; we can debate attributes later. :-)
> > Despite knowing nothing about HLSL, I feel like pushing back a little bit 
> > here: deprecated, nodiscard, maybe_unused, and many others seem like they'd 
> > not only be valid on the target but perhaps useful to users.
> 
> Okay... you got me here. I hadn't considered `deprecated` but can see a use 
> for it. I don't think the other two apply, but I'll concede there may be more 
> general clang attributes that do have uses.
> 
> If we can postpone this discussion though I think we can do some background 
> and get a better feeling for what attributes we should and shouldn't support, 
> and maybe consider the syntax a bit carefully too.
> 
> If I'm reading this correctly the DXC-supported syntax is:
> 
> ```
> cbuffer A { ... } [some_attribute]
> ```
> 
> (note: DXC doesn't really support CXX11 attributes, just the MS syntax)
> 
> If this syntax is really unreachable in DXC (which I believe it is), it might 
> be better 

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae updated this revision to Diff 459090.
python3kgae added a comment.

Remove attribute for first cbuffer commit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/JSONNodeDumper.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/include/clang/Basic/DeclNodes.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Basic/IdentifierTable.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/IdentifierResolver.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaHLSL.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/ASTCommon.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/AST/HLSL/Inputs/empty.hlsl
  clang/test/AST/HLSL/ast-dump-comment-cbuffe-tbufferr.hlsl
  clang/test/AST/HLSL/cbuffer_tbuffer.hlsl
  clang/test/AST/HLSL/pch_hlsl_buffer.hlsl
  clang/test/ParserHLSL/cb_error.hlsl
  clang/test/ParserHLSL/invalid_inside_cb.hlsl
  clang/test/SemaHLSL/cb_error.hlsl

Index: clang/test/SemaHLSL/cb_error.hlsl
===
--- /dev/null
+++ clang/test/SemaHLSL/cb_error.hlsl
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// expected-note@+1 {{declared here}}
+cbuffer a {
+  int x;
+};
+
+int foo() {
+  // expected-error@+1 {{'a' does not refer to a value}}
+  return sizeof(a);
+}
+
+// expected-error@+1 {{expected unqualified-id}}
+template  cbuffer a { Ty f; };
+
+// For back-compat reason, it is OK for multiple cbuffer/tbuffer use same name in hlsl.
+// And these cbuffer name only used for reflection, cannot be removed.
+cbuffer A {
+  float A;
+}
+
+cbuffer A {
+  float b;
+}
+
+tbuffer A {
+  float a;
+}
+
+float bar() {
+  // cbuffer/tbuffer name will not conflict with other variables.
+  return A;
+}
+
+cbuffer a {
+  // expected-error@+2 {{unknown type name 'oh'}}
+  // expected-error@+1 {{expected ';' after top level declarator}}
+  oh no!
+  // expected-warning@+1 {{missing terminating ' character}}
+  this isn't even valid HLSL code
+  despite seeming totally reasonable
+  once you understand that HLSL
+  is so flaming weird.
+}
+
+tbuffer B {
+  // expected-error@+1 {{unknown type name 'flaot'}}
+  flaot f;
+}
Index: clang/test/ParserHLSL/invalid_inside_cb.hlsl
===
--- /dev/null
+++ clang/test/ParserHLSL/invalid_inside_cb.hlsl
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// template not allowed inside cbuffer.
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+template
+T foo(T t) { return t;}
+}
+
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+template
+struct S { float s;};
+}
+
+// typealias not allowed inside cbuffer.
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+// expected-warning@+1 {{alias declarations are a C++11 extension}}
+using F32 = float;
+}
Index: clang/test/ParserHLSL/cb_error.hlsl
===
--- /dev/null
+++ clang/test/ParserHLSL/cb_error.hlsl
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+cbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+cbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int cbuffer;
+// expected-error@+1 {{expected identifier}}
+cbuffer;
+
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+tbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+tbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int tbuffer;
+// expected-error@+1 {{expected identifier}}
+tbuffer;
+
+// expected-error@+1 {{expected unqualified-id}}
+cbuffer A {}, B{}
+
+// cbuffer inside namespace is supported.
+namespace N {
+  cbuffer A {
+float g;
+  }
+}
+
+cbuffer A {
+  // expected-error@+1 {{invalid declaration inside cbuffer}}
+  namespace N {
+  }
+}
+
+cbuffer A {
+  // 

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.



Comment at: clang/lib/Parse/ParseHLSL.cpp:79-80
+ParsedAttributes Attrs(AttrFactory);
+MaybeParseCXX11Attributes(Attrs);
+MaybeParseMicrosoftAttributes(Attrs);
+

aaron.ballman wrote:
> beanz wrote:
> > aaron.ballman wrote:
> > > beanz wrote:
> > > > python3kgae wrote:
> > > > > aaron.ballman wrote:
> > > > > > Just double-checking, but this allows `[[]]` style attributes as 
> > > > > > well as `[]` style attributes, but not `__attribute__` or 
> > > > > > `__declspec` style attributes, is that intended?
> > > > > That is what dxc currently support.
> > > > > It may change in the future. But for now, only [[]] and [] are 
> > > > > supported.
> > > > Well... worth noting, HLSL doesn't actually support C++11 attributes, 
> > > > but that is almost certainly going to change in the near future, so we 
> > > > might as well support them from the start in Clang.
> > > Ah, that is good to know @beanz -- we should think carefully about this 
> > > situation because there are some tradeoffs to consider.
> > > 
> > > 1) It's pretty weird to support half of the Microsoft attribute syntax 
> > > (and the half we barely have any attribute support for, at that). Is 
> > > there a reason to not support `__declspec` as well? (For example, are 
> > > there concerns about things like using those attributes to do dllexport 
> > > or specify a COMDAT section, etc?) In fact, if we're going to support 
> > > vendor attributes like `[[clang::overloadable]]`, it's a bit weird that 
> > > we then prohibit the same attribute from being spelled 
> > > `__attribute__((overloadable))`, is there a particular reason to not 
> > > extend to all attributes?
> > > 2) Supporting `[]` style attributes means that attribute order is 
> > > important. We cannot use `MaybeParseAttributes()` to parse attribute 
> > > specifiers in any order because `[]` causes ambiguities under some 
> > > circumstances. So you're stuck with the order you have -- `[[]]` 
> > > attributes first, `[]` attributes second. Is that ordering reasonable?
> > > 
> > > And for the patch itself -- there are no test cases demonstrating what 
> > > happens when using attributes on things within one of these buffers. I 
> > > expect many things to be quite reasonable, like using `[[deprecated]]`, 
> > > but are the attributes which impact codegen reasonable as well? (Like 
> > > naked functions, returns twice, disable tail calls, etc)
> > @aaron.ballman I think those are all good questions. Generally HLSL has 
> > used Microsoft attribute syntax, and I've started extending the Clang 
> > support to be more robust, but still have more work to do.
> > 
> > More on this patch, I want to take a step back.
> > 
> > I think @python3kgae copied this code from DXC, but I don't think it is 
> > ever used. I don't think we have any attributes in the language that are 
> > valid with cbuffer or tbuffer  subjects. We certainly don't have any 
> > attributes implemented in clang that would be valid on these targets.
> > 
> > That makes me think we should remove since it should be dead and 
> > unreachable and untestable code.
> > 
> > Since these HLSL buffer decls are an older (although frequently used) HLSL 
> > feature, I think our general preference is to not extend new feature 
> > support to them, and instead to encourage users to use the newer buffer 
> > types.
> > 
> > Does that sound reasonable?
> > We certainly don't have any attributes implemented in clang that would be 
> > valid on these targets.
> 
> Despite knowing nothing about HLSL, I feel like pushing back a little bit 
> here: deprecated, nodiscard, maybe_unused, and many others seem like they'd 
> not only be valid on the target but perhaps useful to users.
> 
> > Does that sound reasonable?
> 
> I'm totally fine with that approach; we can debate attributes later. :-)
> Despite knowing nothing about HLSL, I feel like pushing back a little bit 
> here: deprecated, nodiscard, maybe_unused, and many others seem like they'd 
> not only be valid on the target but perhaps useful to users.

Okay... you got me here. I hadn't considered `deprecated` but can see a use for 
it. I don't think the other two apply, but I'll concede there may be more 
general clang attributes that do have uses.

If we can postpone this discussion though I think we can do some background and 
get a better feeling for what attributes we should and shouldn't support, and 
maybe consider the syntax a bit carefully too.

If I'm reading this correctly the DXC-supported syntax is:

```
cbuffer A { ... } [some_attribute]
```

(note: DXC doesn't really support CXX11 attributes, just the MS syntax)

If this syntax is really unreachable in DXC (which I believe it is), it might 
be better to shift the syntax to be more like C++ class and struct attributes:

```
[[some_attribute]]
cbuffer A {...}
```

I think that would be more familiar and understandable to users, especially as 
buffer 

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Parse/ParseHLSL.cpp:79-80
+ParsedAttributes Attrs(AttrFactory);
+MaybeParseCXX11Attributes(Attrs);
+MaybeParseMicrosoftAttributes(Attrs);
+

beanz wrote:
> aaron.ballman wrote:
> > beanz wrote:
> > > python3kgae wrote:
> > > > aaron.ballman wrote:
> > > > > Just double-checking, but this allows `[[]]` style attributes as well 
> > > > > as `[]` style attributes, but not `__attribute__` or `__declspec` 
> > > > > style attributes, is that intended?
> > > > That is what dxc currently support.
> > > > It may change in the future. But for now, only [[]] and [] are 
> > > > supported.
> > > Well... worth noting, HLSL doesn't actually support C++11 attributes, but 
> > > that is almost certainly going to change in the near future, so we might 
> > > as well support them from the start in Clang.
> > Ah, that is good to know @beanz -- we should think carefully about this 
> > situation because there are some tradeoffs to consider.
> > 
> > 1) It's pretty weird to support half of the Microsoft attribute syntax (and 
> > the half we barely have any attribute support for, at that). Is there a 
> > reason to not support `__declspec` as well? (For example, are there 
> > concerns about things like using those attributes to do dllexport or 
> > specify a COMDAT section, etc?) In fact, if we're going to support vendor 
> > attributes like `[[clang::overloadable]]`, it's a bit weird that we then 
> > prohibit the same attribute from being spelled 
> > `__attribute__((overloadable))`, is there a particular reason to not extend 
> > to all attributes?
> > 2) Supporting `[]` style attributes means that attribute order is 
> > important. We cannot use `MaybeParseAttributes()` to parse attribute 
> > specifiers in any order because `[]` causes ambiguities under some 
> > circumstances. So you're stuck with the order you have -- `[[]]` attributes 
> > first, `[]` attributes second. Is that ordering reasonable?
> > 
> > And for the patch itself -- there are no test cases demonstrating what 
> > happens when using attributes on things within one of these buffers. I 
> > expect many things to be quite reasonable, like using `[[deprecated]]`, but 
> > are the attributes which impact codegen reasonable as well? (Like naked 
> > functions, returns twice, disable tail calls, etc)
> @aaron.ballman I think those are all good questions. Generally HLSL has used 
> Microsoft attribute syntax, and I've started extending the Clang support to 
> be more robust, but still have more work to do.
> 
> More on this patch, I want to take a step back.
> 
> I think @python3kgae copied this code from DXC, but I don't think it is ever 
> used. I don't think we have any attributes in the language that are valid 
> with cbuffer or tbuffer  subjects. We certainly don't have any attributes 
> implemented in clang that would be valid on these targets.
> 
> That makes me think we should remove since it should be dead and unreachable 
> and untestable code.
> 
> Since these HLSL buffer decls are an older (although frequently used) HLSL 
> feature, I think our general preference is to not extend new feature support 
> to them, and instead to encourage users to use the newer buffer types.
> 
> Does that sound reasonable?
> We certainly don't have any attributes implemented in clang that would be 
> valid on these targets.

Despite knowing nothing about HLSL, I feel like pushing back a little bit here: 
deprecated, nodiscard, maybe_unused, and many others seem like they'd not only 
be valid on the target but perhaps useful to users.

> Does that sound reasonable?

I'm totally fine with that approach; we can debate attributes later. :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.



Comment at: clang/lib/Parse/ParseHLSL.cpp:79-80
+ParsedAttributes Attrs(AttrFactory);
+MaybeParseCXX11Attributes(Attrs);
+MaybeParseMicrosoftAttributes(Attrs);
+

aaron.ballman wrote:
> beanz wrote:
> > python3kgae wrote:
> > > aaron.ballman wrote:
> > > > Just double-checking, but this allows `[[]]` style attributes as well 
> > > > as `[]` style attributes, but not `__attribute__` or `__declspec` style 
> > > > attributes, is that intended?
> > > That is what dxc currently support.
> > > It may change in the future. But for now, only [[]] and [] are supported.
> > Well... worth noting, HLSL doesn't actually support C++11 attributes, but 
> > that is almost certainly going to change in the near future, so we might as 
> > well support them from the start in Clang.
> Ah, that is good to know @beanz -- we should think carefully about this 
> situation because there are some tradeoffs to consider.
> 
> 1) It's pretty weird to support half of the Microsoft attribute syntax (and 
> the half we barely have any attribute support for, at that). Is there a 
> reason to not support `__declspec` as well? (For example, are there concerns 
> about things like using those attributes to do dllexport or specify a COMDAT 
> section, etc?) In fact, if we're going to support vendor attributes like 
> `[[clang::overloadable]]`, it's a bit weird that we then prohibit the same 
> attribute from being spelled `__attribute__((overloadable))`, is there a 
> particular reason to not extend to all attributes?
> 2) Supporting `[]` style attributes means that attribute order is important. 
> We cannot use `MaybeParseAttributes()` to parse attribute specifiers in any 
> order because `[]` causes ambiguities under some circumstances. So you're 
> stuck with the order you have -- `[[]]` attributes first, `[]` attributes 
> second. Is that ordering reasonable?
> 
> And for the patch itself -- there are no test cases demonstrating what 
> happens when using attributes on things within one of these buffers. I expect 
> many things to be quite reasonable, like using `[[deprecated]]`, but are the 
> attributes which impact codegen reasonable as well? (Like naked functions, 
> returns twice, disable tail calls, etc)
@aaron.ballman I think those are all good questions. Generally HLSL has used 
Microsoft attribute syntax, and I've started extending the Clang support to be 
more robust, but still have more work to do.

More on this patch, I want to take a step back.

I think @python3kgae copied this code from DXC, but I don't think it is ever 
used. I don't think we have any attributes in the language that are valid with 
cbuffer or tbuffer  subjects. We certainly don't have any attributes 
implemented in clang that would be valid on these targets.

That makes me think we should remove since it should be dead and unreachable 
and untestable code.

Since these HLSL buffer decls are an older (although frequently used) HLSL 
feature, I think our general preference is to not extend new feature support to 
them, and instead to encourage users to use the newer buffer types.

Does that sound reasonable?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Parse/ParseHLSL.cpp:79-80
+ParsedAttributes Attrs(AttrFactory);
+MaybeParseCXX11Attributes(Attrs);
+MaybeParseMicrosoftAttributes(Attrs);
+

beanz wrote:
> python3kgae wrote:
> > aaron.ballman wrote:
> > > Just double-checking, but this allows `[[]]` style attributes as well as 
> > > `[]` style attributes, but not `__attribute__` or `__declspec` style 
> > > attributes, is that intended?
> > That is what dxc currently support.
> > It may change in the future. But for now, only [[]] and [] are supported.
> Well... worth noting, HLSL doesn't actually support C++11 attributes, but 
> that is almost certainly going to change in the near future, so we might as 
> well support them from the start in Clang.
Ah, that is good to know @beanz -- we should think carefully about this 
situation because there are some tradeoffs to consider.

1) It's pretty weird to support half of the Microsoft attribute syntax (and the 
half we barely have any attribute support for, at that). Is there a reason to 
not support `__declspec` as well? (For example, are there concerns about things 
like using those attributes to do dllexport or specify a COMDAT section, etc?) 
In fact, if we're going to support vendor attributes like 
`[[clang::overloadable]]`, it's a bit weird that we then prohibit the same 
attribute from being spelled `__attribute__((overloadable))`, is there a 
particular reason to not extend to all attributes?
2) Supporting `[]` style attributes means that attribute order is important. We 
cannot use `MaybeParseAttributes()` to parse attribute specifiers in any order 
because `[]` causes ambiguities under some circumstances. So you're stuck with 
the order you have -- `[[]]` attributes first, `[]` attributes second. Is that 
ordering reasonable?

And for the patch itself -- there are no test cases demonstrating what happens 
when using attributes on things within one of these buffers. I expect many 
things to be quite reasonable, like using `[[deprecated]]`, but are the 
attributes which impact codegen reasonable as well? (Like naked functions, 
returns twice, disable tail calls, etc)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.



Comment at: clang/lib/Parse/ParseHLSL.cpp:79-80
+ParsedAttributes Attrs(AttrFactory);
+MaybeParseCXX11Attributes(Attrs);
+MaybeParseMicrosoftAttributes(Attrs);
+

python3kgae wrote:
> aaron.ballman wrote:
> > Just double-checking, but this allows `[[]]` style attributes as well as 
> > `[]` style attributes, but not `__attribute__` or `__declspec` style 
> > attributes, is that intended?
> That is what dxc currently support.
> It may change in the future. But for now, only [[]] and [] are supported.
Well... worth noting, HLSL doesn't actually support C++11 attributes, but that 
is almost certainly going to change in the near future, so we might as well 
support them from the start in Clang.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae added inline comments.



Comment at: clang/lib/Parse/ParseHLSL.cpp:79-80
+ParsedAttributes Attrs(AttrFactory);
+MaybeParseCXX11Attributes(Attrs);
+MaybeParseMicrosoftAttributes(Attrs);
+

aaron.ballman wrote:
> Just double-checking, but this allows `[[]]` style attributes as well as `[]` 
> style attributes, but not `__attribute__` or `__declspec` style attributes, 
> is that intended?
That is what dxc currently support.
It may change in the future. But for now, only [[]] and [] are supported.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae updated this revision to Diff 458807.
python3kgae marked 4 inline comments as done.
python3kgae added a comment.

Use cast to simplify code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/JSONNodeDumper.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/include/clang/Basic/DeclNodes.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Basic/IdentifierTable.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/IdentifierResolver.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaHLSL.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/ASTCommon.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/AST/HLSL/Inputs/empty.hlsl
  clang/test/AST/HLSL/ast-dump-comment-cbuffe-tbufferr.hlsl
  clang/test/AST/HLSL/cbuffer_tbuffer.hlsl
  clang/test/AST/HLSL/pch_hlsl_buffer.hlsl
  clang/test/ParserHLSL/cb_error.hlsl
  clang/test/ParserHLSL/invalid_inside_cb.hlsl
  clang/test/SemaHLSL/cb_error.hlsl

Index: clang/test/SemaHLSL/cb_error.hlsl
===
--- /dev/null
+++ clang/test/SemaHLSL/cb_error.hlsl
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// expected-note@+1 {{declared here}}
+cbuffer a {
+  int x;
+};
+
+int foo() {
+  // expected-error@+1 {{'a' does not refer to a value}}
+  return sizeof(a);
+}
+
+// expected-error@+1 {{expected unqualified-id}}
+template  cbuffer a { Ty f; };
+
+// For back-compat reason, it is OK for multiple cbuffer/tbuffer use same name in hlsl.
+// And these cbuffer name only used for reflection, cannot be removed.
+cbuffer A {
+  float A;
+}
+
+cbuffer A {
+  float b;
+}
+
+tbuffer A {
+  float a;
+}
+
+float bar() {
+  // cbuffer/tbuffer name will not conflict with other variables.
+  return A;
+}
+
+cbuffer a {
+  // expected-error@+2 {{unknown type name 'oh'}}
+  // expected-error@+1 {{expected ';' after top level declarator}}
+  oh no!
+  // expected-warning@+1 {{missing terminating ' character}}
+  this isn't even valid HLSL code
+  despite seeming totally reasonable
+  once you understand that HLSL
+  is so flaming weird.
+}
+
+tbuffer B {
+  // expected-error@+1 {{unknown type name 'flaot'}}
+  flaot f;
+}
Index: clang/test/ParserHLSL/invalid_inside_cb.hlsl
===
--- /dev/null
+++ clang/test/ParserHLSL/invalid_inside_cb.hlsl
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// template not allowed inside cbuffer.
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+template
+T foo(T t) { return t;}
+}
+
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+template
+struct S { float s;};
+}
+
+// typealias not allowed inside cbuffer.
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+// expected-warning@+1 {{alias declarations are a C++11 extension}}
+using F32 = float;
+}
Index: clang/test/ParserHLSL/cb_error.hlsl
===
--- /dev/null
+++ clang/test/ParserHLSL/cb_error.hlsl
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+cbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+cbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int cbuffer;
+// expected-error@+1 {{expected identifier}}
+cbuffer;
+
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+tbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+tbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int tbuffer;
+// expected-error@+1 {{expected identifier}}
+tbuffer;
+
+// expected-error@+1 {{expected unqualified-id}}
+cbuffer A {}, B{}
+
+// cbuffer inside namespace is supported.
+namespace N {
+  cbuffer A {
+float g;
+  }
+}
+
+cbuffer A {
+  // expected-error@+1 {{invalid declaration inside cbuffer}}
+  namespace N 

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

(I didn't have the chance to do a complete review, but I did a partial one and 
spotted some things)




Comment at: clang/lib/Parse/ParseHLSL.cpp:79-80
+ParsedAttributes Attrs(AttrFactory);
+MaybeParseCXX11Attributes(Attrs);
+MaybeParseMicrosoftAttributes(Attrs);
+

Just double-checking, but this allows `[[]]` style attributes as well as `[]` 
style attributes, but not `__attribute__` or `__declspec` style attributes, is 
that intended?



Comment at: clang/lib/Sema/SemaHLSL.cpp:31-32
+void Sema::ActOnFinishHLSLBuffer(Decl *Dcl, SourceLocation RBrace) {
+  auto *BufDecl = dyn_cast_if_present(Dcl);
+  assert(BufDecl && "Invalid parameter, expected HLSLBufferDecl");
+  BufDecl->setRBraceLoc(RBrace);

`cast` will assert if given nullptr or the cast is invalid, so this should be 
equivalent.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae marked 7 inline comments as done.
python3kgae added inline comments.



Comment at: clang/lib/Parse/ParseHLSL.cpp:59
+
+switch (Tok.getKind()) {
+case tok::kw_namespace:

aaron.ballman wrote:
> The approach of using a switch and handling individual keywords specially 
> strikes me as being quite fragile. I think a better approach is to split this 
> loop out into its own function and model it (at least somewhat) after 
> `ParseStructUnionBody()`.
> 
> The current approach worries me because I have no idea why there's a giant 
> block of invalid things or what should be added to that as we add new 
> declarations to the compiler (and certainly nobody is going to think to come 
> update this function when adding support for new kinds of declarations.
Changed to ParseExternalDeclaration then validate that only Function/Record/Var 
Declarations are in the result.



Comment at: clang/lib/Parse/ParseHLSL.cpp:103-104
+case tok::kw_export:
+case tok::kw_using:
+case tok::kw_typedef:
+case tok::kw_template:

aaron.ballman wrote:
> Why are type aliases prohibited?
cbuffer is a legacy feature for HLSL while type alias is a new feature for 
HLSL2021.
The plan is to keep the legacy features as is.



Comment at: clang/lib/Parse/ParseHLSL.cpp:134
+  }
+  ParseSimpleDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
+ DeclSpecAttrs, true, nullptr, );

python3kgae wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > You're parsing things and then dropping them on the floor?
> > The declarator context looks wrong to me -- I don't see anything 
> > prohibiting the user from doing:
> > ```
> > namespace N {
> > cbuffer {
> >   ...
> > }
> > }
> > ```
> > 
> The goal is to add these things to HLSLBuffer DeclarationContext.
> ActOnStartHLSLBuffer should already make sure that.
namespace N {
  cbuffer A {
  }
}
is supported in HLSL.

cbuffer A {
  namespace N {
  }
} is tricky to support because the namespace N decl inside cbuffer needs to be 
accessed by things outside the cbuffer. This is not supported now and I hope we 
don't need to support it in the future.

I'll add test for the supported case.



Comment at: clang/lib/Parse/ParseHLSL.cpp:134-135
+  }
+  ParseSimpleDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
+ DeclSpecAttrs, true, nullptr, );
+  break;

aaron.ballman wrote:
> aaron.ballman wrote:
> > You're parsing things and then dropping them on the floor?
> The declarator context looks wrong to me -- I don't see anything prohibiting 
> the user from doing:
> ```
> namespace N {
> cbuffer {
>   ...
> }
> }
> ```
> 
The goal is to add these things to HLSLBuffer DeclarationContext.
ActOnStartHLSLBuffer should already make sure that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae updated this revision to Diff 458585.
python3kgae marked 14 inline comments as done.
python3kgae added a comment.

Parse first and report error on the generated AST.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/JSONNodeDumper.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/include/clang/Basic/DeclNodes.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Basic/IdentifierTable.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/IdentifierResolver.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaHLSL.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/ASTCommon.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/AST/HLSL/Inputs/empty.hlsl
  clang/test/AST/HLSL/ast-dump-comment-cbuffe-tbufferr.hlsl
  clang/test/AST/HLSL/cbuffer_tbuffer.hlsl
  clang/test/AST/HLSL/pch_hlsl_buffer.hlsl
  clang/test/ParserHLSL/cb_error.hlsl
  clang/test/ParserHLSL/invalid_inside_cb.hlsl
  clang/test/SemaHLSL/cb_error.hlsl

Index: clang/test/SemaHLSL/cb_error.hlsl
===
--- /dev/null
+++ clang/test/SemaHLSL/cb_error.hlsl
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// expected-note@+1 {{declared here}}
+cbuffer a {
+  int x;
+};
+
+int foo() {
+  // expected-error@+1 {{'a' does not refer to a value}}
+  return sizeof(a);
+}
+
+// expected-error@+1 {{expected unqualified-id}}
+template  cbuffer a { Ty f; };
+
+// For back-compat reason, it is OK for multiple cbuffer/tbuffer use same name in hlsl.
+// And these cbuffer name only used for reflection, cannot be removed.
+cbuffer A {
+  float A;
+}
+
+cbuffer A {
+  float b;
+}
+
+tbuffer A {
+  float a;
+}
+
+float bar() {
+  // cbuffer/tbuffer name will not conflict with other variables.
+  return A;
+}
+
+cbuffer a {
+  // expected-error@+2 {{unknown type name 'oh'}}
+  // expected-error@+1 {{expected ';' after top level declarator}}
+  oh no!
+  // expected-warning@+1 {{missing terminating ' character}}
+  this isn't even valid HLSL code
+  despite seeming totally reasonable
+  once you understand that HLSL
+  is so flaming weird.
+}
+
+tbuffer B {
+  // expected-error@+1 {{unknown type name 'flaot'}}
+  flaot f;
+}
Index: clang/test/ParserHLSL/invalid_inside_cb.hlsl
===
--- /dev/null
+++ clang/test/ParserHLSL/invalid_inside_cb.hlsl
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// template not allowed inside cbuffer.
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+template
+T foo(T t) { return t;}
+}
+
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+template
+struct S { float s;};
+}
+
+// typealias not allowed inside cbuffer.
+cbuffer A {
+// expected-error@+2 {{invalid declaration inside cbuffer}}
+// expected-warning@+1 {{alias declarations are a C++11 extension}}
+using F32 = float;
+}
Index: clang/test/ParserHLSL/cb_error.hlsl
===
--- /dev/null
+++ clang/test/ParserHLSL/cb_error.hlsl
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+cbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+cbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int cbuffer;
+// expected-error@+1 {{expected identifier}}
+cbuffer;
+
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+tbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+tbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int tbuffer;
+// expected-error@+1 {{expected identifier}}
+tbuffer;
+
+// expected-error@+1 {{expected unqualified-id}}
+cbuffer A {}, B{}
+
+// cbuffer inside namespace is supported.
+namespace N {
+  cbuffer A {
+float g;
+  }
+}
+
+cbuffer A {
+  // expected-error@+1 {{invalid declaration inside 

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks for the update, this is heading in a good direction! I think there's 
still a lot of test coverage missing. Consider:

  namespace Name {
  cbuffer a {
int x;
  }
  }
  
  struct S {
cbuffer what {
  int y;
}
  };
  
  void func() {
tbuffer derp {
  int z;
}
  
decltype(derp) another {
  int a;
}
  }




Comment at: clang/include/clang/AST/Decl.h:4690-4693
+  static HLSLBufferDecl *Create(ASTContext , DeclContext *LexicalParent,
+bool CBuffer, SourceLocation KwLoc,
+IdentifierInfo *ID, SourceLocation IDLoc,
+SourceLocation LBrace);

aaron.ballman wrote:
> Speculative question: would it make more sense to add `CreateCBuffer` and 
> `CreateTBuffer` static methods rather than use a `bool` parameter?
I'm still on the fence about this. On the one hand, being explicit is far 
better than hoping the reader understand the bool argument at the call site. On 
the other hand, using the bool parameter means less branching at the call 
sites. So I'm leaning towards leaving this as-is, but if @beanz has other 
opinions, I don't yet have a strong feeling.



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1608
   "expected HLSL Semantic identifier">;
+def err_invalid_declaration_in_hlsl_buffer : Error<"invalid declaration inside 
cbuffer/tbuffer">;
 def err_unknown_hlsl_semantic : Error<"unknown HLSL semantic %0">;

You know what kind of buffer it's within so we can be more precise.



Comment at: clang/lib/AST/JSONNodeDumper.cpp:904
+  VisitNamedDecl(D);
+  JOS.attribute("buffer_kind", D->isCBuffer() ? "cbuffer" : "tbuffer");
+}

Matches the naming style for most everything else in the file.



Comment at: clang/lib/Parse/ParseHLSL.cpp:20-21
 
+Decl *Parser::ParseHLSLBuffer(SourceLocation ,
+  SourceLocation InlineLoc) {
+  assert((Tok.is(tok::kw_cbuffer) || Tok.is(tok::kw_tbuffer)) &&

Parameter is unused, I presume that's intentional rather than accidental?



Comment at: clang/lib/Parse/ParseHLSL.cpp:25
+  bool IsCBuffer = Tok.is(tok::kw_cbuffer);
+  SourceLocation BufferLoc = ConsumeToken(); // eat the 'cbuffer or tbuffer'.
+





Comment at: clang/lib/Parse/ParseHLSL.cpp:33
+  IdentifierInfo *Identifier = Tok.getIdentifierInfo();
+  SourceLocation IdentifierLoc = ConsumeToken(); // consume identifier
+





Comment at: clang/lib/Parse/ParseHLSL.cpp:50-54
+// FIXME: support attribute on constants inside cbuffer/tbuffer.
+ParsedAttributes Attrs(AttrFactory);
+ParsedAttributes DeclSpecAttrs(AttrFactory);
+
+if (PP.isCodeCompletionReached()) {





Comment at: clang/lib/Parse/ParseHLSL.cpp:59
+
+switch (Tok.getKind()) {
+case tok::kw_namespace:

The approach of using a switch and handling individual keywords specially 
strikes me as being quite fragile. I think a better approach is to split this 
loop out into its own function and model it (at least somewhat) after 
`ParseStructUnionBody()`.

The current approach worries me because I have no idea why there's a giant 
block of invalid things or what should be added to that as we add new 
declarations to the compiler (and certainly nobody is going to think to come 
update this function when adding support for new kinds of declarations.



Comment at: clang/lib/Parse/ParseHLSL.cpp:64-65
+  T.skipToEnd();
+  return nullptr;
+  break;
+case tok::kw_cbuffer:





Comment at: clang/lib/Parse/ParseHLSL.cpp:71-74
+  return nullptr;
+
+  [[fallthrough]];
+case tok::annot_pragma_vis:

There's no way to reach this, so we shouldn't need the `[[fallthrough]]` here, 
right?



Comment at: clang/lib/Parse/ParseHLSL.cpp:103-104
+case tok::kw_export:
+case tok::kw_using:
+case tok::kw_typedef:
+case tok::kw_template:

Why are type aliases prohibited?



Comment at: clang/lib/Parse/ParseHLSL.cpp:106-107
+case tok::kw_template:
+case tok::kw_static_assert:
+case tok::kw__Static_assert:
+case tok::kw_inline:

Why are static assertions prohibited?



Comment at: clang/lib/Parse/ParseHLSL.cpp:122-123
+case tok::kw_static:
+  // Parse (then ignore) 'static' prior to a template instantiation. This
+  // is a GCC extension that we intentionally do not support.
+  if (getLangOpts().CPlusPlus && NextToken().is(tok::kw_template)) {

1) GCC supports HLSL?

2) This seems wrong because it will result in:
```
static template  Ty Val{}; // Prohibited
static void func(); // Allowed

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae marked 5 inline comments as done.
python3kgae added inline comments.



Comment at: clang/lib/AST/Decl.cpp:5228-5230
+  if (DC != LexicalParent) {
+Result->setLexicalDeclContext(LexicalParent);
+  }

aaron.ballman wrote:
> So semantically these are all in the global namespace no matter where they're 
> spelled? e.g., if you put one inside of a namespace... it's still globally 
> accessible?
It should not.
I was doing this to hoist nested buffer.
I'll drop the nested buffer support and make namespace work first.
Nested buffer support will be done in another review.




Comment at: clang/lib/Parse/ParseDecl.cpp:1793
+  case tok::kw_tbuffer:
+SingleDecl = ParseHLSLBuffer(DeclEnd);
+break;

aaron.ballman wrote:
> So these can form a declaration group? e.g.,
> ```
> cbuffer a { int b; }, b { float f; };
> ```
> I don't see any test coverage for this situation if that is supported. If 
> it's not supported, I think this should be a `return` statement instead.
No, it cannot form a declaration group.
If early return, still need to convert to Decl group to match the return type 
of ParseDeclaration.



Comment at: clang/lib/Parse/ParseHLSL.cpp:48-52
+  while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) {
+ParsedAttributes Attrs(AttrFactory);
+// FIXME: support attribute on constants inside cbuffer/tbuffer.
+ParseExternalDeclaration(Attrs);
+  }

aaron.ballman wrote:
> This looks like it's going to have some pretty bad error recovery behavior if 
> something is amiss:
> ```
> cbuffer a {
>   oh no!
>   this isn't even valid HLSL code
>   despite seeming totally reasonable
>   once you understand that HLSL
>   is so flaming weird.
> };
> ```
Add more checks for the Tok here.



Comment at: clang/lib/Sema/SemaHLSL.cpp:27
+
+  ActOnDocumentableDecl(Result);
+

aaron.ballman wrote:
> I don't see any tests for this.
Removed. Not see any difference without it when -Wdocumentation is on.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:877
+Decl *TemplateDeclInstantiator::VisitHLSLBufferDecl(HLSLBufferDecl *Decl) {
+  llvm_unreachable("HLSL buffer declarations cannot be instantiated");
+}

aaron.ballman wrote:
> Is this actually unreachable?
Yeah. template on cbuffer/tbuffer is not allowed.



Comment at: clang/test/AST/HLSL/cbuffer_tbuffer.hlsl:7
+  float a;
+}
+

aaron.ballman wrote:
> Something's amiss -- this declaration doesn't have a semicolon, is that 
> expected? Because the SemaHLSL tests do have terminating semicolons and no 
> diagnostics about it being superfluous.
It is expected that cbuffer doesn't need a semicolon.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae updated this revision to Diff 455672.
python3kgae marked 10 inline comments as done.
python3kgae added a comment.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.

Add more comment about no name conflict for cbuffer.
Add Serialize and JSON dump for HLSLBufferDecl.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/JSONNodeDumper.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/include/clang/Basic/DeclNodes.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Basic/IdentifierTable.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/IdentifierResolver.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaHLSL.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/ASTCommon.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/AST/HLSL/Inputs/empty.hlsl
  clang/test/AST/HLSL/ast-dump-comment-cbuffe-tbufferr.hlsl
  clang/test/AST/HLSL/cbuffer_tbuffer.hlsl
  clang/test/AST/HLSL/pch_hlsl_buffer.hlsl
  clang/test/ParserHLSL/cb_error.hlsl
  clang/test/ParserHLSL/invalid_inside_cb.hlsl
  clang/test/SemaHLSL/cb_error.hlsl

Index: clang/test/SemaHLSL/cb_error.hlsl
===
--- /dev/null
+++ clang/test/SemaHLSL/cb_error.hlsl
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// expected-note@+1 {{declared here}}
+cbuffer a {
+  int x;
+};
+
+int foo() {
+  // expected-error@+1 {{'a' does not refer to a value}}
+  return sizeof(a);
+}
+
+// expected-error@+1 {{expected unqualified-id}}
+template  cbuffer a { Ty f; };
+
+// For back-compat reason, it is OK for multiple cbuffer/tbuffer use same name in hlsl.
+// And these cbuffer name only used for reflection, cannot be removed.
+cbuffer A {
+  float A;
+}
+
+cbuffer A {
+  float b;
+}
+
+tbuffer A {
+  float a;
+}
+
+float bar() {
+  // cbuffer/tbuffer name will not conflict with other variables.
+  return A;
+}
+
+cbuffer a {
+  // expected-error@+2 {{unknown type name 'oh'}}
+  // expected-error@+1 {{expected ';' after top level declarator}}
+  oh no!
+  // expected-warning@+1 {{missing terminating ' character}}
+  this isn't even valid HLSL code
+  despite seeming totally reasonable
+  once you understand that HLSL
+  is so flaming weird.
+}
+
+tbuffer B {
+  // expected-error@+1 {{unknown type name 'flaot'}}
+  flaot f;
+}
Index: clang/test/ParserHLSL/invalid_inside_cb.hlsl
===
--- /dev/null
+++ clang/test/ParserHLSL/invalid_inside_cb.hlsl
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+cbuffer A {
+// expected-error@+1 {{invalid declaration inside cbuffer/tbuffer}}
+  namespace N {
+
+  }
+
+}
+
+cbuffer A {
+// expected-error@+1 {{invalid declaration inside cbuffer/tbuffer}}
+cbuffer B {
+
+}
+}
+
+tbuffer A {
+// expected-error@+1 {{invalid declaration inside cbuffer/tbuffer}}
+-
+}
+cbuffer A {
+// expected-error@+1 {{invalid declaration inside cbuffer/tbuffer}}
++
+}
+cbuffer A {
+// expected-error@+1 {{invalid declaration inside cbuffer/tbuffer}}
+export
+}
+tbuffer A {
+// expected-error@+1 {{invalid declaration inside cbuffer/tbuffer}}
+using
+}
+tbuffer A {
+// expected-error@+1 {{invalid declaration inside cbuffer/tbuffer}}
+typedef
+}
+cbuffer A {
+// expected-error@+1 {{invalid declaration inside cbuffer/tbuffer}}
+template
+}
+cbuffer A {
+// expected-error@+1 {{invalid declaration inside cbuffer/tbuffer}}
+  static template
+}
+
Index: clang/test/ParserHLSL/cb_error.hlsl
===
--- /dev/null
+++ clang/test/ParserHLSL/cb_error.hlsl
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+cbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+cbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int cbuffer;
+// expected-error@+1 {{expected identifier}}
+cbuffer;
+
+// expected-error@+2 {{expected identifier}}
+// 

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

There's a whole ton of test coverage missing from this (no actual sema tests, 
no AST dumping tests for text or JSON) not to mention a ton of situational 
tests that are missing. For example, can you put one of these declarations 
inside of a struct (so it's a member declaration)? How about declaring one as a 
local variable? Tests that it properly diagnoses use of a template, as in 
`template  cbuffer a { Ty f; };`? Are there restrictions on what 
can be declared within one of these buffers (can I declare an inline function 
in one?)? All those sorts of situations and more should be tested explicitly.




Comment at: clang/include/clang/AST/Decl.h:4675
+/// HLSLBufferDecl - Represent a cbuffer or tbuffer declaration.
+class HLSLBufferDecl : public NamedDecl, public DeclContext {
+  /// LBraceLoc - The ending location of the source range.

Are these redeclarable declarations? I assume not based on previous 
discussions, so mostly just double checking.



Comment at: clang/include/clang/AST/Decl.h:4690-4693
+  static HLSLBufferDecl *Create(ASTContext , DeclContext *LexicalParent,
+bool CBuffer, SourceLocation KwLoc,
+IdentifierInfo *ID, SourceLocation IDLoc,
+SourceLocation LBrace);

Speculative question: would it make more sense to add `CreateCBuffer` and 
`CreateTBuffer` static methods rather than use a `bool` parameter?



Comment at: clang/include/clang/AST/Decl.h:4696
+
+  virtual SourceRange getSourceRange() const LLVM_READONLY {
+return SourceRange(getLocStart(), RBraceLoc);

Nope. :-D



Comment at: clang/include/clang/AST/Decl.h:4699-4700
+  }
+  const char *getDeclKindName() const;
+  SourceLocation getLocStart() const LLVM_READONLY { return KwLoc; }
+  SourceLocation getRBraceLoc() const { return RBraceLoc; }

Unused and unimplemented.



Comment at: clang/lib/AST/Decl.cpp:5228-5230
+  if (DC != LexicalParent) {
+Result->setLexicalDeclContext(LexicalParent);
+  }

So semantically these are all in the global namespace no matter where they're 
spelled? e.g., if you put one inside of a namespace... it's still globally 
accessible?



Comment at: clang/lib/AST/DeclBase.cpp:1197-1198
 
-  return getDeclKind() == Decl::LinkageSpec || getDeclKind() == Decl::Export;
+  return getDeclKind() == Decl::LinkageSpec || getDeclKind() == Decl::Export ||
+ getDeclKind() == Decl::HLSLBuffer;
 }





Comment at: clang/lib/AST/DeclPrinter.cpp:466-472
 } else if (isa(*D) || isa(*D) ||
  isa(*D) ||
  isa(*D) ||
  isa(*D) ||
  isa(*D) ||
- isa(*D))
+ isa(*D) ||
+ isa(*D))

Let's make this better while we're touching it.



Comment at: clang/lib/AST/DeclPrinter.cpp:1664-1668
+  if (D->isCBuffer()) {
+Out << "cbuffer ";
+  } else {
+Out << "tbuffer ";
+  }





Comment at: clang/lib/AST/TextNodeDumper.cpp:2390
+
+void TextNodeDumper::VisitHLSLBufferDecl(const HLSLBufferDecl *D) {
+  if (D->isCBuffer())

You're missing similar changes for the `JSONNodeDumper`.



Comment at: clang/lib/Parse/ParseDecl.cpp:1793
+  case tok::kw_tbuffer:
+SingleDecl = ParseHLSLBuffer(DeclEnd);
+break;

So these can form a declaration group? e.g.,
```
cbuffer a { int b; }, b { float f; };
```
I don't see any test coverage for this situation if that is supported. If it's 
not supported, I think this should be a `return` statement instead.



Comment at: clang/lib/Parse/ParseHLSL.cpp:48-52
+  while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) {
+ParsedAttributes Attrs(AttrFactory);
+// FIXME: support attribute on constants inside cbuffer/tbuffer.
+ParseExternalDeclaration(Attrs);
+  }

This looks like it's going to have some pretty bad error recovery behavior if 
something is amiss:
```
cbuffer a {
  oh no!
  this isn't even valid HLSL code
  despite seeming totally reasonable
  once you understand that HLSL
  is so flaming weird.
};
```



Comment at: clang/lib/Sema/SemaHLSL.cpp:27
+
+  ActOnDocumentableDecl(Result);
+

I don't see any tests for this.



Comment at: clang/lib/Sema/SemaHLSL.cpp:33
+void Sema::ActOnFinishHLSLBuffer(Decl *Dcl, SourceLocation RBrace) {
+  HLSLBufferDecl *BufDecl = dyn_cast_or_null(Dcl);
+  assert(BufDecl && "Invalid parameter, expected HLSLBufferDecl");

(The interface was deprecated recently and this is the replacement API.)



Comment 

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae updated this revision to Diff 452761.
python3kgae marked 3 inline comments as done.
python3kgae added a comment.

Add more comment about no name conflict for cbuffer.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/include/clang/Basic/DeclNodes.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Basic/IdentifierTable.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaHLSL.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/ASTCommon.cpp
  clang/test/AST/HLSL/cbuffer_tbuffer.hlsl
  clang/test/SemaHLSL/cb_error.hlsl

Index: clang/test/SemaHLSL/cb_error.hlsl
===
--- /dev/null
+++ clang/test/SemaHLSL/cb_error.hlsl
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+cbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+cbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int cbuffer;
+// expected-error@+1 {{expected identifier}}
+cbuffer;
+
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+tbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+tbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int tbuffer;
+// expected-error@+1 {{expected identifier}}
+tbuffer;
Index: clang/test/AST/HLSL/cbuffer_tbuffer.hlsl
===
--- /dev/null
+++ clang/test/AST/HLSL/cbuffer_tbuffer.hlsl
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -ast-dump -o - %s | FileCheck %s
+
+// CHECK:HLSLBufferDecl 0x[[CB:[0-9a-f]+]] {{.*}} line:5:9 cbuffer CB
+// CHECK-NEXT:VarDecl 0x[[A:[0-9a-f]+]] {{.*}} col:9 used a 'float'
+cbuffer CB {
+  float a;
+}
+
+// CHECK:HLSLBufferDecl 0x[[TB:[0-9a-f]+]] {{.*}} line:11:9 tbuffer TB
+// CHECK-NEXT:VarDecl 0x[[B:[0-9a-f]+]] {{.*}} col:9 used b 'float'
+tbuffer TB {
+  float b;
+}
+
+float foo() {
+// CHECK: BinaryOperator 0x{{[0-9a-f]+}}  'float' '+'
+// CHECK-NEXT: ImplicitCastExpr 0x{{[0-9a-f]+}}  'float' 
+// CHECK-NEXT: DeclRefExpr 0x{{[0-9a-f]+}}  'float' lvalue Var 0x[[A]] 'a' 'float'
+// CHECK-NEXT: ImplicitCastExpr 0x{{[0-9a-f]+}}  'float' 
+// CHECK-NEXT: DeclRefExpr 0x{{[0-9a-f]+}}  'float' lvalue Var 0x[[B]] 'b' 'float'
+  return a + b;
+}
Index: clang/lib/Serialization/ASTCommon.cpp
===
--- clang/lib/Serialization/ASTCommon.cpp
+++ clang/lib/Serialization/ASTCommon.cpp
@@ -433,6 +433,7 @@
   case Decl::LifetimeExtendedTemporary:
   case Decl::RequiresExprBody:
   case Decl::UnresolvedUsingIfExists:
+  case Decl::HLSLBuffer:
 return false;
 
   // These indirectly derive from Redeclarable but are not actually
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -873,6 +873,10 @@
   llvm_unreachable("Translation units cannot be instantiated");
 }
 
+Decl *TemplateDeclInstantiator::VisitHLSLBufferDecl(HLSLBufferDecl *Decl) {
+  llvm_unreachable("HLSL buffer declarations cannot be instantiated");
+}
+
 Decl *
 TemplateDeclInstantiator::VisitPragmaCommentDecl(PragmaCommentDecl *D) {
   llvm_unreachable("pragma comment cannot be instantiated");
Index: clang/lib/Sema/SemaHLSL.cpp
===
--- /dev/null
+++ clang/lib/Sema/SemaHLSL.cpp
@@ -0,0 +1,37 @@
+//===- SemaHLSL.cpp - Semantic Analysis for HLSL constructs ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+// This implements Semantic Analysis for HLSL constructs.
+//===--===//
+
+#include "clang/Sema/Sema.h"
+
+using namespace clang;
+
+Decl *Sema::ActOnStartHLSLBuffer(Scope *BufferScope, bool CBuffer,
+ SourceLocation KwLoc, IdentifierInfo *Ident,
+ 

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

Since this is adding a new AST node (hopefully the only one we need in HLSL) it 
would be nice to get @aaron.ballman to take a look here too to make sure this 
isn't too insane.




Comment at: clang/lib/AST/DeclBase.cpp:1191
 
+  if (getDeclKind() == Decl::HLSLBuffer)
+return true;

This could be merged into the return line below, which would make the function 
easier to understand.



Comment at: clang/test/SemaHLSL/cb_error.hlsl:12
+cbuffer;
+

We should duplicate these cases for `tbuffer`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae marked 2 inline comments as done.
python3kgae added inline comments.



Comment at: clang/test/SemaHLSL/cbuffer_tbuffer.hlsl:1
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -ast-dump -o 
- %s | FileCheck %s
+

beanz wrote:
> This looks like it should be an AST test, not SEMA.
> 
> We should also have parser tests for malformed parse cases:
> 
> ```
> cbuffer { ... };
> cbuffer missing_definition;
> int cbuffer; // using a keyword as a variable name
> cbuffer; // lots wrong here...
> cbuffer missing_semicolon { int woo; }
> ```
> 
> It looks like this patch doesn't handle making the buffer variables constant. 
> Having not looked at that I don't know how big of a change that is, so it 
> might be okay as a subsequent patch, but we'll need that support to handle 
> cases like:
> 
> ```
> cbuffer cb{
> int x;
> };
> 
> [numthreads(1,1,1)]
> void main(int GI : SV_GROUPINDEX) {
>   x = GI; // error: buffer content is const
> }
> ```
Marking constant variables const better be done when supporting global constant 
buffer. Will add it in another patch.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae updated this revision to Diff 445162.
python3kgae added a comment.

Code cleanup and add test for error case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/include/clang/Basic/DeclNodes.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Basic/IdentifierTable.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaHLSL.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/ASTCommon.cpp
  clang/test/AST/HLSL/cbuffer_tbuffer.hlsl
  clang/test/SemaHLSL/cb_error.hlsl

Index: clang/test/SemaHLSL/cb_error.hlsl
===
--- /dev/null
+++ clang/test/SemaHLSL/cb_error.hlsl
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+cbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+cbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int cbuffer;
+// expected-error@+1 {{expected identifier}}
+cbuffer;
+
Index: clang/test/AST/HLSL/cbuffer_tbuffer.hlsl
===
--- /dev/null
+++ clang/test/AST/HLSL/cbuffer_tbuffer.hlsl
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -ast-dump -o - %s | FileCheck %s
+
+// CHECK:HLSLBufferDecl 0x[[CB:[0-9a-f]+]] {{.*}} line:5:9 cbuffer CB
+// CHECK-NEXT:VarDecl 0x[[A:[0-9a-f]+]] {{.*}} col:9 used a 'float'
+cbuffer CB {
+  float a;
+}
+
+// CHECK:HLSLBufferDecl 0x[[TB:[0-9a-f]+]] {{.*}} line:11:9 tbuffer TB
+// CHECK-NEXT:VarDecl 0x[[B:[0-9a-f]+]] {{.*}} col:9 used b 'float'
+tbuffer TB {
+  float b;
+}
+
+float foo() {
+// CHECK: BinaryOperator 0x{{[0-9a-f]+}}  'float' '+'
+// CHECK-NEXT: ImplicitCastExpr 0x{{[0-9a-f]+}}  'float' 
+// CHECK-NEXT: DeclRefExpr 0x{{[0-9a-f]+}}  'float' lvalue Var 0x[[A]] 'a' 'float'
+// CHECK-NEXT: ImplicitCastExpr 0x{{[0-9a-f]+}}  'float' 
+// CHECK-NEXT: DeclRefExpr 0x{{[0-9a-f]+}}  'float' lvalue Var 0x[[B]] 'b' 'float'
+  return a + b;
+}
Index: clang/lib/Serialization/ASTCommon.cpp
===
--- clang/lib/Serialization/ASTCommon.cpp
+++ clang/lib/Serialization/ASTCommon.cpp
@@ -433,6 +433,7 @@
   case Decl::LifetimeExtendedTemporary:
   case Decl::RequiresExprBody:
   case Decl::UnresolvedUsingIfExists:
+  case Decl::HLSLBuffer:
 return false;
 
   // These indirectly derive from Redeclarable but are not actually
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -873,6 +873,10 @@
   llvm_unreachable("Translation units cannot be instantiated");
 }
 
+Decl *TemplateDeclInstantiator::VisitHLSLBufferDecl(HLSLBufferDecl *Decl) {
+  llvm_unreachable("HLSL buffer declarations cannot be instantiated");
+}
+
 Decl *
 TemplateDeclInstantiator::VisitPragmaCommentDecl(PragmaCommentDecl *D) {
   llvm_unreachable("pragma comment cannot be instantiated");
Index: clang/lib/Sema/SemaHLSL.cpp
===
--- /dev/null
+++ clang/lib/Sema/SemaHLSL.cpp
@@ -0,0 +1,37 @@
+//===- SemaHLSL.cpp - Semantic Analysis for HLSL constructs ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+// This implements Semantic Analysis for HLSL constructs.
+//===--===//
+
+#include "clang/Sema/Sema.h"
+
+using namespace clang;
+
+Decl *Sema::ActOnStartHLSLBuffer(Scope *BufferScope, bool CBuffer,
+ SourceLocation KwLoc, IdentifierInfo *Ident,
+ SourceLocation IdentLoc,
+ SourceLocation LBrace) {
+  // For anonymous namespace, take the location of the left brace.
+  DeclContext *LexicalParent = getCurLexicalContext();
+  HLSLBufferDecl *Result = HLSLBufferDecl::Create(
+  Context, LexicalParent, CBuffer, KwLoc, Ident, IdentLoc, LBrace);
+
+  

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.



Comment at: clang/include/clang/Parse/Parser.h:2820
   SourceLocation *EndLoc = nullptr);
+  Decl *ParseCTBuffer(SourceLocation ,
+  SourceLocation InlineLoc = SourceLocation());

nit: maybe `ParseHLSLBuffer`, I don't think `ParseCTBuffer` makes it apparent 
what this is doing, also everything else is `HLSLBuffer`



Comment at: clang/test/SemaHLSL/cbuffer_tbuffer.hlsl:1
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -ast-dump -o 
- %s | FileCheck %s
+

This looks like it should be an AST test, not SEMA.

We should also have parser tests for malformed parse cases:

```
cbuffer { ... };
cbuffer missing_definition;
int cbuffer; // using a keyword as a variable name
cbuffer; // lots wrong here...
cbuffer missing_semicolon { int woo; }
```

It looks like this patch doesn't handle making the buffer variables constant. 
Having not looked at that I don't know how big of a change that is, so it might 
be okay as a subsequent patch, but we'll need that support to handle cases like:

```
cbuffer cb{
int x;
};

[numthreads(1,1,1)]
void main(int GI : SV_GROUPINDEX) {
  x = GI; // error: buffer content is const
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129883/new/

https://reviews.llvm.org/D129883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae created this revision.
python3kgae added reviewers: svenvh, asavonic, beanz, pow2clk, Anastasia, 
aaron.ballman.
Herald added a subscriber: mgorny.
Herald added a project: All.
python3kgae requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is first part for support cbuffer/tbuffer.

The format for cbuffer/tbuffer is
BufferType [Name] [: register(b#)] { VariableDeclaration [: 
packoffset(c#.xyzw)]; ... };

More details at 
https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-constants

New keyword 'cbuffer' and 'tbuffer' are added.
New AST node HLSLBufferDecl is added.
Build AST for simple cbuffer/tbuffer without attribute support.

The special thing is variables declared inside cbuffer is exposed into global 
scope.
So isTransparentContext should return true for HLSLBuffer.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129883

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/include/clang/Basic/DeclNodes.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Basic/IdentifierTable.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaHLSL.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/ASTCommon.cpp
  clang/test/SemaHLSL/cbuffer_tbuffer.hlsl

Index: clang/test/SemaHLSL/cbuffer_tbuffer.hlsl
===
--- /dev/null
+++ clang/test/SemaHLSL/cbuffer_tbuffer.hlsl
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -ast-dump -o - %s | FileCheck %s
+
+// CHECK:HLSLBufferDecl 0x[[CB:[0-9a-f]+]] {{.*}} line:5:9 cbuffer CB
+// CHECK-NEXT:VarDecl 0x[[A:[0-9a-f]+]] {{.*}} col:9 used a 'float'
+cbuffer CB {
+  float a;
+};
+
+// CHECK:HLSLBufferDecl 0x[[TB:[0-9a-f]+]] {{.*}} line:11:9 tbuffer TB
+// CHECK-NEXT:VarDecl 0x[[B:[0-9a-f]+]] {{.*}} col:9 used b 'float'
+tbuffer TB {
+  float b;
+};
+
+float foo() {
+// CHECK: BinaryOperator 0x{{[0-9a-f]+}}  'float' '+'
+// CHECK-NEXT: ImplicitCastExpr 0x{{[0-9a-f]+}}  'float' 
+// CHECK-NEXT: DeclRefExpr 0x{{[0-9a-f]+}}  'float' lvalue Var 0x[[A]] 'a' 'float'
+// CHECK-NEXT: ImplicitCastExpr 0x{{[0-9a-f]+}}  'float' 
+// CHECK-NEXT: DeclRefExpr 0x{{[0-9a-f]+}}  'float' lvalue Var 0x[[B]] 'b' 'float'
+  return a + b;
+}
Index: clang/lib/Serialization/ASTCommon.cpp
===
--- clang/lib/Serialization/ASTCommon.cpp
+++ clang/lib/Serialization/ASTCommon.cpp
@@ -433,6 +433,7 @@
   case Decl::LifetimeExtendedTemporary:
   case Decl::RequiresExprBody:
   case Decl::UnresolvedUsingIfExists:
+  case Decl::HLSLBuffer:
 return false;
 
   // These indirectly derive from Redeclarable but are not actually
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -873,6 +873,10 @@
   llvm_unreachable("Translation units cannot be instantiated");
 }
 
+Decl *TemplateDeclInstantiator::VisitHLSLBufferDecl(HLSLBufferDecl *Decl) {
+  llvm_unreachable("HLSL buffer declarations cannot be instantiated");
+}
+
 Decl *
 TemplateDeclInstantiator::VisitPragmaCommentDecl(PragmaCommentDecl *D) {
   llvm_unreachable("pragma comment cannot be instantiated");
Index: clang/lib/Sema/SemaHLSL.cpp
===
--- /dev/null
+++ clang/lib/Sema/SemaHLSL.cpp
@@ -0,0 +1,37 @@
+//===- SemaHLSL.cpp - Semantic Analysis for HLSL constructs ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+// This implements Semantic Analysis for HLSL constructs.
+//===--===//
+
+#include "clang/Sema/Sema.h"
+
+using namespace clang;
+
+Decl *Sema::ActOnStartHLSLBuffer(Scope *BufferScope, bool CBuffer,
+ SourceLocation KwLoc, IdentifierInfo *Ident,
+ SourceLocation IdentLoc,
+ SourceLocation LBrace) {
+  // For anonymous namespace, take the location of the left brace.
+  DeclContext *LexicalParent = getCurLexicalContext();
+  HLSLBufferDecl *Result = HLSLBufferDecl::Create(
+  Context,