Improve library profile diagnostics #5789

bob80905 · 2023-09-27T02:49:53Z

There are several diagnostics that are missing when a function declaration that targets a specific shader stage is missing a necessary attribute. For example, if a hull shader function declaration does not have the patchconstantfunc attribute, there is no diagnosis of the missing attribute in sema. This PR addresses this diagnostic hole by filling in some functions that check that all required attributes exist for a given shader stage.
This PR partially addresses #5747

tools/clang/lib/Sema/SemaHLSL.cpp

llvm-beanz · 2023-09-27T19:13:52Z

tools/clang/test/CodeGenHLSL/attributes-hs-no-pcf.hlsl

Can we move these tests into test/SemaHLSL/attributes and have them be run using %dxc ... %s -verify instead?

I strongly suspect if we did that these tests would fail because it looks to me like we're emitting errors for this twice because the old code is still there.

Agreed.

Also, many entry point diagnostics can be checked in a single HLSL file with a lib target, rather than having one file per scenario. With this new codepath, the entry point checks are the same code path between lib and normal shader entry cases, making it possible to separate the unit tests for adding the shader attribute for the non-lib case from verification of all the entry point rules.

tex3d · 2023-09-27T19:20:17Z

Shouldn't there be corresponding diagnostic code deleted from AddHLSLFunctionInfo() in CGHLSLMS.cpp?

bob80905 · 2023-09-27T19:48:01Z

Shouldn't there be corresponding diagnostic code deleted from AddHLSLFunctionInfo() in CGHLSLMS.cpp?

No, because the only thing CGHLSLMS.cpp does is check if the attribute exists, and if it does, verify that it is on a valid shader stage. It does not ever first check the shader stage, and validate that all necessary/required attributes exist.
Regardless, I agree the tests are worth moving to test/SemaHLSL/attributes

llvm-beanz · 2023-09-27T21:23:44Z

SemaHLSL.cpp hlsl::DiagnoseTranslationUnit does have duplicate emission of these errors, so we probably do need some cleanup.

…file, move to semahlsl/attributes

github-actions · 2023-09-27T22:33:22Z

PR contains clang-format violations. First 50 lines of the diff:

--- tools/clang/lib/Sema/SemaHLSL.cpp	(before formatting) +++ tools/clang/lib/Sema/SemaHLSL.cpp	(after formatting) @@ -11387,7 +11387,7 @@ if (pEntryPointDecl) { const auto *shaderModel = hlsl::ShaderModel::GetByName(self->getLangOpts().HLSLProfile.c_str()); -  + if (shaderModel->IsHS()) { if (const HLSLPatchConstantFuncAttr *Attr = pEntryPointDecl->getAttr<HLSLPatchConstantFuncAttr>()) { --- tools/clang/unittests/HLSL/ValidationTest.cpp	(before formatting) +++ tools/clang/unittests/HLSL/ValidationTest.cpp	(after formatting) @@ -152,7 +152,7 @@ TEST_METHOD(DuplicateSysValue) TEST_METHOD(FunctionAttributes) TEST_METHOD(GSMainMissingAttributeFail) - TEST_METHOD(GSOtherMissingAttributeFail)  + TEST_METHOD(GSOtherMissingAttributeFail) TEST_METHOD(GetAttributeAtVertexInVSFail) TEST_METHOD(GetAttributeAtVertexIn60Fail) TEST_METHOD(GetAttributeAtVertexInterpFail)

See action log for the full diff.

pow2clk

Looks good!

pow2clk · 2023-09-28T18:27:42Z

I don't know why this can't go directly into main. I don't see any mention of nodes or other 6.8 features.

github-actions · 2023-09-28T18:33:59Z

PR contains clang-format violations. First 50 lines of the diff:

--- include/dxc/HLSL/HLOperations.h	(before formatting) +++ include/dxc/HLSL/HLOperations.h	(after formatting) @@ -293,7 +293,6 @@ const unsigned kSampleCmpClampArgIndex = 6; const unsigned kSampleCmpStatusArgIndex = 7; - // SampleCmpBias. const unsigned kSampleCmpBCmpValArgIndex = 4; const unsigned kSampleCmpBBiasArgIndex = 5; --- include/llvm/InitializePasses.h	(before formatting) +++ include/llvm/InitializePasses.h	(after formatting) @@ -258,7 +258,7 @@ void initializeDynamicIndexingVectorToArrayPass(PassRegistry&); void initializeMultiDimArrayToOneDimArrayPass(PassRegistry&); void initializeResourceToHandlePass(PassRegistry&); -void initializeLowerWaveMatTypePass(PassRegistry&); +void initializeLowerWaveMatTypePass(PassRegistry &); void initializeSROA_SSAUp_HLSLPass(PassRegistry&); void initializeHoistConstantArrayPass(PassRegistry&); void initializeDxilLoopUnrollPass(PassRegistry&); --- include/llvm/Transforms/Scalar.h	(before formatting) +++ include/llvm/Transforms/Scalar.h	(after formatting) @@ -185,7 +185,7 @@ // Flatten resource into handle. // ModulePass *createLowerWaveMatTypePass(); -void initializeLowerWaveMatTypePass(PassRegistry&); +void initializeLowerWaveMatTypePass(PassRegistry &); //===----------------------------------------------------------------------===// // Hoist a local array initialized with constant values to a global array with --- lib/DXIL/DxilWaveMatrix.cpp	(before formatting) +++ lib/DXIL/DxilWaveMatrix.cpp	(after formatting) @@ -19,7 +19,6 @@ #include "llvm/IR/DerivedTypes.h" #include "llvm/IR/LLVMContext.h" #include "llvm/IR/Module.h" - using namespace llvm; --- lib/Transforms/Utils/Local.cpp	(before formatting) +++ lib/Transforms/Utils/Local.cpp	(after formatting) @@ -45,10 +45,10 @@ #include "llvm/Support/MathExtras.h" #include "llvm/Support/raw_ostream.h" +#include "dxc/DXIL/DxilInstructions.h" // HLSL Change - DxilInst_OutputComplete usage #include "dxc/DXIL/DxilMetadataHelper.h" // HLSL Change - combine dxil metadata.

See action log for the full diff.

bob80905 · 2023-09-28T18:35:43Z

There seem to be some clang format issues. Perhaps I can cherry pick this over to main once it's in staging.

tex3d · 2023-10-03T18:07:19Z

tools/clang/lib/Sema/SemaHLSL.cpp

+ if (shaderModel->IsHS()) {
 if (const HLSLPatchConstantFuncAttr *Attr =
 pEntryPointDecl->getAttr<HLSLPatchConstantFuncAttr>()) {
 NameLookup NL = GetSingleFunctionDeclByName(


Shouldn't this code be moved into DiagnoseHullEntry?

The reason I can't move this block is because pPatchFnDecl is set by this block, and is used below when checking for recursion. If I moved it into DiagnoseHullEntry, I would need to repeat this code here anyways in order to set pPatchFnDecl.

I realize that this code also captures the patch constant function for recursion check later. I think all this needs to be moved somewhere else eventually, since we need the recursion check on all functions (but also don't want to have to pay a cost of re-checking the same call graph over and over again).

Or is the reason this can't be moved also related to timing - maybe the patch constant function won't have been created yet when we are in DiagnoseEntry, for example?

tools/clang/unittests/HLSL/ValidationTest.cpp

tex3d · 2023-10-03T19:07:12Z

tools/clang/test/SemaHLSL/attributes/NecessaryAttribsInLib.hlsl

+[shader("mesh")]
+//[NumThreads(8, 8, 2)]
+//[OutputTopology("triangle")]
+void MSmain(out vertices myvert verts[32], /* expected-error{{mesh entry point must have the numthreads attribute}} */ /* expected-error{{mesh entry point must have the outputtopology attribute}} */


Tip: for better readability, you can use syntax like: expected-error@+2, for instance:

// expected-error@+2{{mesh entry point must have the numthreads attribute}} // expected-error@+1{{mesh entry point must have the outputtopology attribute}} void MSmain(out vertices myvert verts[32],

… shaders (#5858) Previously, there would be no recursion validation performed on any function declarations in library shaders. This PR adds code that validates that no function declarations that are found inside library shaders are recursive. The diagnostics aren't repeated, since there's logic to prevent the same diagnostic from being emitted by 2 similar ancestors of the same recursive function. The PR also adjusts recursion detection for shaders in non-library cases, and improves the original diagnostic by adding information about the name of the function that is recursive. The PR is meant to complete the work on issue #5789. However, more work needs to be done for full accuracy. The PR checks *all* function declarations in the translation unit for the library shader case, when it should really only be checking a subset of function declarations. This issue is filed here: #5857 Fixes #5789

add diagnose*entry fxns for verifying attrs that must exist do exist

91679a2

bob80905 changed the base branch from main to staging-sm-6.8 September 27, 2023 02:50

This comment was marked as resolved.

Sign in to view

bob80905 added 2 commits September 26, 2023 19:54

clang format

067a89e

update spirv tests that fail

5cd5802

tex3d reviewed Sep 27, 2023

View reviewed changes

tools/clang/lib/Sema/SemaHLSL.cpp Outdated Show resolved Hide resolved

llvm-beanz reviewed Sep 27, 2023

View reviewed changes

move tests to semahlsl/attributes, and use -verify

e3ce1cf

clean up diagnoseTranslationUnit, merge necessary attr checks into 1 …

7140eeb

…file, move to semahlsl/attributes

clang format

379cb18

pow2clk approved these changes Sep 28, 2023

View reviewed changes

bob80905 changed the base branch from staging-sm-6.8 to main September 28, 2023 18:33

bob80905 changed the base branch from main to staging-sm-6.8 September 28, 2023 18:34

tex3d reviewed Oct 3, 2023

View reviewed changes

tools/clang/unittests/HLSL/ValidationTest.cpp Show resolved Hide resolved

tex3d reviewed Oct 3, 2023

View reviewed changes

tex3d approved these changes Oct 3, 2023

View reviewed changes

improve expected error readability

7b3f6d9

bob80905 merged commit da249d8 into microsoft:staging-sm-6.8 Oct 3, 2023

bob80905 mentioned this pull request Oct 12, 2023

[Diagnostics] Improve recursion diagnostics, specifically for library shaders #5858

Merged

pow2clk mentioned this pull request Nov 2, 2023

Library entry points are missing many diagnostics #5747

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve library profile diagnostics #5789

Improve library profile diagnostics #5789

Uh oh!

bob80905 commented Sep 27, 2023

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

llvm-beanz Sep 27, 2023

tex3d Sep 27, 2023

tex3d commented Sep 27, 2023

bob80905 commented Sep 27, 2023

llvm-beanz commented Sep 27, 2023

github-actions bot commented Sep 27, 2023

pow2clk left a comment

pow2clk commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

bob80905 commented Sep 28, 2023

tex3d Oct 3, 2023

bob80905 Oct 3, 2023

tex3d Oct 3, 2023

Uh oh!

tex3d Oct 3, 2023

Labels

4 participants

Improve library profile diagnostics #5789

Improve library profile diagnostics #5789

Uh oh!

Conversation

bob80905 commented Sep 27, 2023

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

llvm-beanz Sep 27, 2023

Choose a reason for hiding this comment

tex3d Sep 27, 2023

Choose a reason for hiding this comment

tex3d commented Sep 27, 2023

bob80905 commented Sep 27, 2023

llvm-beanz commented Sep 27, 2023

github-actions bot commented Sep 27, 2023

pow2clk left a comment

Choose a reason for hiding this comment

pow2clk commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

bob80905 commented Sep 28, 2023

tex3d Oct 3, 2023

Choose a reason for hiding this comment

bob80905 Oct 3, 2023

Choose a reason for hiding this comment

tex3d Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

tex3d Oct 3, 2023

Choose a reason for hiding this comment

Labels

4 participants