[HLSL] Fix MSFT Attribute parsing, add numthreads

[beanz]

Summary

HLSL uses Microsoft-style attributes [attr], which clang mostly
ignores. For HLSL we need to handle known Microsoft attributes, and to
maintain C/C++ as-is we ignore unknown attributes.

To utilize this new code path, this change adds the HLSL numthreads
attribute.

[pow2clk]

Looks good. I made a comment about the CS 4.0 thing, but that can be addressed when more thorough examination of the requested shader models comes around.

clang/lib/Sema/SemaDeclAttr.cpp

6857

As we discussed, compute shaders didn't actually exist pre-4.0 and I don't know that we'll ever seek to do anything with a 4.0 shader model except convert it to a higher version and issue a notification, but that handling is clearly not the intent of this change. So it's fine with me.

[rnk]

lgtm

clang/include/clang/Basic/AttrDocs.td

6377

I think it would be fair to link to the Microsoft docs here:
https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/sm5-attributes-numthreads

We've done this elsewhere for Microsoft attributes.

clang/lib/Sema/SemaDecl.cpp

11326

Style nit: comments should use follow normal capitalization and punctuation rules: "When writing comments, write them as English prose, using proper capitalization, punctuation, etc."
https://llvm.org/docs/CodingStandards.html#commenting

[aaron.ballman]

clang/test/SemaHLSL/num_threads.hlsl

48

There are a few test cases that are missing here:

1. Writing the attribute on the wrong subject.
2. Passing no args to the attribute, passing too many args to the attribute.
3. Function merging where the redeclaration has attributes that don't match. e.g.,

[numthreads(1, 2, 3)] void func();
[numthreads(4, 5, 6)] void func() {}

4. Should the user be able to pick the values via NTTP or other constant expressions? e.g.,

template <int X, int Y, int Z>
[numthreads(X, Y, Z)] void func();

constexpr int foo();
[numthreads(foo(), 2, 3)] void other_func();

5. Can you write this attribute on a member function? Can the member function be virtual?

[beanz]

clang/test/SemaHLSL/num_threads.hlsl

48

Great catches!

1-3 are definitely missing coverage. I'll add additional coverage. I also should add a test case where a value is a preprocessor macro.

For 4 & 5, the short answer is that HLSL doesn't support those situations, but I think there are two related diagnostics that I should be emitting:

1. Applying the numthreads attribute on a template function should warn (It won't work anyways, but better to issue the diagnostic)
2. Applying the numthreads attribute on a member function should

The point by point explanation is:

4. Constexpr isn't supported at all, and user-defined templates are new to HLSL 2021, can't be used as entry functions. This _could_ be relaxed in the future, but we would need to force instantiation, which probably means some form of extern template support, which we don't have in the language yet.
5. Member functions also can't be used as entry functions, and HLSL doesn't support anything virtual.

I'll update the tests and diagnostics today.

[beanz]

@aaron.ballman I pushed updates in rGff6696c842ba.

The one bit we discussed that I didn't do anything for is the template case. Neither clang or the HLSL compiler support parsing Microsoft attributes on templates right now, so the parser falls over and errors. It would be better to have a nicer diagnostic, but since the current state matches our compiler I left it as-is.

Thank you!

[aaron.ballman]

In D122627#3417495, @beanz wrote:

@aaron.ballman I pushed updates in rGff6696c842ba.

The one bit we discussed that I didn't do anything for is the template case. Neither clang or the HLSL compiler support parsing Microsoft attributes on templates right now, so the parser falls over and errors. It would be better to have a nicer diagnostic, but since the current state matches our compiler I left it as-is.

That sounds reasonable to me.

Thank you!

Thanks for the post-commit changes! One question on them:

def GlobalFunction
    : SubsetSubject<Function, [{S->isGlobal()}], "global functions">;

Are you sure that's what you want? This returns true for a static C++ member function, false for a static free function, and false for within an unnamed namespace, and true otherwise.

Also, I didn't see any new test coverage for function merging behavior.

[beanz]

In D122627#3417557, @aaron.ballman wrote:

Are you sure that's what you want? This returns true for a static C++ member function, false for a static free function, and false for within an unnamed namespace, and true otherwise.

You're right this isn't quite right, but getting closer... HLSL doesn't support unnamed namespaces, and we only support static free functions by accident... (the current compiler ignores static on free functions).

This actually revealed some gaps in the documentation for HLSL, I've gone back and gotten feedback from my team's HLSL expert and I think I've got the right set of constraints for where this can be applied now (clang will even have this better than the HLSL compiler).

Also, I didn't see any new test coverage for function merging behavior.

Doh! I knew I was forgetting something. Juggling too many balls today. I'll get that covered too!

[aaron.ballman]

In D122627#3417919, @beanz wrote:

In D122627#3417557, @aaron.ballman wrote:

Are you sure that's what you want? This returns true for a static C++ member function, false for a static free function, and false for within an unnamed namespace, and true otherwise.

You're right this isn't quite right, but getting closer... HLSL doesn't support unnamed namespaces, and we only support static free functions by accident... (the current compiler ignores static on free functions).

This actually revealed some gaps in the documentation for HLSL, I've gone back and gotten feedback from my team's HLSL expert and I think I've got the right set of constraints for where this can be applied now (clang will even have this better than the HLSL compiler).

SGTM, thanks for digging into it!

Also, I didn't see any new test coverage for function merging behavior.

Doh! I knew I was forgetting something. Juggling too many balls today. I'll get that covered too!

No worries, thanks for following up on it. :-)

[beanz]

I just pushed one more commit rG19054163e11a6632b4973c936e5aa93ec742c866 that further refines the subject specification and covers the merging attributes and conflicting attributes on the same decl.

Thank you so much for the spectacular review feedback!

clang/include/clang/Basic/Attr.td

 def OpenCL : LangOpt<"OpenCL">;
 def RenderScript : LangOpt<"RenderScript">;
 def ObjC : LangOpt<"ObjC">;
 def BlocksSupported : LangOpt<"Blocks">;
 def ObjCAutoRefCount : LangOpt<"ObjCAutoRefCount">;
 def ObjCNonFragileRuntime
     : LangOpt<"", "LangOpts.ObjCRuntime.allowsClassStubs()">;
 
+def HLSL : LangOpt<"HLSL">;
+
 // Language option for CMSE extensions
 def Cmse : LangOpt<"Cmse">;
 
 // Defines targets for target-specific attributes. Empty lists are unchecked.
 class TargetSpec {
   // Specifies Architectures for which the target applies, based off the
   // ArchType enumeration in Triple.h.
   list<string> Arches = [];

 def Error : InheritableAttr {
   let Spellings = [GCC<"error">, GCC<"warning">];
   let Accessors = [Accessor<"isError", [GCC<"error">]>,
                    Accessor<"isWarning", [GCC<"warning">]>];
   let Args = [StringArgument<"UserDiagnostic">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [ErrorAttrDocs];
 }
+
+def HLSLNumThreads: InheritableAttr {
+  let Spellings = [Microsoft<"numthreads">];
+  let Args = [IntArgument<"X">, IntArgument<"Y">, IntArgument<"Z">];
+  let Subjects = SubjectList<[Function]>;
+  let LangOpts = [HLSL];
+  let Documentation = [NumThreadsDocs];
+}

clang/include/clang/Basic/AttrDocs.td

- ``all-gpr-arg`` zeros all call-used GPR registers used to pass arguments to
   the function.
 
 The default for the attribute is contolled by the ``-fzero-call-used-regs``
 flag.
 
 .. _Return-Oriented Programming: https://en.wikipedia.org/wiki/Return-oriented_programming
   }];
 }
+
+def NumThreadsDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+The ``numthreads`` attribute applies to HLSL shaders where explcit thread counts
+are required. The ``X``, ``Y``, and ``Z`` values provided to the attribute
+dictate the thread id. Total number of threads executed is ``X * Y * Z``.
+
+The full documentation is available here: https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/sm5-attributes-numthreads
+  }];
+}

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_riscv_builtin_requires_extension : Error<
   "builtin requires at least one of the following extensions support to be enabled : %0">;
 def err_riscv_builtin_invalid_lmul : Error<
   "LMUL argument must be in the range [0,3] or [5,7]">;
 
 def err_std_source_location_impl_not_found : Error<
   "'std::source_location::__impl' was not found; it must be defined before '__builtin_source_location' is called">;
 def err_std_source_location_impl_malformed : Error<
   "'std::source_location::__impl' must be standard-layout and have only two 'const char *' fields '_M_file_name' and '_M_function_name', and two integral fields '_M_line' and '_M_column'">;
+
+// HLSL Diagnostics
+def err_hlsl_attr_unsupported_in_stage : Error<"attribute %0 is unsupported in %select{Pixel|Vertex|Geometry|Hull|Domain|Compute|Library|RayGeneration|Intersection|AnyHit|ClosestHit|Miss|Callable|Mesh|Amplification|Invalid}1 shaders, requires %2">;
+
+def err_hlsl_numthreads_argument_oor : Error<"argument '%select{X|Y|Z}0' to numthreads attribute cannot exceed %1">;
+def err_hlsl_numthreads_invalid : Error<"total number of threads cannot exceed %0">;
+
 } // end of sema component.
+

clang/include/clang/Parse/Parser.h

bool ParseCXX11AttributeArgs(IdentifierInfo *AttrName,
                                CachedTokens &OpenMPTokens);
 
   IdentifierInfo *TryParseCXX11AttributeIdentifier(
       SourceLocation &Loc,
       Sema::AttributeCompletion Completion = Sema::AttributeCompletion::None,
       const IdentifierInfo *EnclosingScope = nullptr);
 
   void MaybeParseMicrosoftAttributes(ParsedAttributes &Attrs) {
-    if (getLangOpts().MicrosoftExt && Tok.is(tok::l_square)) {
+    if ((getLangOpts().MicrosoftExt || getLangOpts().HLSL) &&
+        Tok.is(tok::l_square)) {
       ParsedAttributes AttrsWithRange(AttrFactory);
       ParseMicrosoftAttributes(AttrsWithRange);
       Attrs.takeAllFrom(AttrsWithRange);
     }
   }
   void ParseMicrosoftUuidAttributeArgs(ParsedAttributes &Attrs);
   void ParseMicrosoftAttributes(ParsedAttributes &Attrs);
   bool MaybeParseMicrosoftDeclSpecs(ParsedAttributes &Attrs) {

clang/lib/Parse/ParseDeclCXX.cpp

bool Parser::ParseCXX11AttributeArgs(IdentifierInfo *AttrName,
                                      SourceLocation ScopeLoc,
                                      CachedTokens &OpenMPTokens) {
   assert(Tok.is(tok::l_paren) && "Not a C++11 attribute argument list");
   SourceLocation LParenLoc = Tok.getLocation();
   const LangOptions &LO = getLangOpts();
   ParsedAttr::Syntax Syntax =
       LO.CPlusPlus ? ParsedAttr::AS_CXX11 : ParsedAttr::AS_C2x;
 
+  // Try parsing microsoft attributes
+  if (getLangOpts().MicrosoftExt || getLangOpts().HLSL) {
+    if (hasAttribute(AttrSyntax::Microsoft, ScopeName, AttrName,
+                     getTargetInfo(), getLangOpts()))
+      Syntax = ParsedAttr::AS_Microsoft;
+  }
+
   // If the attribute isn't known, we will not attempt to parse any
   // arguments.
-  if (!hasAttribute(LO.CPlusPlus ? AttrSyntax::CXX : AttrSyntax::C, ScopeName,
+  if (Syntax != ParsedAttr::AS_Microsoft &&
+      !hasAttribute(LO.CPlusPlus ? AttrSyntax::CXX : AttrSyntax::C, ScopeName,
                     AttrName, getTargetInfo(), getLangOpts())) {
+    if (getLangOpts().MicrosoftExt || getLangOpts().HLSL) {}
     // Eat the left paren, then skip to the ending right paren.
     ConsumeParen();
     SkipUntil(tok::r_paren);
     return false;
   }
 
   if (ScopeName && (ScopeName->isStr("gnu") || ScopeName->isStr("__gnu__"))) {
     // GNU-scoped attributes have some special cases to handle GNU-specific
while (true) {
                                       Sema::AttributeCompletion::Attribute,
                                       /*Scope=*/nullptr);
         break;
       }
       if (Tok.isNot(tok::identifier)) // ']', but also eof
         break;
       if (Tok.getIdentifierInfo()->getName() == "uuid")
         ParseMicrosoftUuidAttributeArgs(Attrs);
-      else
+      else {
+        IdentifierInfo *II = Tok.getIdentifierInfo();
+        SourceLocation NameLoc = Tok.getLocation();
         ConsumeToken();
+        if (Tok.is(tok::l_paren)) {
+          CachedTokens OpenMPTokens;
+          ParseCXX11AttributeArgs(II, NameLoc, Attrs, &EndLoc, nullptr,
+                                  SourceLocation(), OpenMPTokens);
+          ReplayOpenMPAttributeTokens(OpenMPTokens);
+        } // FIXME: handle attributes that don't have arguments
+      }
     }
 
     T.consumeClose();
     EndLoc = T.getCloseLocation();
   } while (Tok.is(tok::l_square));
 
   Attrs.Range = SourceRange(StartLoc, EndLoc);
 }

clang/lib/Sema/SemaDecl.cpp

void Sema::CheckMain(FunctionDecl* FD, const DeclSpec& DS) {
 
   if (getLangOpts().OpenCL) {
     Diag(FD->getLocation(), diag::err_opencl_no_main)
         << FD->hasAttr<OpenCLKernelAttr>();
     FD->setInvalidDecl();
     return;
   }
 
+  // Functions named main in hlsl are default entries, but don't have specific
+  // signatures they are required to conform to.
+  if (getLangOpts().HLSL)
+    return;
+
   QualType T = FD->getType();
   assert(T->isFunctionType() && "function decl is not of function type");
   const FunctionType* FT = T->castAs<FunctionType>();
 
   // Set default calling convention for main()
   if (FT->getCallConv() != CC_C) {
     FT = Context.adjustFunctionType(FT, FT->getExtInfo().withCallingConv(CC_C));
     FD->setType(QualType(FT, 0));

clang/lib/Sema/SemaDeclAttr.cpp

 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Mangle.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/DarwinSDKInfo.h"
+#include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Initialization.h"
static void handleUuidAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (AL.isMicrosoftAttribute()) // Check for [uuid(...)] spelling.
     S.Diag(AL.getLoc(), diag::warn_atl_uuid_deprecated);
 
   UuidAttr *UA = S.mergeUuidAttr(D, AL, OrigStrRef, Guid);
   if (UA)
     D->addAttr(UA);
 }
 
+static void handleHLSLNumThreadsAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  using llvm::Triple;
+  Triple Target = S.Context.getTargetInfo().getTriple();
+  if (!llvm::is_contained({Triple::Compute, Triple::Mesh, Triple::Amplification,
+                           Triple::Library},
+                          Target.getEnvironment())) {
+    uint32_t Pipeline =
+        (uint32_t)S.Context.getTargetInfo().getTriple().getEnvironment() -
+        (uint32_t)llvm::Triple::Pixel;
+    S.Diag(AL.getLoc(), diag::err_hlsl_attr_unsupported_in_stage)
+        << AL << Pipeline << "Compute, Amplification, Mesh or Library";
+    return;
+  }
+
+  llvm::VersionTuple SMVersion = Target.getOSVersion();
+  uint32_t ZMax = 1024;
+  uint32_t ThreadMax = 1024;
+  if (SMVersion.getMajor() <= 4) {
+    ZMax = 1;
+    ThreadMax = 768;
+  } else if (SMVersion.getMajor() == 5) {
+    ZMax = 64;
+    ThreadMax = 1024;
+  }
+
+  uint32_t X;
+  if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), X))
+    return;
+  if (X > 1024) {
+    S.Diag(AL.getArgAsExpr(0)->getExprLoc(),
+           diag::err_hlsl_numthreads_argument_oor) << 0 << 1024;
+    return;
+  }
+  uint32_t Y;
+  if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(1), Y))
+    return;
+  if (Y > 1024) {
+    S.Diag(AL.getArgAsExpr(1)->getExprLoc(),
+           diag::err_hlsl_numthreads_argument_oor) << 1 << 1024;
+    return;
+  }
+  uint32_t Z;
+  if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(2), Z))
+    return;
+  if (Z > ZMax) {
+    S.Diag(AL.getArgAsExpr(2)->getExprLoc(),
+           diag::err_hlsl_numthreads_argument_oor) << 2 << ZMax;
+    return;
+  }
+
+  if (X * Y * Z > ThreadMax) {
+    S.Diag(AL.getLoc(), diag::err_hlsl_numthreads_invalid) << ThreadMax;
+    return;
+  }
+
+  D->addAttr(::new (S.Context) HLSLNumThreadsAttr(S.Context, AL, X, Y, Z));
+}
+
 static void handleMSInheritanceAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (!S.LangOpts.CPlusPlus) {
     S.Diag(AL.getLoc(), diag::err_attribute_not_supported_in_lang)
         << AL << AttributeLangSupport::C;
     return;
   }
   MSInheritanceAttr *IA = S.mergeMSInheritanceAttr(
       D, AL, /*BestCase=*/true, (MSInheritanceModel)AL.getSemanticSpelling());
case ParsedAttr::AT_Uuid:
     handleUuidAttr(S, D, AL);
     break;
   case ParsedAttr::AT_MSInheritance:
     handleMSInheritanceAttr(S, D, AL);
     break;
   case ParsedAttr::AT_Thread:
     handleDeclspecThreadAttr(S, D, AL);
     break;
   
+  // HLSL attributes:
+  case ParsedAttr::AT_HLSLNumThreads:
+    handleHLSLNumThreadsAttr(S, D, AL);
+    break;
+
   case ParsedAttr::AT_AbiTag:
     handleAbiTagAttr(S, D, AL);
     break;
   case ParsedAttr::AT_CFGuard:
     handleCFGuardAttr(S, D, AL);
     break;
 
   // Thread safety attributes:

clang/test/SemaHLSL/lit.local.cfg

+config.suffixes = ['.hlsl']

clang/test/SemaHLSL/num_threads.hlsl

+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -ast-dump -o - %s | FileCheck %s 
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-mesh -x hlsl -ast-dump -o - %s | FileCheck %s 
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-amplification -x hlsl -ast-dump -o - %s | FileCheck %s 
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-library -x hlsl -ast-dump -o - %s | FileCheck %s 
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-pixel -x hlsl -ast-dump -o - %s -verify
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-vertex -x hlsl -ast-dump -o - %s -verify
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-hull -x hlsl -ast-dump -o - %s -verify
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-domain -x hlsl -ast-dump -o - %s -verify
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -ast-dump -o - %s -DFAIL -verify
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel5.0-compute -x hlsl -ast-dump -o - %s -DFAIL -verify
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel4.0-compute -x hlsl -ast-dump -o - %s -DFAIL -verify
+
+#if __SHADER_TARGET_STAGE == __SHADER_STAGE_COMPUTE || __SHADER_TARGET_STAGE == __SHADER_STAGE_MESH || __SHADER_TARGET_STAGE == __SHADER_STAGE_AMPLIFICATION || __SHADER_TARGET_STAGE == __SHADER_STAGE_LIBRARY
+#ifdef FAIL
+#if __SHADER_TARGET_MAJOR == 6
+// expected-error@+1 {{'numthreads' attribute requires an integer constant}}
+[numthreads("1",2,3)]
+// expected-error@+1 {{argument 'X' to numthreads attribute cannot exceed 1024}}
+[numthreads(-1,2,3)]
+// expected-error@+1 {{argument 'Y' to numthreads attribute cannot exceed 1024}}
+[numthreads(1,-2,3)]
+// expected-error@+1 {{argument 'Z' to numthreads attribute cannot exceed 1024}}
+[numthreads(1,2,-3)]
+// expected-error@+1 {{total number of threads cannot exceed 1024}}
+[numthreads(1024,1024,1024)]
+#elif __SHADER_TARGET_MAJOR == 5
+// expected-error@+1 {{argument 'Z' to numthreads attribute cannot exceed 64}}
+[numthreads(1,2,68)]
+#else
+// expected-error@+1 {{argument 'Z' to numthreads attribute cannot exceed 1}}
+[numthreads(1,2,2)]
+// expected-error@+1 {{total number of threads cannot exceed 768}}
+[numthreads(1024,1,1)]
+#endif
+#endif
+// CHECK: HLSLNumThreadsAttr 0x{{[0-9a-fA-F]+}} <line:{{[0-9]+}}:2, col:18> 1 2 1
+[numthreads(1,2,1)]
+int entry() {
+ return 1;
+}
+#else
+// expected-error-re@+1 {{attribute 'numthreads' is unsupported in {{[A-Za-z]+}} shaders, requires Compute, Amplification, Mesh or Library}}
+[numthreads(1,1,1)]
+int main() {
+ return 1;
+}
+#endif
+
+