[HLSL] Support -E option for HLSL.

[python3kgae]

Summary

-E option will set entry function for hlsl.
The format is -E entry_name.

To avoid conflict with existing option with name 'E', add an extra prefix '--'.

A new field HLSLEntry is added to TargetOption.
To share code with HLSLShaderAttr, entry function will be add HLSLShaderAttr attribute too.

[python3kgae]

Rebase to main for fix merge conflict.

[MaskRay]

Is there a specification or reference implementation stating that -E is used?

Option<["--", "/", "-"], "E",

Do you need the prefix --? You may define something like CLFlag. I have missed previous patches, but if / options are not necessary, removing them will be the best to avoid collision with filenames starting with /.

[python3kgae]

In D124751#3513283, @MaskRay wrote:

Is there a specification or reference implementation stating that -E is used?

Option<["--", "/", "-"], "E",

Do you need the prefix --? You may define something like CLFlag. I have missed previous patches, but if / options are not necessary, removing them will be the best to avoid collision with filenames starting with /.

Unfortunately, '/' is necessary. dxc allow both '-' and '/' like 'CLFlag'.
Remove '/' means existing customers may need to change their command line to compile hlsl.
And add '--' feels not hurt anyone, that's why I choose to add '--' to work-around the conflict.

[MaskRay]

In D124751#3513513, @python3kgae wrote:

In D124751#3513283, @MaskRay wrote:

Is there a specification or reference implementation stating that -E is used?

Option<["--", "/", "-"], "E",

Do you need the prefix --? You may define something like CLFlag. I have missed previous patches, but if / options are not necessary, removing them will be the best to avoid collision with filenames starting with /.

Unfortunately, '/' is necessary. dxc allow both '-' and '/' like 'CLFlag'.
Remove '/' means existing customers may need to change their command line to compile hlsl.
And add '--' feels not hurt anyone, that's why I choose to add '--' to work-around the conflict.

OK. I guess you have a pre-existing implementation using -E and the upstreaming implementation wants to be compatible.
I wonder why the new tool needs to reuse clang/include/clang/Driver/Options.td. ISTM it would be cleaner to use a new .td file.

[python3kgae]

In D124751#3529652, @MaskRay wrote:

In D124751#3513513, @python3kgae wrote:

In D124751#3513283, @MaskRay wrote:

Is there a specification or reference implementation stating that -E is used?

Option<["--", "/", "-"], "E",

Do you need the prefix --? You may define something like CLFlag. I have missed previous patches, but if / options are not necessary, removing them will be the best to avoid collision with filenames starting with /.

Unfortunately, '/' is necessary. dxc allow both '-' and '/' like 'CLFlag'.
Remove '/' means existing customers may need to change their command line to compile hlsl.
And add '--' feels not hurt anyone, that's why I choose to add '--' to work-around the conflict.

OK. I guess you have a pre-existing implementation using -E and the upstreaming implementation wants to be compatible.
I wonder why the new tool needs to reuse clang/include/clang/Driver/Options.td. ISTM it would be cleaner to use a new .td file.

That is a good idea.
Let me try that.

[beanz]

clang/lib/Sema/SemaDecl.cpp

9887

If the HLSLShaderAttr::ShaderType enum is properly ordered you should be able to convert the integer value:

HLSLShaderAttr::ShaderType StageInteger = (HLSLShaderAttr::ShaderType)TI.getTriple().getEnvironment() -
                            (uint32_t)llvm::Triple::Pixel;

Then this code doesn't need to be updated if a new shader type is added.

clang/test/SemaHLSL/entry.hlsl

16

Please fix.

[python3kgae]

In D124751#3529652, @MaskRay wrote:

In D124751#3513513, @python3kgae wrote:

In D124751#3513283, @MaskRay wrote:

Is there a specification or reference implementation stating that -E is used?

Option<["--", "/", "-"], "E",

Do you need the prefix --? You may define something like CLFlag. I have missed previous patches, but if / options are not necessary, removing them will be the best to avoid collision with filenames starting with /.

Unfortunately, '/' is necessary. dxc allow both '-' and '/' like 'CLFlag'.
Remove '/' means existing customers may need to change their command line to compile hlsl.
And add '--' feels not hurt anyone, that's why I choose to add '--' to work-around the conflict.

OK. I guess you have a pre-existing implementation using -E and the upstreaming implementation wants to be compatible.
I wonder why the new tool needs to reuse clang/include/clang/Driver/Options.td. ISTM it would be cleaner to use a new .td file.

I tried to create a DxcOptions.td which only have dxc options and core options. Then create OptTable from DxcOptions.td and use the Dxc OptTable when ParseArgs in Driver::ParseArgStrings. It works fine.

But after ParseArgs, there're lots of code in Driver::BuildCompilation use options::OPT_* enum which depends on Options.td. Cannot find a clean solution to switch to DxcOptions while share code about core options in Driver::BuildCompilation. Do you have any suggestions about how to update options::OPT_* enum after switch to new .td file?

[python3kgae]

Add library to HLSLShaderAttr to help convert ShaderType.

[bruno]

Hi, nice to see this getting in. Comments inline!

clang/lib/Sema/SemaDecl.cpp

11747

You can use getEnvironmentTypeName to get the name here.

11748

Since this is both checking and invalidating a decl, perhaps it should return a bool (maybe some asserts in the default case?) and be renamed CheckAndInvalidateHLSLEntryPoint?

clang/test/SemaHLSL/entry.hlsl

3

Can you also add a check that rejects -Efoo with an error message when the language is not hlsl?

[MaskRay]

clang/test/SemaHLSL/entry.hlsl

3

For most triples, the cc1 -E means stopping after preprocessing. If dxil -E is different, better to use a different option to avoid confusion.

[python3kgae]

Add hlsl-entry for cc1 option.

[python3kgae]

clang/lib/Sema/SemaDecl.cpp

11748

The name is following CheckMain and CheckMSVCRTEntryPoint which also checking and invalidating.

[python3kgae]

Fix test fail after rebased on llvm/main.

[python3kgae]

Gentle Ping.

All the previous issues are fixed.

[pow2clk]

Looks to me that all the comments were responded to. I see nothing else worth commenting on.

[beanz]

One very nitpick-y comment, otherwise LGTM.

clang/lib/Sema/SemaDecl.cpp

9873

nit: Comments in the middle of the if make it a bit hard to read, and is not a common style in LLVM code. Please put the comments above the if describing the condition.

[python3kgae]

Cleanup comments and fix test after rebase.

[MaskRay]

clang/test/Driver/hlsl-entry.cpp

1 ↗

(On Diff #450185)

Note, test/Driver deliberately discourages %clang_cc1 and %clang -cc1 tests. The tests test other components of Clang (not Driver) and should be moved to appropriate components.

[python3kgae]

clang/test/Driver/hlsl-entry.cpp

1 ↗

(On Diff #450185)

Moved to Frontend.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 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">;
+def err_hlsl_missing_numthreads : Error<"missing numthreads attribute for %0 shader entry">;
 def err_hlsl_attribute_param_mismatch : Error<"%0 attribute parameters do not match the previous declaration">;
 
 def err_hlsl_pointers_unsupported : Error<
   "%select{pointers|references}0 are unsupported in HLSL">;
 
 def err_hlsl_operator_unsupported : Error<
   "the '%select{&|*|->}0' operator is unsupported in HLSL">;
 
 // Layout randomization diagnostics.
 def err_non_designated_init_used : Error<
   "a randomized struct can only be initialized with a designated initializer">;
 def err_cast_from_randomized_struct : Error<
   "casting from randomized structure pointer type %0 to %1">;
 } // end of sema component.

clang/include/clang/Basic/TargetOptions.h

public:
   std::string DarwinTargetVariantTriple;
 
   /// The version of the darwin target variant SDK which was used during the
   /// compilation.
   llvm::VersionTuple DarwinTargetVariantSDKVersion;
 
   /// The validator version for dxil.
   std::string DxilValidatorVersion;
+
+  /// The entry point name for HLSL shader being compiled as specified by -E.
+  std::string HLSLEntry;
 };
 
 } // end namespace clang
 
 #endif

clang/include/clang/Driver/Options.td

Values<"ps_6_0, ps_6_1, ps_6_2, ps_6_3, ps_6_4, ps_6_5, ps_6_6, ps_6_7,"
          "cs_6_0, cs_6_1, cs_6_2, cs_6_3, cs_6_4, cs_6_5, cs_6_6, cs_6_7,"
          "lib_6_3, lib_6_4, lib_6_5, lib_6_6, lib_6_7, lib_6_x,"
          "ms_6_5, ms_6_6, ms_6_7,"
          "as_6_5, as_6_6, as_6_7">;
 def emit_pristine_llvm : DXCFlag<"emit-pristine-llvm">,
   HelpText<"Emit pristine LLVM IR from the frontend by not running any LLVM passes at all."
            "Same as -S + -emit-llvm + -disable-llvm-passes.">;
 def fcgl : DXCFlag<"fcgl">, Alias<emit_pristine_llvm>;
+def hlsl_entrypoint : Option<["--", "/", "-"], "E", KIND_JOINED_OR_SEPARATE>,
+                      Group<dxc_Group>,
+                      Flags<[DXCOption, CC1Option, NoXarchOption]>,
+                      MarshallingInfoString<TargetOpts<"HLSLEntry">>,
+                      HelpText<"Entry point name">;

clang/include/clang/Sema/Sema.h

public:
   bool CheckFunctionDeclaration(Scope *S,
                                 FunctionDecl *NewFD, LookupResult &Previous,
                                 bool IsMemberSpecialization, bool DeclIsDefn);
   bool shouldLinkDependentDeclWithPrevious(Decl *D, Decl *OldDecl);
   bool canFullyTypeCheckRedeclaration(ValueDecl *NewD, ValueDecl *OldD,
                                       QualType NewT, QualType OldT);
   void CheckMain(FunctionDecl *FD, const DeclSpec &D);
   void CheckMSVCRTEntryPoint(FunctionDecl *FD);
+  void CheckHLSLEntryPoint(FunctionDecl *FD);
   Attr *getImplicitCodeSegOrSectionAttrForFunction(const FunctionDecl *FD,
                                                    bool IsDefinition);
   void CheckFunctionOrTemplateParamDeclarator(Scope *S, Declarator &D);
   Decl *ActOnParamDeclarator(Scope *S, Declarator &D);
   ParmVarDecl *BuildParmVarDeclForTypedef(DeclContext *DC,
                                           SourceLocation Loc,
                                           QualType T);
   ParmVarDecl *CheckParameter(DeclContext *DC, SourceLocation StartLoc,

clang/lib/Driver/ToolChains/Clang.cpp

if ((types::isOpenCL(InputType) ||
     CmdArgs.push_back("-fdeclare-opencl-builtins");
   }
 }
 
 static void RenderHLSLOptions(const ArgList &Args, ArgStringList &CmdArgs,
                               types::ID InputType) {
   const unsigned ForwardedArguments[] = {options::OPT_dxil_validator_version,
                                          options::OPT_S, options::OPT_emit_llvm,
-                                         options::OPT_disable_llvm_passes};
+                                         options::OPT_disable_llvm_passes,
+                                         options::OPT_hlsl_entrypoint};
 
   for (const auto &Arg : ForwardedArguments)
     if (const auto *A = Args.getLastArg(Arg))
       A->renderAsInput(Args, CmdArgs);
 }
 
 static void RenderARCMigrateToolOptions(const Driver &D, const ArgList &Args,
                                         ArgStringList &CmdArgs) {

clang/lib/Sema/SemaDecl.cpp

if (getLangOpts().OpenCL) {
     LangAS AddressSpace = NewFD->getReturnType().getAddressSpace();
     if (AddressSpace != LangAS::Default) {
       Diag(NewFD->getLocation(),
            diag::err_opencl_return_value_with_address_space);
       NewFD->setInvalidDecl();
     }
   }
 
+  if (getLangOpts().HLSL) {
+    auto &TargetInfo = getASTContext().getTargetInfo();
+    if (!NewFD->isInvalidDecl() &&
+        // Skip operator overload which not identifier.
+        Name.isIdentifier() &&
+        NewFD->getName() == TargetInfo.getTargetOpts().HLSLEntry &&
+        // Make sure it is in translation-unit scope.
+        S->getDepth() == 0) {
+      CheckHLSLEntryPoint(NewFD);
+      if (!NewFD->isInvalidDecl()) {
+        auto TripeShaderType = TargetInfo.getTriple().getEnvironment();
+        AttributeCommonInfo AL(NewFD->getBeginLoc());
+        HLSLShaderAttr::ShaderType ShaderType =
+            HLSLShaderAttr::ShaderType::Callable;
+        switch (TripeShaderType) {
+        default:
+          break;
+        case llvm::Triple::EnvironmentType::Compute:
+          ShaderType = HLSLShaderAttr::ShaderType::Compute;
+          break;
+        case llvm::Triple::EnvironmentType::Vertex:
+          ShaderType = HLSLShaderAttr::ShaderType::Vertex;
+          break;
+        case llvm::Triple::EnvironmentType::Hull:
+          ShaderType = HLSLShaderAttr::ShaderType::Hull;
+          break;
+        case llvm::Triple::EnvironmentType::Domain:
+          ShaderType = HLSLShaderAttr::ShaderType::Domain;
+          break;
+        case llvm::Triple::EnvironmentType::Geometry:
+          ShaderType = HLSLShaderAttr::ShaderType::Geometry;
+          break;
+        case llvm::Triple::EnvironmentType::Pixel:
+          ShaderType = HLSLShaderAttr::ShaderType::Pixel;
+          break;
+        case llvm::Triple::EnvironmentType::Mesh:
+          ShaderType = HLSLShaderAttr::ShaderType::Mesh;
+          break;
+        case llvm::Triple::EnvironmentType::Amplification:
+          ShaderType = HLSLShaderAttr::ShaderType::Amplification;
+          break;
+        }
+        // To share code with HLSLShaderAttr, add HLSLShaderAttr to entry function.
+        if (HLSLShaderAttr *Attr = mergeHLSLShaderAttr(NewFD, AL, ShaderType))
+          NewFD->addAttr(Attr);
+      }
+    }
+  }
+
   if (!getLangOpts().CPlusPlus) {
     // Perform semantic checking on the function declaration.
     if (!NewFD->isInvalidDecl() && NewFD->isMain())
       CheckMain(NewFD, D.getDeclSpec());
 
     if (!NewFD->isInvalidDecl() && NewFD->isMSVCRTEntryPoint())
       CheckMSVCRTEntryPoint(NewFD);
 
void Sema::CheckMSVCRTEntryPoint(FunctionDecl *FD) {
   }
 
   if (!FD->isInvalidDecl() && FD->getDescribedFunctionTemplate()) {
     Diag(FD->getLocation(), diag::err_mainlike_template_decl) << FD;
     FD->setInvalidDecl();
   }
 }
 
+void Sema::CheckHLSLEntryPoint(FunctionDecl *FD) {
+  auto &TargetInfo = getASTContext().getTargetInfo();
+  switch (TargetInfo.getTriple().getEnvironment()) {
+  default:
+    // FIXME: check all shader profiles.
+    break;
+  case llvm::Triple::EnvironmentType::Compute:
+    if (!FD->hasAttr<HLSLNumThreadsAttr>()) {
+      Diag(FD->getLocation(), diag::err_hlsl_missing_numthreads) << "Compute";
+      FD->setInvalidDecl();
+    }
+    break;
+  }
+}
+
 bool Sema::CheckForConstantInitializer(Expr *Init, QualType DclT) {
   // FIXME: Need strict checking.  In C89, we need to check for
   // any assignment, increment, decrement, function-calls, or
   // commas outside of a sizeof.  In C99, it's the same list,
   // except that the aforementioned are allowed in unevaluated
   // expressions.  Everything else falls under the
   // "may accept other forms of constant expressions" exception.
   //

clang/test/SemaHLSL/entry.hlsl

+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-compute -x hlsl -Efoo -DWITH_NUM_THREADS -ast-dump -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-compute -x hlsl -Efoo  -o - %s  -verify
+
+
+// Make sure add HLSLShaderAttr along with HLSLNumThreadsAttr.
+// CHECK:HLSLNumThreadsAttr 0x{{.*}} <line:10:2, col:18> 1 1 1
+// CHECK:HLSLShaderAttr 0x{{.*}} <line:13:1> Compute
+
+#ifdef WITH_NUM_THREADS
+[numthreads(1,1,1)]
+#endif
+// expected-error@+1 {{missing numthreads attribute for Compute shader entry}}
+void foo() {
+
+}
 No newline at end of file

clang/test/SemaHLSL/prohibit_pointer.hlsl

-// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -o - -fsyntax-only %s -verify
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-library -x hlsl -o - -fsyntax-only %s -verify
 
 // expected-error@+1 {{pointers are unsupported in HLSL}}
 typedef int (*fn_int)(int);
 void* bark(int); // expected-error {{pointers are unsupported in HLSL}}
 void meow(int*); // expected-error {{pointers are unsupported in HLSL}}
 
 struct Foo {
   int X;