Report error for unsupported types of SV semantics #3043

vcsharma · 2020-07-16T01:48:34Z

FXC reports error, but DXC only asserts.

Fixes #2299
Fixes #2954
Fixes #3444

AppVeyorBot · 2020-07-16T03:54:23Z

✅ Build DirectXShaderCompiler 1.0.3442 completed (commit 50a6ce0eab by @vcsharma)

pow2clk

I have some suggestions for alternate ways to do things, but you're free to take them or leave them.

tools/clang/lib/Sema/SemaHLSL.cpp

pow2clk · 2020-07-16T13:35:47Z

tools/clang/lib/Sema/SemaHLSL.cpp

+
+  if (isTypeDef || isLocalVar) {
+    self->Diag(semanticDecl->Loc, diag::err_hlsl_varmodifierna)
+        << "semantic" << declarationType;


This is pre-existing of course, but I'm not sure it's consistent with our documentation as I read it here:
https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-variable-syntax#Semantic

which says that "The compiler ignores semantics unless they are declared on a global variable, or a parameter passed into a shader."

pow2clk · 2020-07-16T13:37:02Z

tools/clang/test/HLSLFileCheck/hlsl/diagnostics/errors/unsupported_type_cs_input_semantic.hlsl

+// RUN: %dxc -E main -T cs_6_0 -DTID_TY=uint3 -DGI_TY=uint -DGID_TY=uint3 -DGTID_TY=float1x1 %s | FileCheck %s -check-prefix=CHK_GTID_TY_ERR
+// RUN: %dxc -E main -T cs_6_0 -DTID_TY=uint3 -DGI_TY=uint -DGID_TY=uint3 -DGTID_TY=float %s | FileCheck %s -check-prefix=CHK_GTID_TY_ERR
+// RUN: %dxc -E main -T cs_6_0 -DTID_TY=uint3 -DGI_TY=uint -DGID_TY=uint3 -DGTID_TY=bool %s | FileCheck %s -check-prefix=CHK_GTID_TY_ERR
+// RUN: %dxc -E main -T cs_6_0 -DTID_TY=uint3 -DGI_TY=uint -DGID_TY=uint3 -DGTID_TY=min16float %s | FileCheck %s -check-prefix=CHK_GTID_TY_ERR


Very thorough! 👍

pow2clk

Overall, I like this approach better. Most of my comments are pretty trivial, but I'm worried about the lack of matrix handling in GetCompSize

tools/clang/lib/AST/HlslTypes.cpp

tools/clang/lib/CodeGen/CGHLSLMS.cpp

AppVeyorBot · 2020-07-17T20:24:59Z

✅ Build DirectXShaderCompiler 1.0.3447 completed (commit 433e024062 by @vcsharma)

pow2clk

Looks good! thanks!

tools/clang/lib/AST/HlslTypes.cpp

AppVeyorBot · 2020-07-17T23:05:03Z

✅ Build DirectXShaderCompiler 1.0.3448 completed (commit 26f204416a by @vcsharma)

tex3d

I'm afraid the location the error checking is hooked in to is still too early to pick up the majority of errors it is meant to catch. It needs to be moved to HLSignatureLower, when the signature elements are actually known. Unfortunately, that also means you won't have any of the front-end decl or semantic location information available, unless we add something to preserve it through extra debug info. The current error checking there points to the function, which should be sufficient for now.

There are other issues with the error checking that need addressing, such as disallowing matrix or array types except for very special cases, which may actually be made easier by moving to HLSignatureLower, where you can validate sizes based on the final shape (rows/columns), and basic llvm types, rather than the whole variety of decl possibilities at the high level.

If we wanted to add source locations for error handling such as this (future work), I suggest we add something to the type annotations. Normally we would try to use debug info, but for basic validation like this it is nice to be able to provide locations without requiring /Zi, and currently /Zi doesn't even provide anything better than the entry point location without additional work to preserve more.

tools/clang/lib/AST/HlslTypes.cpp

tex3d · 2020-07-18T20:56:04Z

tools/clang/lib/CodeGen/CGHLSLMS.cpp

+  if (qty->isArrayType())
+    return GetCompSize(GetArrayElementType(qty));
+  if (IsHLSLMatType(qty))
+    return GetCompSize(GetHLSLMatElementType(qty));


I don't think we should support array or matrix for system values - other than under the Other designation for special cases.
No system value can be an array except for special cases handled under Other.
If fxc suports matrix types, I think that's an oversight, and should not be by-design. If the matrix had multiple rows, that would be illegal for all system values except tessfactors.

tools/clang/lib/CodeGen/CGHLSLMS.cpp

tools/clang/test/HLSLFileCheck/hlsl/diagnostics/errors/unsupported_type_cs_input_semantic.hlsl

…mpiler into bugfix

AppVeyorBot · 2020-12-25T00:58:34Z

✅ Build DirectXShaderCompiler 1.0.4032 completed (commit deb6a7a30d by @vcsharma)

This reverts commit 4006a27.

…mpiler into bugfix

AppVeyorBot · 2021-02-09T19:26:29Z

❌ Build DirectXShaderCompiler 1.0.4262 failed (commit 757ce14c1f by @vcsharma)

AppVeyorBot · 2021-02-09T20:30:32Z

✅ Build DirectXShaderCompiler 1.0.4263 completed (commit ebfc77c285 by @vcsharma)

AppVeyorBot · 2021-02-11T07:58:18Z

✅ Build DirectXShaderCompiler 1.0.4283 completed (commit 05cb537a3e by @vcsharma)

pow2clk

It looks really good!

This was a pretty big hole in error reporting and I think this will resolve it definitively.

I just have a couple concerns on specific interpretation and error propagating where I might well be confused

lib/DXIL/DxilSemantic.cpp

pow2clk · 2021-02-11T19:17:47Z

lib/HLSL/HLSignatureLower.cpp

+  // Generate error and exit if semantic type
+  // is not one of the allowed types
+  if (!ValidateSemanticType(Entry))
+    return;


This provides no indication of failure to the subsequent passes, I worry this will bail out of the rest of this pass and potentially cause weird followup errors on subsequent passes that expect these operations to have been performed.

That's true, and I have a feeling that continuing with this pass will result in fewer weird cascading errors from this than returning here. Currently, it seems the only way to abort the pipeline is with a report_fatal_error, which currently raises a structured exception - which is way too harsh for something like this, and we should likely change report_fatal_error to throw a C++ exception, since we still have a chance of emitting a useful error message then.

I have a feeling that continuing with this pass will result in fewer weird cascading errors from this than returning here

Actually bailing out early avoids crashes at least in some cases. Like for CS SV semantics, if we pass an invalid type like float and let it continue with the lowering then it crashes in HLSignatureLower::GenerateDxilCSInputs() in the below line.

// If the argument is of non-i32 type, convert here if (newArg->getType() != NumTy) newArg = Builder.CreateZExtOrTrunc(newArg, NumTy);

Based on the comments here, I agree we need a mechanism to stop further compilation when we generate errors. I suspect we would need that mechanism not just in this change, but probably at other places too where we generate errors. Therefore I created an issue #3496 to track this as a future enhancement that we would like to implement.

That's consistent with my later investigations. I approve of solving the larger problem right later.

…mpiler into bugfix

...ck/hlsl/diagnostics/errors/semantics_type_checking/unsupported_types_cs_input_semantics.hlsl

tex3d · 2021-02-23T02:05:56Z

lib/HLSL/HLSignatureLower.cpp

+        inputQual == DxilParamInputQual::OutIndices ||
+        inputQual == DxilParamInputQual::OutPrimitives ||
+        inputQual == DxilParamInputQual::OutputPatch ||
+        inputQual == DxilParamInputQual::OutVertices) {


Wish there was a better place to associate the input qualifier property with the expected array parameter shape (though this is a bit of an internal HL IR detail). Even if it was in its own function somewhere.

…mpiler into bugfix

AppVeyorBot · 2021-02-23T18:59:56Z

✅ Build DirectXShaderCompiler 1.0.4385 completed (commit 3376a31105 by @vcsharma)

AppVeyorBot · 2021-02-25T01:19:54Z

❌ Build DirectXShaderCompiler 1.0.4397 failed (commit d94d1319c7 by @vcsharma)

…mpiler into bugfix

tex3d · 2021-02-25T22:39:06Z

include/dxc/DXIL/DxilSemantic.h

  };

  enum class SizeClass {
    Scalar,
+    Vec1,


Can't we treat Vec1 as Scalar? I think either one should be legal for all scalar system values, right? I see some that still only go up to Scalar, so does that mean those won't accept vec1? Why not?

Also, it would be nice to make the 0 enum value something like Invalid, or the Unknown. If Scalar aliased Vec1, then vector size would align with the enum values (always easier when looking at raw values, such as in debugger).

tex3d · 2021-02-25T22:39:12Z

lib/DXIL/DxilSemantic.cpp

@@ -126,7 +126,10 @@ Semantic::SizeClass Semantic::GetCompCount(llvm::Type* ty) const {
    return SizeClass::Unknown;

  if (ty->isVectorTy()) {
-    if (ty->getVectorNumElements() == 2) {
+    if (ty->getVectorNumElements() == 1) {
+      return SizeClass::Vec1;


Here is where we could simply return Scalar, right?

tex3d

LGTM!

AppVeyorBot · 2021-02-26T00:35:42Z

✅ Build DirectXShaderCompiler 1.0.4414 completed (commit b5b4b7ca45 by @vcsharma)

…#3043)" This reverts commit bece3d4. Revert system value type checking code due to regressions. Will re-merge once it's fully verified fixed.

…3532) This reverts commit bece3d4. Revert system value type checking code due to regressions. Will re-merge once it's fully verified fixed.

pow2clk · 2023-09-26T15:49:26Z

Just to throw this into the mix, #5768 is related to this cadre of errors as well.

We could use the means of defining what types go with what semantics here, but we wouldn't want to place it in the DxilSignature pass as this did. Rather, we should put it in Sema, possibly SemaHLSL DiagnoseEntry function.

Report error on unsupported types for CS input semantics

5ae34bf

vcsharma requested review from tex3d and pow2clk July 16, 2020 01:48

pow2clk approved these changes Jul 16, 2020

View reviewed changes

Vishal C Sharma added 3 commits July 16, 2020 10:37

Undo changes in SemaHLSL

772847e

Offline feedback from Tex

8e245ac

Update test

8575cf8

vcsharma changed the title ~~Report error for unsupported types of CS input semantics~~ Report error for unsupported types of SV semantics Jul 17, 2020

Fix travis build error

8771fb4

pow2clk reviewed Jul 17, 2020

View reviewed changes

tools/clang/lib/AST/HlslTypes.cpp Outdated Show resolved Hide resolved

tools/clang/lib/CodeGen/CGHLSLMS.cpp Outdated Show resolved Hide resolved

tools/clang/lib/CodeGen/CGHLSLMS.cpp Outdated Show resolved Hide resolved

Vishal C Sharma added 3 commits July 17, 2020 13:30

Use DXASSERT

79bb9bb

Handle matrix

f6f9ebd

Fix indentation

1f628e1

pow2clk approved these changes Jul 17, 2020

View reviewed changes

tools/clang/lib/AST/HlslTypes.cpp Outdated Show resolved Hide resolved

tex3d requested changes Jul 18, 2020

View reviewed changes

vcsharma linked an issue Dec 3, 2020 that may be closed by this pull request

Silent crash or validation error when SV_InnerCoverage semantic is assigned to a float parameter instead of uint #2954

Closed

Vishal C Sharma added 3 commits December 24, 2020 16:02

Revert previous change

35266e9

Generate error for disallowed types for CS input semantics

4006a27

Merge branch 'master' of https://github.com/microsoft/DirectXShaderCo…

9a80b70

…mpiler into bugfix

Vishal C Sharma added 3 commits February 8, 2021 16:53

Revert "Generate error for disallowed types for CS input semantics"

953457e

This reverts commit 4006a27.

Merge branch 'master' of https://github.com/microsoft/DirectXShaderCo…

1238ade

…mpiler into bugfix

Extend type checking for all semantics

34d0e50

Fix travis build error

fda69bb

Use dxilutil::IsIntegerOrFloatingPointType

581fe30

pow2clk reviewed Feb 11, 2021

View reviewed changes

Merge branch 'master' of https://github.com/microsoft/DirectXShaderCo…

61f2575

…mpiler into bugfix

vcsharma mentioned this pull request Feb 19, 2021

[spirv] support non-int type SV_DispatchThreadId #3481

Closed

Merge branch 'master' of https://github.com/microsoft/DirectXShaderCo…

5bac07c

…mpiler into bugfix

tex3d reviewed Feb 23, 2021

View reviewed changes

...ck/hlsl/diagnostics/errors/semantics_type_checking/unsupported_types_cs_input_semantics.hlsl Show resolved Hide resolved

tex3d reviewed Feb 23, 2021

View reviewed changes

tex3d approved these changes Feb 23, 2021

View reviewed changes

vcsharma added 3 commits February 23, 2021 09:26

Merge branch 'master' of https://github.com/microsoft/DirectXShaderCo…

2ec8f4f

…mpiler into bugfix

Accomodate change in signature for dxilutil::EmitErrorOnFunction

51bfb18

Review comments

1c4767f

vcsharma mentioned this pull request Feb 23, 2021

Add an option to abort compilation when generating errors #3496

Closed

jaebaek mentioned this pull request Feb 24, 2021

[SPIR-V] Assigning SV_DispatchThreadID to a float argument causes validation errors (does not happen with DXIL backend) #3443

Closed

Relax the SV type checking criteria based on internal testing

29c2101

Merge branch 'master' of https://github.com/microsoft/DirectXShaderCo…

533a4dd

…mpiler into bugfix

tex3d reviewed Feb 25, 2021

View reviewed changes

Review comments

53d077f

tex3d approved these changes Feb 25, 2021

View reviewed changes

This was referenced Feb 26, 2021

Some SV semantics support scalar but crashes on vec1 type #3508

Closed

[DXIL] Decorating CS float argument with SV_DispatchThreadID semantic crashes the compiler (float2, float3 and float4 works) #3444

Open

vcsharma merged commit bece3d4 into microsoft:master Feb 26, 2021

vcsharma deleted the bugfix branch February 26, 2021 00:38

tex3d added a commit that referenced this pull request Mar 3, 2021

Revert "Report error for unsupported types of SV semantics (#3043)" (#…

5bdf357

…3532) This reverts commit bece3d4. Revert system value type checking code due to regressions. Will re-merge once it's fully verified fixed.

bob80905 pushed a commit to bob80905/DirectXShaderCompiler that referenced this pull request Apr 27, 2022

Report error for unsupported types of SV semantics (microsoft#3043)

2db5ed3

bob80905 pushed a commit to bob80905/DirectXShaderCompiler that referenced this pull request Apr 28, 2022

Report error for unsupported types of SV semantics (microsoft#3043)

cc65df2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report error for unsupported types of SV semantics #3043

Report error for unsupported types of SV semantics #3043

vcsharma commented Jul 16, 2020 •

edited

Loading

AppVeyorBot commented Jul 16, 2020

pow2clk left a comment

pow2clk Jul 16, 2020

pow2clk Jul 16, 2020

pow2clk left a comment

AppVeyorBot commented Jul 17, 2020

pow2clk left a comment

AppVeyorBot commented Jul 17, 2020

tex3d left a comment

tex3d Jul 18, 2020

AppVeyorBot commented Dec 25, 2020

AppVeyorBot commented Feb 9, 2021

AppVeyorBot commented Feb 9, 2021

AppVeyorBot commented Feb 11, 2021

pow2clk left a comment

pow2clk Feb 11, 2021

tex3d Feb 23, 2021

vcsharma Feb 23, 2021

pow2clk Feb 23, 2021

tex3d Feb 23, 2021

AppVeyorBot commented Feb 23, 2021

AppVeyorBot commented Feb 25, 2021

tex3d Feb 25, 2021

tex3d Feb 25, 2021

tex3d left a comment

AppVeyorBot commented Feb 26, 2021

pow2clk commented Sep 26, 2023

Report error for unsupported types of SV semantics #3043

Report error for unsupported types of SV semantics #3043

Conversation

vcsharma commented Jul 16, 2020 • edited Loading

AppVeyorBot commented Jul 16, 2020

pow2clk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pow2clk left a comment

Choose a reason for hiding this comment

AppVeyorBot commented Jul 17, 2020

pow2clk left a comment

Choose a reason for hiding this comment

AppVeyorBot commented Jul 17, 2020

tex3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Dec 25, 2020

AppVeyorBot commented Feb 9, 2021

AppVeyorBot commented Feb 9, 2021

AppVeyorBot commented Feb 11, 2021

pow2clk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Feb 23, 2021

AppVeyorBot commented Feb 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tex3d left a comment

Choose a reason for hiding this comment

AppVeyorBot commented Feb 26, 2021

pow2clk commented Sep 26, 2023

vcsharma commented Jul 16, 2020 •

edited

Loading