[HLSL] Support packoffset attribute in AST #89836

python3kgae · 2024-04-23T21:50:50Z

Add HLSLPackOffsetAttr to save packoffset in AST.

Since we have to parse the attribute manually in ParseHLSLAnnotations, we could create the ParsedAttribute with a integer offset parameter instead of string. This approach avoids parsing the string if the offset is saved as a string in HLSLPackOffsetAttr.

For #57914.

Add HLSLPackOffsetAttr to save packoffset in AST. Since we have to parse the attribute manually in ParseHLSLAnnotations, we could create the ParsedAttribute with a integer offset parameter instead of string. This approach avoids parsing the string if the offset is saved as a string in HLSLPackOffsetAttr.

llvmbot · 2024-04-23T21:51:23Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-hlsl

Author: Xiang Li (python3kgae)

Changes

Add HLSLPackOffsetAttr to save packoffset in AST.

Since we have to parse the attribute manually in ParseHLSLAnnotations, we could create the ParsedAttribute with a integer offset parameter instead of string. This approach avoids parsing the string if the offset is saved as a string in HLSLPackOffsetAttr.

For #57914.

Full diff: https://github.com/llvm/llvm-project/pull/89836.diff

9 Files Affected:

(modified) clang/include/clang/Basic/Attr.td (+7)
(modified) clang/include/clang/Basic/AttrDocs.td (+18)
(modified) clang/include/clang/Basic/DiagnosticParseKinds.td (+2)
(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3)
(modified) clang/lib/Parse/ParseHLSL.cpp (+80)
(modified) clang/lib/Sema/SemaDeclAttr.cpp (+44)
(modified) clang/lib/Sema/SemaHLSL.cpp (+48)
(added) clang/test/AST/HLSL/packoffset.hlsl (+16)
(added) clang/test/SemaHLSL/packoffset-invalid.hlsl (+55)

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 4408d517e70e58..d3d006ed9633f4 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -4372,6 +4372,13 @@ def HLSLResourceBinding: InheritableAttr {
   let Documentation = [HLSLResourceBindingDocs];
 }
 
+def HLSLPackOffset: HLSLAnnotationAttr {
+  let Spellings = [HLSLAnnotation<"packoffset">];
+  let LangOpts = [HLSL];
+  let Args = [IntArgument<"Offset">];
+  let Documentation = [HLSLPackOffsetDocs];
+}
+
 def HLSLSV_DispatchThreadID: HLSLAnnotationAttr {
   let Spellings = [HLSLAnnotation<"SV_DispatchThreadID">];
   let Subjects = SubjectList<[ParmVar, Field]>;
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index a0bbe5861c5722..6bc7813bd43cb4 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -7398,6 +7398,24 @@ The full documentation is available here: https://docs.microsoft.com/en-us/windo
   }];
 }
 
+def HLSLPackOffsetDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+The packoffset attribute is used to change the layout of a cbuffer.
+Attribute spelling in HLSL is: ``packoffset(c[Subcomponent[.component]])``.
+A subcomponent is a register number, which is an integer. A component is in the form of [.xyzw].
+
+Here're packoffset examples with and without component:
+.. code-block:: c++
+  cbuffer A {
+    float3 a : packoffset(c0.y);
+    float4 b : packoffset(c4);
+  }
+
+The full documentation is available here: https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-variable-packoffset
+  }];
+}
+
 def HLSLSV_DispatchThreadIDDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 38174cf3549f14..81433ee79d48b2 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1745,5 +1745,7 @@ def err_hlsl_separate_attr_arg_and_number : Error<"wrong argument format for hls
 def ext_hlsl_access_specifiers : ExtWarn<
   "access specifiers are a clang HLSL extension">,
   InGroup<HLSLExtension>;
+def err_hlsl_unsupported_component : Error<"invalid component '%0' used; expected 'x', 'y', 'z', or 'w'">;
+def err_hlsl_packoffset_invalid_reg : Error<"invalid resource class specifier '%0' for packoffset, expected 'c'">;
 
 } // end of Parser diagnostics
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 63e951daec7477..bde9617c9820a8 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12167,6 +12167,9 @@ def err_hlsl_init_priority_unsupported : Error<
 def err_hlsl_unsupported_register_type : Error<"invalid resource class specifier '%0' used; expected 'b', 's', 't', or 'u'">;
 def err_hlsl_unsupported_register_number : Error<"register number should be an integer">;
 def err_hlsl_expected_space : Error<"invalid space specifier '%0' used; expected 'space' followed by an integer, like space1">;
+def err_hlsl_packoffset_mix : Error<"cannot mix packoffset elements with nonpackoffset elements in a cbuffer">;
+def err_hlsl_packoffset_overlap : Error<"packoffset overlap between %0, %1">;
+def err_hlsl_packoffset_cross_reg_boundary : Error<"packoffset cannot cross register boundary">;
 def err_hlsl_pointers_unsupported : Error<
   "%select{pointers|references}0 are unsupported in HLSL">;
 
diff --git a/clang/lib/Parse/ParseHLSL.cpp b/clang/lib/Parse/ParseHLSL.cpp
index f4cbece31f1810..eac4876ccab49a 100644
--- a/clang/lib/Parse/ParseHLSL.cpp
+++ b/clang/lib/Parse/ParseHLSL.cpp
@@ -183,6 +183,86 @@ void Parser::ParseHLSLAnnotations(ParsedAttributes &Attrs,
       return;
     }
   } break;
+  case ParsedAttr::AT_HLSLPackOffset: {
+    // Parse 'packoffset( c[Subcomponent][.component] )'.
+    // Check '('.
+    if (ExpectAndConsume(tok::l_paren, diag::err_expected_lparen_after)) {
+      SkipUntil(tok::r_paren, StopAtSemi); // skip through )
+      return;
+    }
+    // Check c[Subcomponent] as an identifier.
+    if (!Tok.is(tok::identifier)) {
+      Diag(Tok.getLocation(), diag::err_expected) << tok::identifier;
+      SkipUntil(tok::r_paren, StopAtSemi); // skip through )
+      return;
+    }
+    StringRef OffsetStr = Tok.getIdentifierInfo()->getName();
+    SourceLocation OffsetLoc = Tok.getLocation();
+    if (OffsetStr[0] != 'c') {
+      Diag(Tok.getLocation(), diag::err_hlsl_packoffset_invalid_reg)
+          << OffsetStr;
+      SkipUntil(tok::r_paren, StopAtSemi); // skip through )
+      return;
+    }
+    OffsetStr = OffsetStr.substr(1);
+    unsigned SubComponent = 0;
+    if (!OffsetStr.empty()) {
+      // Make sure SubComponent is a number.
+      if (OffsetStr.getAsInteger(10, SubComponent)) {
+        Diag(OffsetLoc.getLocWithOffset(1),
+             diag::err_hlsl_unsupported_register_number);
+        return;
+      }
+    }
+    unsigned Component = 0;
+    ConsumeToken(); // consume identifier.
+    if (Tok.is(tok::period)) {
+      ConsumeToken(); // consume period.
+      if (!Tok.is(tok::identifier)) {
+        Diag(Tok.getLocation(), diag::err_expected) << tok::identifier;
+        SkipUntil(tok::r_paren, StopAtSemi); // skip through )
+        return;
+      }
+      StringRef ComponentStr = Tok.getIdentifierInfo()->getName();
+      SourceLocation SpaceLoc = Tok.getLocation();
+      ConsumeToken(); // consume identifier.
+      // Make sure Component is a single character.
+      if (ComponentStr.size() != 1) {
+        Diag(SpaceLoc, diag::err_hlsl_unsupported_component) << ComponentStr;
+        SkipUntil(tok::r_paren, StopAtSemi); // skip through )
+        return;
+      } else {
+        switch (ComponentStr[0]) {
+        case 'x':
+          Component = 0;
+          break;
+        case 'y':
+          Component = 1;
+          break;
+        case 'z':
+          Component = 2;
+          break;
+        case 'w':
+          Component = 3;
+          break;
+        default:
+          Diag(SpaceLoc, diag::err_hlsl_unsupported_component) << ComponentStr;
+          SkipUntil(tok::r_paren, StopAtSemi); // skip through )
+          return;
+        }
+      }
+    }
+    unsigned Offset = SubComponent * 4 + Component;
+    ASTContext &Ctx = Actions.getASTContext();
+    ArgExprs.push_back(IntegerLiteral::Create(
+        Ctx, llvm::APInt(Ctx.getTypeSize(Ctx.getSizeType()), Offset),
+        Ctx.getSizeType(),
+        SourceLocation())); // Dummy location for integer literal.
+    if (ExpectAndConsume(tok::r_paren, diag::err_expected)) {
+      SkipUntil(tok::r_paren, StopAtSemi); // skip through )
+      return;
+    }
+  } break;
   case ParsedAttr::UnknownAttribute:
     Diag(Loc, diag::err_unknown_hlsl_semantic) << II;
     return;
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 363ae93cb62df1..373f2e8f50cdb5 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -7314,6 +7314,47 @@ static void handleHLSLSV_DispatchThreadIDAttr(Sema &S, Decl *D,
   D->addAttr(::new (S.Context) HLSLSV_DispatchThreadIDAttr(S.Context, AL));
 }
 
+static void handleHLSLPackOffsetAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  if (!isa<VarDecl>(D)) {
+    S.Diag(AL.getLoc(), diag::err_hlsl_attr_invalid_ast_node)
+        << AL << "cbuffer constant";
+    return;
+  }
+  auto *BufDecl = dyn_cast<HLSLBufferDecl>(D->getDeclContext());
+  if (!BufDecl) {
+    S.Diag(AL.getLoc(), diag::err_hlsl_attr_invalid_ast_node)
+        << AL << "cbuffer constant";
+    return;
+  }
+
+  uint32_t Offset;
+  if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Offset))
+    return;
+
+  QualType T = cast<VarDecl>(D)->getType().getCanonicalType();
+  // Check if T is an array or struct type.
+  // TODO: mark matrix type as aggregate type.
+  bool IsAggregateTy = (T->isArrayType() || T->isStructureType());
+
+  unsigned ComponentNum = Offset & 0x3;
+  // Check ComponentNum is valid for T.
+  if (IsAggregateTy) {
+    if (ComponentNum != 0) {
+      S.Diag(AL.getLoc(), diag::err_hlsl_packoffset_cross_reg_boundary);
+      return;
+    }
+  } else {
+    unsigned size = S.getASTContext().getTypeSize(T);
+    // Make sure ComponentNum + sizeof(T) <= 4.
+    if ((ComponentNum * 32 + size) > 128) {
+      S.Diag(AL.getLoc(), diag::err_hlsl_packoffset_cross_reg_boundary);
+      return;
+    }
+  }
+
+  D->addAttr(::new (S.Context) HLSLPackOffsetAttr(S.Context, AL, Offset));
+}
+
 static void handleHLSLShaderAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   StringRef Str;
   SourceLocation ArgLoc;
@@ -9735,6 +9776,9 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
   case ParsedAttr::AT_HLSLSV_DispatchThreadID:
     handleHLSLSV_DispatchThreadIDAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_HLSLPackOffset:
+    handleHLSLPackOffsetAttr(S, D, AL);
+    break;
   case ParsedAttr::AT_HLSLShader:
     handleHLSLShaderAttr(S, D, AL);
     break;
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index bb9e37f18d370c..fa62cab54e6902 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -42,6 +42,54 @@ Decl *SemaHLSL::ActOnStartBuffer(Scope *BufferScope, bool CBuffer,
 void SemaHLSL::ActOnFinishBuffer(Decl *Dcl, SourceLocation RBrace) {
   auto *BufDecl = cast<HLSLBufferDecl>(Dcl);
   BufDecl->setRBraceLoc(RBrace);
+
+  // Validate packoffset.
+  llvm::SmallVector<std::pair<VarDecl *, HLSLPackOffsetAttr *>> PackOffsetVec;
+  bool HasPackOffset = false;
+  bool HasNonPackOffset = false;
+  for (auto *Field : BufDecl->decls()) {
+    VarDecl *Var = dyn_cast<VarDecl>(Field);
+    if (!Var)
+      continue;
+    if (Field->hasAttr<HLSLPackOffsetAttr>()) {
+      PackOffsetVec.emplace_back(Var, Field->getAttr<HLSLPackOffsetAttr>());
+      HasPackOffset = true;
+    } else {
+      HasNonPackOffset = true;
+    }
+  }
+
+  if (HasPackOffset && HasNonPackOffset) {
+    Diag(BufDecl->getLocation(), diag::err_hlsl_packoffset_mix);
+  } else if (HasPackOffset) {
+    ASTContext &Context = getASTContext();
+    // Make sure no overlap in packoffset.
+    llvm::SmallDenseMap<VarDecl *, std::pair<unsigned, unsigned>>
+        PackOffsetRanges;
+    for (auto &Pair : PackOffsetVec) {
+      VarDecl *Var = Pair.first;
+      HLSLPackOffsetAttr *Attr = Pair.second;
+      unsigned Size = Context.getTypeSize(Var->getType());
+      unsigned Begin = Attr->getOffset() * 32;
+      unsigned End = Begin + Size;
+      for (auto &Range : PackOffsetRanges) {
+        VarDecl *OtherVar = Range.first;
+        unsigned OtherBegin = Range.second.first;
+        unsigned OtherEnd = Range.second.second;
+        if (Begin < OtherEnd && OtherBegin < Begin) {
+          Diag(Var->getLocation(), diag::err_hlsl_packoffset_overlap)
+              << Var << OtherVar;
+          break;
+        } else if (OtherBegin < End && Begin < OtherBegin) {
+          Diag(Var->getLocation(), diag::err_hlsl_packoffset_overlap)
+              << Var << OtherVar;
+          break;
+        }
+      }
+      PackOffsetRanges[Var] = std::make_pair(Begin, End);
+    }
+  }
+
   SemaRef.PopDeclContext();
 }
 
diff --git a/clang/test/AST/HLSL/packoffset.hlsl b/clang/test/AST/HLSL/packoffset.hlsl
new file mode 100644
index 00000000000000..d3cf798c995758
--- /dev/null
+++ b/clang/test/AST/HLSL/packoffset.hlsl
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -triple dxil-unknown-shadermodel6.3-library -S -finclude-default-header  -ast-dump  -x hlsl %s | FileCheck %s
+
+
+// CHECK: HLSLBufferDecl {{.*}} cbuffer A
+cbuffer A
+{
+    // CHECK-NEXT: VarDecl {{.*}} C1 'float4'
+    // CHECK-NEXT: HLSLPackOffsetAttr {{.*}} 0
+    float4 C1 : packoffset(c);
+    // CHECK-NEXT: VarDecl {{.*}} col:11 C2 'float'
+    // CHECK-NEXT: HLSLPackOffsetAttr {{.*}} 4
+    float C2 : packoffset(c1);
+    // CHECK-NEXT: VarDecl {{.*}} col:11 C3 'float'
+    // CHECK-NEXT: HLSLPackOffsetAttr {{.*}} 5
+    float C3 : packoffset(c1.y);
+}
diff --git a/clang/test/SemaHLSL/packoffset-invalid.hlsl b/clang/test/SemaHLSL/packoffset-invalid.hlsl
new file mode 100644
index 00000000000000..a2c7bb5a1e05cd
--- /dev/null
+++ b/clang/test/SemaHLSL/packoffset-invalid.hlsl
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -finclude-default-header -triple dxil-pc-shadermodel6.3-library -verify %s
+
+// expected-error@+1{{cannot mix packoffset elements with nonpackoffset elements in a cbuffer}}
+cbuffer Mix
+{
+    float4 M1 : packoffset(c0);
+    float M2;
+    float M3 : packoffset(c1.y);
+}
+
+// expected-error@+1{{cannot mix packoffset elements with nonpackoffset elements in a cbuffer}}
+cbuffer Mix2
+{
+    float4 M4;
+    float M5 : packoffset(c1.y);
+    float M6 ;
+}
+
+// expected-error@+1{{attribute 'packoffset' only applies to cbuffer constant}}
+float4 g : packoffset(c0);
+
+cbuffer IllegalOffset
+{
+    // expected-error@+1{{invalid resource class specifier 't2' for packoffset, expected 'c'}}
+    float4 i1 : packoffset(t2);
+    // expected-error@+1{{invalid component 'm' used; expected 'x', 'y', 'z', or 'w'}}
+    float i2 : packoffset(c1.m);
+}
+
+cbuffer Overlap
+{
+    float4 o1 : packoffset(c0);
+    // expected-error@+1{{packoffset overlap between 'o2', 'o1'}}
+    float2 o2 : packoffset(c0.z);
+}
+
+cbuffer CrossReg
+{
+    // expected-error@+1{{packoffset cannot cross register boundary}}
+    float4 c1 : packoffset(c0.y);
+    // expected-error@+1{{packoffset cannot cross register boundary}}
+    float2 c2 : packoffset(c1.w);
+}
+
+struct ST {
+  float s;
+};
+
+cbuffer Aggregate
+{
+    // expected-error@+1{{packoffset cannot cross register boundary}}
+    ST A1 : packoffset(c0.y);
+    // expected-error@+1{{packoffset cannot cross register boundary}}
+    float A2[2] : packoffset(c1.w);
+}

llvmbot · 2024-04-23T21:51:23Z

@llvm/pr-subscribers-clang

Author: Xiang Li (python3kgae)

Changes

Add HLSLPackOffsetAttr to save packoffset in AST.

Since we have to parse the attribute manually in ParseHLSLAnnotations, we could create the ParsedAttribute with a integer offset parameter instead of string. This approach avoids parsing the string if the offset is saved as a string in HLSLPackOffsetAttr.

For #57914.

Full diff: https://github.com/llvm/llvm-project/pull/89836.diff

9 Files Affected:

(modified) clang/include/clang/Basic/Attr.td (+7)
(modified) clang/include/clang/Basic/AttrDocs.td (+18)
(modified) clang/include/clang/Basic/DiagnosticParseKinds.td (+2)
(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3)
(modified) clang/lib/Parse/ParseHLSL.cpp (+80)
(modified) clang/lib/Sema/SemaDeclAttr.cpp (+44)
(modified) clang/lib/Sema/SemaHLSL.cpp (+48)
(added) clang/test/AST/HLSL/packoffset.hlsl (+16)
(added) clang/test/SemaHLSL/packoffset-invalid.hlsl (+55)

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 4408d517e70e58..d3d006ed9633f4 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -4372,6 +4372,13 @@ def HLSLResourceBinding: InheritableAttr {
   let Documentation = [HLSLResourceBindingDocs];
 }
 
+def HLSLPackOffset: HLSLAnnotationAttr {
+  let Spellings = [HLSLAnnotation<"packoffset">];
+  let LangOpts = [HLSL];
+  let Args = [IntArgument<"Offset">];
+  let Documentation = [HLSLPackOffsetDocs];
+}
+
 def HLSLSV_DispatchThreadID: HLSLAnnotationAttr {
   let Spellings = [HLSLAnnotation<"SV_DispatchThreadID">];
   let Subjects = SubjectList<[ParmVar, Field]>;
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index a0bbe5861c5722..6bc7813bd43cb4 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -7398,6 +7398,24 @@ The full documentation is available here: https://docs.microsoft.com/en-us/windo
   }];
 }
 
+def HLSLPackOffsetDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+The packoffset attribute is used to change the layout of a cbuffer.
+Attribute spelling in HLSL is: ``packoffset(c[Subcomponent[.component]])``.
+A subcomponent is a register number, which is an integer. A component is in the form of [.xyzw].
+
+Here're packoffset examples with and without component:
+.. code-block:: c++
+  cbuffer A {
+    float3 a : packoffset(c0.y);
+    float4 b : packoffset(c4);
+  }
+
+The full documentation is available here: https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-variable-packoffset
+  }];
+}
+
 def HLSLSV_DispatchThreadIDDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 38174cf3549f14..81433ee79d48b2 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1745,5 +1745,7 @@ def err_hlsl_separate_attr_arg_and_number : Error<"wrong argument format for hls
 def ext_hlsl_access_specifiers : ExtWarn<
   "access specifiers are a clang HLSL extension">,
   InGroup<HLSLExtension>;
+def err_hlsl_unsupported_component : Error<"invalid component '%0' used; expected 'x', 'y', 'z', or 'w'">;
+def err_hlsl_packoffset_invalid_reg : Error<"invalid resource class specifier '%0' for packoffset, expected 'c'">;
 
 } // end of Parser diagnostics
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 63e951daec7477..bde9617c9820a8 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12167,6 +12167,9 @@ def err_hlsl_init_priority_unsupported : Error<
 def err_hlsl_unsupported_register_type : Error<"invalid resource class specifier '%0' used; expected 'b', 's', 't', or 'u'">;
 def err_hlsl_unsupported_register_number : Error<"register number should be an integer">;
 def err_hlsl_expected_space : Error<"invalid space specifier '%0' used; expected 'space' followed by an integer, like space1">;
+def err_hlsl_packoffset_mix : Error<"cannot mix packoffset elements with nonpackoffset elements in a cbuffer">;
+def err_hlsl_packoffset_overlap : Error<"packoffset overlap between %0, %1">;
+def err_hlsl_packoffset_cross_reg_boundary : Error<"packoffset cannot cross register boundary">;
 def err_hlsl_pointers_unsupported : Error<
   "%select{pointers|references}0 are unsupported in HLSL">;
 
diff --git a/clang/lib/Parse/ParseHLSL.cpp b/clang/lib/Parse/ParseHLSL.cpp
index f4cbece31f1810..eac4876ccab49a 100644
--- a/clang/lib/Parse/ParseHLSL.cpp
+++ b/clang/lib/Parse/ParseHLSL.cpp
@@ -183,6 +183,86 @@ void Parser::ParseHLSLAnnotations(ParsedAttributes &Attrs,
       return;
     }
   } break;
+  case ParsedAttr::AT_HLSLPackOffset: {
+    // Parse 'packoffset( c[Subcomponent][.component] )'.
+    // Check '('.
+    if (ExpectAndConsume(tok::l_paren, diag::err_expected_lparen_after)) {
+      SkipUntil(tok::r_paren, StopAtSemi); // skip through )
+      return;
+    }
+    // Check c[Subcomponent] as an identifier.
+    if (!Tok.is(tok::identifier)) {
+      Diag(Tok.getLocation(), diag::err_expected) << tok::identifier;
+      SkipUntil(tok::r_paren, StopAtSemi); // skip through )
+      return;
+    }
+    StringRef OffsetStr = Tok.getIdentifierInfo()->getName();
+    SourceLocation OffsetLoc = Tok.getLocation();
+    if (OffsetStr[0] != 'c') {
+      Diag(Tok.getLocation(), diag::err_hlsl_packoffset_invalid_reg)
+          << OffsetStr;
+      SkipUntil(tok::r_paren, StopAtSemi); // skip through )
+      return;
+    }
+    OffsetStr = OffsetStr.substr(1);
+    unsigned SubComponent = 0;
+    if (!OffsetStr.empty()) {
+      // Make sure SubComponent is a number.
+      if (OffsetStr.getAsInteger(10, SubComponent)) {
+        Diag(OffsetLoc.getLocWithOffset(1),
+             diag::err_hlsl_unsupported_register_number);
+        return;
+      }
+    }
+    unsigned Component = 0;
+    ConsumeToken(); // consume identifier.
+    if (Tok.is(tok::period)) {
+      ConsumeToken(); // consume period.
+      if (!Tok.is(tok::identifier)) {
+        Diag(Tok.getLocation(), diag::err_expected) << tok::identifier;
+        SkipUntil(tok::r_paren, StopAtSemi); // skip through )
+        return;
+      }
+      StringRef ComponentStr = Tok.getIdentifierInfo()->getName();
+      SourceLocation SpaceLoc = Tok.getLocation();
+      ConsumeToken(); // consume identifier.
+      // Make sure Component is a single character.
+      if (ComponentStr.size() != 1) {
+        Diag(SpaceLoc, diag::err_hlsl_unsupported_component) << ComponentStr;
+        SkipUntil(tok::r_paren, StopAtSemi); // skip through )
+        return;
+      } else {
+        switch (ComponentStr[0]) {
+        case 'x':
+          Component = 0;
+          break;
+        case 'y':
+          Component = 1;
+          break;
+        case 'z':
+          Component = 2;
+          break;
+        case 'w':
+          Component = 3;
+          break;
+        default:
+          Diag(SpaceLoc, diag::err_hlsl_unsupported_component) << ComponentStr;
+          SkipUntil(tok::r_paren, StopAtSemi); // skip through )
+          return;
+        }
+      }
+    }
+    unsigned Offset = SubComponent * 4 + Component;
+    ASTContext &Ctx = Actions.getASTContext();
+    ArgExprs.push_back(IntegerLiteral::Create(
+        Ctx, llvm::APInt(Ctx.getTypeSize(Ctx.getSizeType()), Offset),
+        Ctx.getSizeType(),
+        SourceLocation())); // Dummy location for integer literal.
+    if (ExpectAndConsume(tok::r_paren, diag::err_expected)) {
+      SkipUntil(tok::r_paren, StopAtSemi); // skip through )
+      return;
+    }
+  } break;
   case ParsedAttr::UnknownAttribute:
     Diag(Loc, diag::err_unknown_hlsl_semantic) << II;
     return;
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 363ae93cb62df1..373f2e8f50cdb5 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -7314,6 +7314,47 @@ static void handleHLSLSV_DispatchThreadIDAttr(Sema &S, Decl *D,
   D->addAttr(::new (S.Context) HLSLSV_DispatchThreadIDAttr(S.Context, AL));
 }
 
+static void handleHLSLPackOffsetAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  if (!isa<VarDecl>(D)) {
+    S.Diag(AL.getLoc(), diag::err_hlsl_attr_invalid_ast_node)
+        << AL << "cbuffer constant";
+    return;
+  }
+  auto *BufDecl = dyn_cast<HLSLBufferDecl>(D->getDeclContext());
+  if (!BufDecl) {
+    S.Diag(AL.getLoc(), diag::err_hlsl_attr_invalid_ast_node)
+        << AL << "cbuffer constant";
+    return;
+  }
+
+  uint32_t Offset;
+  if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Offset))
+    return;
+
+  QualType T = cast<VarDecl>(D)->getType().getCanonicalType();
+  // Check if T is an array or struct type.
+  // TODO: mark matrix type as aggregate type.
+  bool IsAggregateTy = (T->isArrayType() || T->isStructureType());
+
+  unsigned ComponentNum = Offset & 0x3;
+  // Check ComponentNum is valid for T.
+  if (IsAggregateTy) {
+    if (ComponentNum != 0) {
+      S.Diag(AL.getLoc(), diag::err_hlsl_packoffset_cross_reg_boundary);
+      return;
+    }
+  } else {
+    unsigned size = S.getASTContext().getTypeSize(T);
+    // Make sure ComponentNum + sizeof(T) <= 4.
+    if ((ComponentNum * 32 + size) > 128) {
+      S.Diag(AL.getLoc(), diag::err_hlsl_packoffset_cross_reg_boundary);
+      return;
+    }
+  }
+
+  D->addAttr(::new (S.Context) HLSLPackOffsetAttr(S.Context, AL, Offset));
+}
+
 static void handleHLSLShaderAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   StringRef Str;
   SourceLocation ArgLoc;
@@ -9735,6 +9776,9 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
   case ParsedAttr::AT_HLSLSV_DispatchThreadID:
     handleHLSLSV_DispatchThreadIDAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_HLSLPackOffset:
+    handleHLSLPackOffsetAttr(S, D, AL);
+    break;
   case ParsedAttr::AT_HLSLShader:
     handleHLSLShaderAttr(S, D, AL);
     break;
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index bb9e37f18d370c..fa62cab54e6902 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -42,6 +42,54 @@ Decl *SemaHLSL::ActOnStartBuffer(Scope *BufferScope, bool CBuffer,
 void SemaHLSL::ActOnFinishBuffer(Decl *Dcl, SourceLocation RBrace) {
   auto *BufDecl = cast<HLSLBufferDecl>(Dcl);
   BufDecl->setRBraceLoc(RBrace);
+
+  // Validate packoffset.
+  llvm::SmallVector<std::pair<VarDecl *, HLSLPackOffsetAttr *>> PackOffsetVec;
+  bool HasPackOffset = false;
+  bool HasNonPackOffset = false;
+  for (auto *Field : BufDecl->decls()) {
+    VarDecl *Var = dyn_cast<VarDecl>(Field);
+    if (!Var)
+      continue;
+    if (Field->hasAttr<HLSLPackOffsetAttr>()) {
+      PackOffsetVec.emplace_back(Var, Field->getAttr<HLSLPackOffsetAttr>());
+      HasPackOffset = true;
+    } else {
+      HasNonPackOffset = true;
+    }
+  }
+
+  if (HasPackOffset && HasNonPackOffset) {
+    Diag(BufDecl->getLocation(), diag::err_hlsl_packoffset_mix);
+  } else if (HasPackOffset) {
+    ASTContext &Context = getASTContext();
+    // Make sure no overlap in packoffset.
+    llvm::SmallDenseMap<VarDecl *, std::pair<unsigned, unsigned>>
+        PackOffsetRanges;
+    for (auto &Pair : PackOffsetVec) {
+      VarDecl *Var = Pair.first;
+      HLSLPackOffsetAttr *Attr = Pair.second;
+      unsigned Size = Context.getTypeSize(Var->getType());
+      unsigned Begin = Attr->getOffset() * 32;
+      unsigned End = Begin + Size;
+      for (auto &Range : PackOffsetRanges) {
+        VarDecl *OtherVar = Range.first;
+        unsigned OtherBegin = Range.second.first;
+        unsigned OtherEnd = Range.second.second;
+        if (Begin < OtherEnd && OtherBegin < Begin) {
+          Diag(Var->getLocation(), diag::err_hlsl_packoffset_overlap)
+              << Var << OtherVar;
+          break;
+        } else if (OtherBegin < End && Begin < OtherBegin) {
+          Diag(Var->getLocation(), diag::err_hlsl_packoffset_overlap)
+              << Var << OtherVar;
+          break;
+        }
+      }
+      PackOffsetRanges[Var] = std::make_pair(Begin, End);
+    }
+  }
+
   SemaRef.PopDeclContext();
 }
 
diff --git a/clang/test/AST/HLSL/packoffset.hlsl b/clang/test/AST/HLSL/packoffset.hlsl
new file mode 100644
index 00000000000000..d3cf798c995758
--- /dev/null
+++ b/clang/test/AST/HLSL/packoffset.hlsl
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -triple dxil-unknown-shadermodel6.3-library -S -finclude-default-header  -ast-dump  -x hlsl %s | FileCheck %s
+
+
+// CHECK: HLSLBufferDecl {{.*}} cbuffer A
+cbuffer A
+{
+    // CHECK-NEXT: VarDecl {{.*}} C1 'float4'
+    // CHECK-NEXT: HLSLPackOffsetAttr {{.*}} 0
+    float4 C1 : packoffset(c);
+    // CHECK-NEXT: VarDecl {{.*}} col:11 C2 'float'
+    // CHECK-NEXT: HLSLPackOffsetAttr {{.*}} 4
+    float C2 : packoffset(c1);
+    // CHECK-NEXT: VarDecl {{.*}} col:11 C3 'float'
+    // CHECK-NEXT: HLSLPackOffsetAttr {{.*}} 5
+    float C3 : packoffset(c1.y);
+}
diff --git a/clang/test/SemaHLSL/packoffset-invalid.hlsl b/clang/test/SemaHLSL/packoffset-invalid.hlsl
new file mode 100644
index 00000000000000..a2c7bb5a1e05cd
--- /dev/null
+++ b/clang/test/SemaHLSL/packoffset-invalid.hlsl
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -finclude-default-header -triple dxil-pc-shadermodel6.3-library -verify %s
+
+// expected-error@+1{{cannot mix packoffset elements with nonpackoffset elements in a cbuffer}}
+cbuffer Mix
+{
+    float4 M1 : packoffset(c0);
+    float M2;
+    float M3 : packoffset(c1.y);
+}
+
+// expected-error@+1{{cannot mix packoffset elements with nonpackoffset elements in a cbuffer}}
+cbuffer Mix2
+{
+    float4 M4;
+    float M5 : packoffset(c1.y);
+    float M6 ;
+}
+
+// expected-error@+1{{attribute 'packoffset' only applies to cbuffer constant}}
+float4 g : packoffset(c0);
+
+cbuffer IllegalOffset
+{
+    // expected-error@+1{{invalid resource class specifier 't2' for packoffset, expected 'c'}}
+    float4 i1 : packoffset(t2);
+    // expected-error@+1{{invalid component 'm' used; expected 'x', 'y', 'z', or 'w'}}
+    float i2 : packoffset(c1.m);
+}
+
+cbuffer Overlap
+{
+    float4 o1 : packoffset(c0);
+    // expected-error@+1{{packoffset overlap between 'o2', 'o1'}}
+    float2 o2 : packoffset(c0.z);
+}
+
+cbuffer CrossReg
+{
+    // expected-error@+1{{packoffset cannot cross register boundary}}
+    float4 c1 : packoffset(c0.y);
+    // expected-error@+1{{packoffset cannot cross register boundary}}
+    float2 c2 : packoffset(c1.w);
+}
+
+struct ST {
+  float s;
+};
+
+cbuffer Aggregate
+{
+    // expected-error@+1{{packoffset cannot cross register boundary}}
+    ST A1 : packoffset(c0.y);
+    // expected-error@+1{{packoffset cannot cross register boundary}}
+    float A2[2] : packoffset(c1.w);
+}

clang/include/clang/Basic/AttrDocs.td

clang/lib/Parse/ParseHLSL.cpp

clang/lib/Sema/SemaDeclAttr.cpp

clang/lib/Sema/SemaHLSL.cpp

hekota

LGTM!

llvm-beanz · 2024-04-29T14:58:06Z

clang/include/clang/Basic/AttrDocs.td

+  let Content = [{
+The packoffset attribute is used to change the layout of a cbuffer.
+Attribute spelling in HLSL is: ``packoffset( c[Subcomponent][.component] )``.
+A subcomponent is a register number, which is an integer. A component is in the form of [.xyzw].


What about .rgba? https://godbolt.org/z/z814668Pd

llvm-beanz · 2024-04-29T15:02:18Z

clang/test/AST/HLSL/packoffset.hlsl

+    float C2 : packoffset(c1);
+    // CHECK-NEXT: VarDecl {{.*}} col:11 C3 'float'
+    // CHECK-NEXT: HLSLPackOffsetAttr {{.*}} 5
+    float C3 : packoffset(c1.y);


How about some valid cases for vector and array cbuffer elements?

llvm-beanz · 2024-04-29T15:02:20Z

clang/test/SemaHLSL/packoffset-invalid.hlsl

+    ST A1 : packoffset(c0.y);
+    // expected-error@+1{{packoffset cannot cross register boundary}}
+    float A2[2] : packoffset(c1.w);
+}


Some additional error cases to consider:

// Some parsing fails packoffset(); packoffset((c0)); packoffset(c0, x); packoffset(c.x); packoffset; packoffset(; packoffset); packoffset c0.x; // Some other odd failures that look more real packoffset(c0.xy); packoffset(c0.rg); packoffset(c0.yes); packoffset(c0.woo); packoffset(c0.xr);

llvm-beanz · 2024-04-29T15:07:58Z

clang/lib/Parse/ParseHLSL.cpp

+    ArgExprs.push_back(IntegerLiteral::Create(
+        Ctx, llvm::APInt(Ctx.getTypeSize(Ctx.getSizeType()), Offset),
+        Ctx.getSizeType(),
+        SourceLocation())); // Dummy location for integer literal.


Why not give this the location of the parsed identifier?

llvm-beanz · 2024-04-29T15:11:36Z

clang/lib/Sema/SemaDeclAttr.cpp

+static void handleHLSLPackOffsetAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  if (!isa<VarDecl>(D) || !isa<HLSLBufferDecl>(D->getDeclContext())) {
+    S.Diag(AL.getLoc(), diag::err_hlsl_attr_invalid_ast_node)
+        << AL << "cbuffer constant";


Maybe:

Suggested change

<< AL << "cbuffer constant";

<< AL << "cbuffer member variable";

"cbuffer constant" is less clear to me because the variable declaration isn't actually marked constant in the source, it is just implicitly constant because it is in a cbuffer.

llvm-beanz · 2024-04-29T15:15:25Z

clang/test/AST/HLSL/packoffset.hlsl

+// CHECK: HLSLBufferDecl {{.*}} cbuffer A
+cbuffer A
+{
+    // CHECK-NEXT: VarDecl {{.*}} C1 'float4'


We could use some tests for double and half scalars, arrays and vectors.

Support rgba as component. Add calculateLegacyCbufferSize to get correct size when check overlap.

llvm-beanz · 2024-05-01T18:40:30Z

clang/include/clang/Basic/Attr.td

+def HLSLPackOffset: HLSLAnnotationAttr {
+  let Spellings = [HLSLAnnotation<"packoffset">];
+  let LangOpts = [HLSL];
+  let Args = [IntArgument<"Offset">];


Looking at this more this seems really weird to me. The argument isn't actually an integer, it's a special string. You're parsing it to an integer, but that actually loses some fidelity.

We should probably store index and sub-component values in the attribute rather than collapsing it down into one value.

Do you mind the index and sub-component value store as integer or string?

Information lost is the difference between 'c0' and 'c', 'xyzw' and 'rgba'.
If we want to keep that, they'll need to be kept these as string.
Then we'll need to parse the string when using the value as integer.

I think it is fine to keep these as integers.

The feedback here still isn't resolved. The argument is an IdentifierArgument with special parsing handling, it isn't an integer. We can store the parsed integer values in the attribute but we shouldn't be calling the argument an IntArgument.

Of if we do, we should have separate int arguments for the row and column offsets.

damyanp

Apologies, it looks like I forgot to submit this review! Hopefully the comments aren't too out of date.

clang/include/clang/Basic/AttrDocs.td

clang/include/clang/Basic/DiagnosticParseKinds.td

…rence doc.

clang/docs/HLSL/ExpectedDifferences.rst

llvm-beanz

I'm marking this as requesting changes because I don't think we should land this as-is.

python3kgae · 2024-05-06T22:55:15Z

I'm marking this as requesting changes because I don't think we should land this as-is.

Switched to warning.

This reverts commit c5509fe.

…"" This reverts commit 5809281.

This reapplies c5509fe "[HLSL] Support packoffset attribute in AST (#89836)" with a fix for the test failure caused by missing -fnative-half-type. Since we have to parse the attribute manually in ParseHLSLAnnotations, we could create the ParsedAttribute with an integer offset parameter instead of string. This approach avoids parsing the string if the offset is saved as a string in HLSLPackOffsetAttr. For #57914

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" HLSL HLSL Language Support labels Apr 23, 2024

python3kgae removed clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 23, 2024

Fix doc format.

c356db6

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 24, 2024

hekota requested changes Apr 25, 2024

View reviewed changes

Update per comment.

2e406f5

hekota approved these changes Apr 25, 2024

View reviewed changes

llvm-beanz reviewed Apr 29, 2024

View reviewed changes

python3kgae added 2 commits May 1, 2024 12:28

Check double alignment.

228aca7

Support rgba as component. Add calculateLegacyCbufferSize to get correct size when check overlap.

Update error message.

72df629

llvm-beanz reviewed May 1, 2024

View reviewed changes

damyanp requested changes May 1, 2024

View reviewed changes

clang/include/clang/Basic/AttrDocs.td Outdated Show resolved Hide resolved

clang/include/clang/Basic/DiagnosticParseKinds.td Show resolved Hide resolved

Add the difference for mix packoffset and non-packoffset to the diffe…

1e1a8ce

…rence doc.

damyanp approved these changes May 1, 2024

View reviewed changes

llvm-beanz reviewed May 1, 2024

View reviewed changes

clang/docs/HLSL/ExpectedDifferences.rst Outdated Show resolved Hide resolved

llvm-beanz requested changes May 1, 2024

View reviewed changes

Change to error on mix into warning.

932e748

damyanp approved these changes May 6, 2024

View reviewed changes

Switch to save 2 int args.

db23da0

llvm-beanz approved these changes May 7, 2024

View reviewed changes

python3kgae added 2 commits May 7, 2024 13:23

Remove the expected difference.

81f1b39

Add warning group for mix packoffset.

4b6cc97

python3kgae merged commit c5509fe into llvm:main May 8, 2024
5 checks passed

python3kgae deleted the packoffset branch May 8, 2024 12:26

python3kgae added a commit that referenced this pull request May 8, 2024

Revert "[HLSL] Support packoffset attribute in AST (#89836)" (#91473)

9c09b08

This reverts commit c5509fe.

python3kgae added a commit to python3kgae/llvm-project that referenced this pull request May 8, 2024

Revert "Revert "[HLSL] Support packoffset attribute in AST (llvm#89836)…

251bbfb

…"" This reverts commit 5809281.

bogner mentioned this pull request May 8, 2024

[HLSL] reenable add packoffset in AST #91474

Merged

	<< AL << "cbuffer constant";
	<< AL << "cbuffer member variable";

[HLSL] Support packoffset attribute in AST #89836

[HLSL] Support packoffset attribute in AST #89836

Uh oh!

Conversation

python3kgae commented Apr 23, 2024 • edited by damyanp Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 23, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hekota left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

python3kgae May 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llvm-beanz May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damyanp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llvm-beanz left a comment

Choose a reason for hiding this comment

Uh oh!

python3kgae commented May 6, 2024

Uh oh!

Uh oh!

Uh oh!

python3kgae commented Apr 23, 2024 •

edited by damyanp

Loading

llvmbot commented Apr 23, 2024 •

edited

Loading

hekota left a comment •

edited

Loading

python3kgae May 1, 2024 •

edited

Loading

llvm-beanz May 7, 2024 •

edited

Loading