[llvm-branch-commits] [clang] [llvm] [HLSL][RootSignature] Implement Parsing of Descriptor Tables (PR #122982)

[Ashley Coleman via llvm-branch-commits]

@@ -148,6 +148,333 @@ bool RootSignatureLexer::LexToken(RootSignatureToken 
&Result) {
   return false;
 }
 
+// Parser Definitions
+
+RootSignatureParser::RootSignatureParser(
+SmallVector &Elements,
+const SmallVector &Tokens)
+: Elements(Elements) {
+  CurTok = Tokens.begin();
+  LastTok = Tokens.end();
+}
+
+bool RootSignatureParser::ReportError() { return true; }
+
+bool RootSignatureParser::Parse() {
+  CurTok--; // Decrement once here so we can use the ...ExpectedToken api
+
+  // Iterate as many RootElements as possible
+  bool HasComma = true;
+  while (HasComma &&
+ !TryConsumeExpectedToken(ArrayRef{TokenKind::kw_DescriptorTable})) {

V-FEXrt wrote:

If the `TryConsumeExpectedToken` fails on the first iteration, does this code 
incorrectly report the comma error?

https://github.com/llvm/llvm-project/pull/122982
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL][RootSignature] Implement Parsing of Descriptor Tables (PR #122982)

[Ashley Coleman via llvm-branch-commits]

@@ -148,6 +148,333 @@ bool RootSignatureLexer::LexToken(RootSignatureToken 
&Result) {
   return false;
 }
 
+// Parser Definitions
+
+RootSignatureParser::RootSignatureParser(
+SmallVector &Elements,
+const SmallVector &Tokens)
+: Elements(Elements) {
+  CurTok = Tokens.begin();
+  LastTok = Tokens.end();
+}
+
+bool RootSignatureParser::ReportError() { return true; }
+
+bool RootSignatureParser::Parse() {
+  CurTok--; // Decrement once here so we can use the ...ExpectedToken api
+
+  // Iterate as many RootElements as possible
+  bool HasComma = true;
+  while (HasComma &&
+ !TryConsumeExpectedToken(ArrayRef{TokenKind::kw_DescriptorTable})) {
+if (ParseRootElement())
+  return true;
+HasComma = !TryConsumeExpectedToken(TokenKind::pu_comma);
+  }
+  if (HasComma)
+return ReportError(); // report 'comma' denotes a required extra item
+
+  // Ensure that we are at the end of the tokens
+  CurTok++;
+  if (CurTok != LastTok)
+return ReportError(); // report expected end of input but got more
+  return false;
+}
+
+bool RootSignatureParser::ParseRootElement() {
+  // Dispatch onto the correct parse method
+  switch (CurTok->Kind) {
+  case TokenKind::kw_DescriptorTable:
+return ParseDescriptorTable();
+  default:
+llvm_unreachable("Switch for an expected token was not provided");
+return true;
+  }
+}
+
+bool RootSignatureParser::ParseDescriptorTable() {
+  DescriptorTable Table;
+
+  if (ConsumeExpectedToken(TokenKind::pu_l_paren))
+return true;
+
+  // Iterate as many DescriptorTableClaues as possible
+  bool HasComma = true;
+  while (!TryConsumeExpectedToken({TokenKind::kw_CBV, TokenKind::kw_SRV,

V-FEXrt wrote:

You need the `while (HasComma && ...` here right? Otherwise it will continue 
looping even if the comma is missing?

https://github.com/llvm/llvm-project/pull/122982
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL][RootSignature] Implement Parsing of Descriptor Tables (PR #122982)

[Ashley Coleman via llvm-branch-commits]

@@ -148,6 +148,333 @@ bool RootSignatureLexer::LexToken(RootSignatureToken 
&Result) {
   return false;
 }
 
+// Parser Definitions
+
+RootSignatureParser::RootSignatureParser(
+SmallVector &Elements,
+const SmallVector &Tokens)
+: Elements(Elements) {
+  CurTok = Tokens.begin();
+  LastTok = Tokens.end();
+}
+
+bool RootSignatureParser::ReportError() { return true; }
+
+bool RootSignatureParser::Parse() {
+  CurTok--; // Decrement once here so we can use the ...ExpectedToken api
+
+  // Iterate as many RootElements as possible
+  bool HasComma = true;
+  while (HasComma &&
+ !TryConsumeExpectedToken(ArrayRef{TokenKind::kw_DescriptorTable})) {
+if (ParseRootElement())
+  return true;
+HasComma = !TryConsumeExpectedToken(TokenKind::pu_comma);
+  }
+  if (HasComma)
+return ReportError(); // report 'comma' denotes a required extra item
+
+  // Ensure that we are at the end of the tokens
+  CurTok++;
+  if (CurTok != LastTok)
+return ReportError(); // report expected end of input but got more
+  return false;
+}
+
+bool RootSignatureParser::ParseRootElement() {
+  // Dispatch onto the correct parse method
+  switch (CurTok->Kind) {
+  case TokenKind::kw_DescriptorTable:
+return ParseDescriptorTable();
+  default:
+llvm_unreachable("Switch for an expected token was not provided");
+return true;
+  }
+}
+
+bool RootSignatureParser::ParseDescriptorTable() {
+  DescriptorTable Table;
+
+  if (ConsumeExpectedToken(TokenKind::pu_l_paren))
+return true;
+
+  // Iterate as many DescriptorTableClaues as possible
+  bool HasComma = true;
+  while (!TryConsumeExpectedToken({TokenKind::kw_CBV, TokenKind::kw_SRV,
+   TokenKind::kw_UAV, TokenKind::kw_Sampler})) 
{
+if (ParseDescriptorTableClause())
+  return true;
+Table.NumClauses++;
+HasComma = !TryConsumeExpectedToken(TokenKind::pu_comma);
+  }
+
+  // Consume optional 'visibility' paramater
+  if (HasComma && !TryConsumeExpectedToken(TokenKind::kw_visibility)) {
+if (ConsumeExpectedToken(TokenKind::pu_equal))
+  return true;
+
+if (ParseShaderVisibility(Table.Visibility))
+  return true;
+
+HasComma = !TryConsumeExpectedToken(TokenKind::pu_comma);
+  }
+
+  if (HasComma && Table.NumClauses != 0)
+return ReportError(); // report 'comma' denotes a required extra item
+
+  if (ConsumeExpectedToken(TokenKind::pu_r_paren))
+return true;
+
+  Elements.push_back(RootElement(Table));
+  return false;
+}
+
+bool RootSignatureParser::ParseDescriptorTableClause() {
+  // Determine the type of Clause first so we can initialize the struct with
+  // the correct default flags
+  ClauseType CT;
+  switch (CurTok->Kind) {
+  case TokenKind::kw_CBV:
+CT = ClauseType::CBV;
+break;
+  case TokenKind::kw_SRV:
+CT = ClauseType::SRV;
+break;
+  case TokenKind::kw_UAV:
+CT = ClauseType::UAV;
+break;
+  case TokenKind::kw_Sampler:
+CT = ClauseType::Sampler;
+break;
+  default:
+llvm_unreachable("Switch for an expected token was not provided");
+return true;
+  }
+  DescriptorTableClause Clause(CT);
+
+  if (ConsumeExpectedToken(TokenKind::pu_l_paren))
+return true;
+
+  // Consume mandatory Register paramater
+  if (ConsumeExpectedToken(
+  {TokenKind::bReg, TokenKind::tReg, TokenKind::uReg, 
TokenKind::sReg}))
+return true;
+  if (ParseRegister(Clause.Register))
+return true;
+
+  // Start parsing the optional parameters

V-FEXrt wrote:

Is the order of the optional flags set (i.e must be numDescriptors, then space, 
then offset, then flags ...) or can they be provided in any order?

https://github.com/llvm/llvm-project/pull/122982
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL][RootSignature] Implement Parsing of Descriptor Tables (PR #122982)

[Ashley Coleman via llvm-branch-commits]

@@ -148,6 +148,333 @@ bool RootSignatureLexer::LexToken(RootSignatureToken 
&Result) {
   return false;
 }
 
+// Parser Definitions
+
+RootSignatureParser::RootSignatureParser(
+SmallVector &Elements,
+const SmallVector &Tokens)
+: Elements(Elements) {
+  CurTok = Tokens.begin();
+  LastTok = Tokens.end();
+}
+
+bool RootSignatureParser::ReportError() { return true; }
+
+bool RootSignatureParser::Parse() {
+  CurTok--; // Decrement once here so we can use the ...ExpectedToken api

V-FEXrt wrote:

This is UB right? You are decrementing the `CurTok` out of its actual bounds

https://github.com/llvm/llvm-project/pull/122982
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL] Add resource constructor with implicit binding for global resources (PR #138976)

[Ashley Coleman via llvm-branch-commits]

@@ -668,6 +668,26 @@ BuiltinTypeDeclBuilder::addHandleConstructorFromBinding() {
   .finalize();
 }
 
+BuiltinTypeDeclBuilder &
+BuiltinTypeDeclBuilder::addHandleConstructorFromImplicitBinding() {
+  if (Record->isCompleteDefinition())

V-FEXrt wrote:

Could you explain why this is needed?

https://github.com/llvm/llvm-project/pull/138976
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL] Add resource constructor with implicit binding for global resources (PR #138976)

[Ashley Coleman via llvm-branch-commits]

https://github.com/V-FEXrt approved this pull request.


https://github.com/llvm/llvm-project/pull/138976
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL] Add resource constructor with implicit binding for global resources (PR #138976)

[Ashley Coleman via llvm-branch-commits]

@@ -668,6 +668,26 @@ BuiltinTypeDeclBuilder::addHandleConstructorFromBinding() {
   .finalize();
 }
 
+BuiltinTypeDeclBuilder &
+BuiltinTypeDeclBuilder::addHandleConstructorFromImplicitBinding() {
+  if (Record->isCompleteDefinition())
+return *this;
+
+  using PH = BuiltinTypeMethodBuilder::PlaceHolder;
+  ASTContext &AST = SemaRef.getASTContext();
+  QualType HandleType = getResourceHandleField()->getType();
+
+  return BuiltinTypeMethodBuilder(*this, "", AST.VoidTy, false, true)
+  .addParam("spaceNo", AST.UnsignedIntTy)
+  .addParam("range", AST.IntTy)
+  .addParam("index", AST.UnsignedIntTy)
+  .addParam("order_id", AST.UnsignedIntTy)

V-FEXrt wrote:

nit: `spaceNo` and `order_id` are both in different styles. Can we align them?

https://github.com/llvm/llvm-project/pull/138976
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL] Add resource constructor with implicit binding for global resources (PR #138976)

[Ashley Coleman via llvm-branch-commits]

@@ -3269,27 +3285,42 @@ static bool initVarDeclWithCtor(Sema &S, VarDecl *VD,
   return true;
 }
 
-static bool initGlobalResourceDecl(Sema &S, VarDecl *VD) {
+bool SemaHLSL::initGlobalResourceDecl(VarDecl *VD) {
+  std::optional RegisterSlot;
+  uint32_t SpaceNo = 0;
   HLSLResourceBindingAttr *RBA = VD->getAttr();
-  if (!RBA || !RBA->hasRegisterSlot())
-// FIXME: add support for implicit binding (llvm/llvm-project#110722)
-return false;
+  if (RBA) {
+if (RBA->hasRegisterSlot())
+  RegisterSlot = RBA->getSlotNumber();
+SpaceNo = RBA->getSpaceNumber();
+  }
 
-  ASTContext &AST = S.getASTContext();
+  ASTContext &AST = SemaRef.getASTContext();
   uint64_t UIntTySize = AST.getTypeSize(AST.UnsignedIntTy);
   uint64_t IntTySize = AST.getTypeSize(AST.IntTy);
-  Expr *Args[] = {
-  IntegerLiteral::Create(AST, llvm::APInt(UIntTySize, 
RBA->getSlotNumber()),
- AST.UnsignedIntTy, SourceLocation()),
-  IntegerLiteral::Create(AST,
- llvm::APInt(UIntTySize, RBA->getSpaceNumber()),
- AST.UnsignedIntTy, SourceLocation()),
-  IntegerLiteral::Create(AST, llvm::APInt(IntTySize, 1), AST.IntTy,
- SourceLocation()),
-  IntegerLiteral::Create(AST, llvm::APInt(UIntTySize, 0), 
AST.UnsignedIntTy,
- SourceLocation())};
-
-  return initVarDeclWithCtor(S, VD, Args);
+  IntegerLiteral *One = IntegerLiteral::Create(AST, llvm::APInt(IntTySize, 1),
+   AST.IntTy, SourceLocation());
+  IntegerLiteral *Zero = IntegerLiteral::Create(
+  AST, llvm::APInt(UIntTySize, 0), AST.UnsignedIntTy, SourceLocation());
+  IntegerLiteral *Space =
+  IntegerLiteral::Create(AST, llvm::APInt(UIntTySize, SpaceNo),
+ AST.UnsignedIntTy, SourceLocation());
+
+  // resource with explicit binding

V-FEXrt wrote:

This comment is duplicated here and 3318. Is that intentional? 

https://github.com/llvm/llvm-project/pull/138976
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL] Add resource constructor with implicit binding for global resources (PR #138976)

[Ashley Coleman via llvm-branch-commits]

@@ -55,11 +55,33 @@ export void foo() {
 // CHECK-SAME: i32 noundef %0, i32 noundef %1, i32 noundef %2, i32 noundef %3)
 // CHECK-NEXT: ret void
 
-// Buf2 initialization part 1 - FIXME: constructor with implicit binding does 
not exist yet; 
-// the global init function currently calls the default RWByteAddressBuffer C1 
constructor
-// CHECK: define internal void @__cxx_global_var_init.1()
+// Buf2 initialization part 1 - global init function that calls 
RWByteAddressBuffer C1 constructor with implicit binding
+// CHECK: define internal void @__cxx_global_var_init.1() #0 {
 // CHECK-NEXT: entry:
-// CHECK-NEXT: call void @_ZN4hlsl19RWByteAddressBufferC1Ev(ptr noundef 
nonnull align 4 dereferenceable(4) @_ZL4Buf2)
+// CHECK-NEXT: call void @_ZN4hlsl19RWByteAddressBufferC1Ejijj(ptr noundef 
nonnull align 4 dereferenceable(4) @_ZL4Buf2,
+// CHECK-SAME: i32 noundef 0, i32 noundef 1, i32 noundef 0, i32 noundef 0)
+
+// Buf2 initialization part 2 - body of RWByteAddressBuffer C1 constructor 
with implicit binding that calls the C2 constructor
+// CHECK: define linkonce_odr void @_ZN4hlsl19RWByteAddressBufferC1Ejijj(ptr 
noundef nonnull align 4 dereferenceable(4) %this,
+// CHECK-SAME: i32 noundef %spaceNo, i32 noundef %range, i32 noundef %index, 
i32 noundef %order_id)
+// CHECK-NEXT: entry:
+// CHECK-NEXT: %this.addr = alloca ptr, align 4

V-FEXrt wrote:

Is this test too fragile? In other tests we don't actually want to verify all 
the `alloca`/`load`s since they aren't actually doing the work we care about 
and will get optimized away. Is that the case here too?

https://github.com/llvm/llvm-project/pull/138976
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL][RootSignature] Add optional parameters for RootConstants (PR #138007)

[Ashley Coleman via llvm-branch-commits]

@@ -82,6 +82,8 @@ class RootSignatureParser {
   struct ParsedConstantParams {
 std::optional Reg;
 std::optional Num32BitConstants;
+std::optional Space;

V-FEXrt wrote:

Yeah I think I agree with Finn here. Seems to map more directly to the source 
to treat space as a param

https://github.com/llvm/llvm-project/pull/138007
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL][RootSignature] Add optional parameters for RootConstants (PR #138007)

[Ashley Coleman via llvm-branch-commits]

@@ -78,6 +78,13 @@ std::optional 
RootSignatureParser::parseRootConstants() {
 
   Constants.Reg = Params->Reg.value();
 
+  // Fill in optional parameters
+  if (Params->Visibility.has_value())

V-FEXrt wrote:

nit:
```suggestion
  if (Params->Visibility)
```

https://github.com/llvm/llvm-project/pull/138007
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL][RootSignature] Add optional parameters for RootConstants (PR #138007)

[Ashley Coleman via llvm-branch-commits]

https://github.com/V-FEXrt approved this pull request.


https://github.com/llvm/llvm-project/pull/138007
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL][RootSignature] Add mandatory parameters for RootConstants (PR #138002)

[Ashley Coleman via llvm-branch-commits]

https://github.com/V-FEXrt approved this pull request.


https://github.com/llvm/llvm-project/pull/138002
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL][RootSignature] Add mandatory parameters for RootConstants (PR #138002)

[Ashley Coleman via llvm-branch-commits]

@@ -57,6 +57,27 @@ std::optional 
RootSignatureParser::parseRootConstants() {
 
   RootConstants Constants;
 
+  auto Params = parseRootConstantParams();
+  if (!Params.has_value())
+return std::nullopt;
+
+  // Check mandatory parameters were provided

V-FEXrt wrote:

```suggestion
  // Check mandatory parameters where provided
```

https://github.com/llvm/llvm-project/pull/138002
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL][RootSignature] Add mandatory parameters for RootConstants (PR #138002)

[Ashley Coleman via llvm-branch-commits]

@@ -57,6 +57,27 @@ std::optional 
RootSignatureParser::parseRootConstants() {
 
   RootConstants Constants;
 
+  auto Params = parseRootConstantParams();
+  if (!Params.has_value())
+return std::nullopt;
+
+  // Check mandatory parameters were provided
+  if (!Params->Num32BitConstants.has_value()) {
+getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
+<< TokenKind::kw_num32BitConstants;
+return std::nullopt;
+  }
+
+  Constants.Num32BitConstants = Params->Num32BitConstants.value();

V-FEXrt wrote:

nit: You can dereference an optional to get the value
```suggestion
  Constants.Num32BitConstants = *Params->Num32BitConstants;
```

https://github.com/llvm/llvm-project/pull/138002
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL][RootSignature] Add mandatory parameters for RootConstants (PR #138002)

[Ashley Coleman via llvm-branch-commits]

@@ -57,6 +57,27 @@ std::optional 
RootSignatureParser::parseRootConstants() {
 
   RootConstants Constants;
 
+  auto Params = parseRootConstantParams();
+  if (!Params.has_value())

V-FEXrt wrote:

nit: 
```suggestion
  if (Params)
```

https://github.com/llvm/llvm-project/pull/138002
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL][RootSignature] Add parsing for RootFlags (PR #138055)

[Ashley Coleman via llvm-branch-commits]

https://github.com/V-FEXrt approved this pull request.


https://github.com/llvm/llvm-project/pull/138055
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [llvm] [HLSL][RootSignature] Add parsing for RootFlags (PR #138055)

[Ashley Coleman via llvm-branch-commits]

@@ -27,6 +27,13 @@ 
RootSignatureParser::RootSignatureParser(SmallVector &Elements,
 bool RootSignatureParser::parse() {
   // Iterate as many RootElements as possible
   do {
+if (tryConsumeExpectedToken(TokenKind::kw_RootFlags)) {
+  auto Flags = parseRootFlags();
+  if (!Flags.has_value())

V-FEXrt wrote:

nit: 
```suggestion
  if (!Flags)
```

https://github.com/llvm/llvm-project/pull/138055
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm] [DirectX] Detect resources with identical overlapping binding (PR #140645)

[Ashley Coleman via llvm-branch-commits]

https://github.com/V-FEXrt approved this pull request.


https://github.com/llvm/llvm-project/pull/140645
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm] [DirectX] Improve error message when a binding cannot be found for a resource (PR #140642)

[Ashley Coleman via llvm-branch-commits]

https://github.com/V-FEXrt approved this pull request.


https://github.com/llvm/llvm-project/pull/140642
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits