Disallow illegal Atomic targets + propagate consts #4453

pow2clk · 2022-05-12T05:43:02Z

This disallows a number of illegal destination parameters to atomic
operations. This includes SRVs and other const values, non-groupshared
and non-resourse variables, members of typed resources, and bitfield
members of any resource.

In addition, this correctly propagates const information so that they
can be properly rejected and also the information is reflected.
This involves the consolidation and earlier detection of a number of
these failures as well as an expansion of that detection.
Also adds validation checks that the targets are not const and either
UAVs or groupshared address space.

Add compilation tests as well as validation tests for all.

Fixes #4319
Fixes #4377
Fixes #4437

This disallows a number of illegal destination parameters to atomic operations. This includes SRVs and other const values, non-groupshared and non-resourse variables, members of typed resources, and bitfield members of any resource. In addition, this correctly propagates const information so that they can be properly rejected and also the information is reflected. This involves the consolidation and earlier detection of a number of these failures as well as an expansion of that detection. Also adds validation checks that the targets are not const and either UAVs or groupshared address space. Add compilation tests as well as validation tests for all. Fixes microsoft#4319 Fixes microsoft#4377 Fixes microsoft#4437

pow2clk · 2022-05-12T05:49:25Z

lib/HLSL/HLOperationLower.cpp

-          dxilutil::EmitErrorOnInstruction(userCall, "Invalid operation on typed buffer.");
-          return;
-          break;
-        }


All of this was just to generate different errors when atomic ops were performed on typed resources had non-scalar elements. Because this is handled elsewhere, the distinction need not be done here anymore

lib/HLSL/HLOperationLower.cpp

tools/clang/lib/Sema/SemaExpr.cpp

tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl

AppVeyorBot · 2022-05-12T06:49:14Z

✅ Build DirectXShaderCompiler 1.0.1619 completed (commit 41fe64520e by @pow2clk)

AppVeyorBot · 2022-05-12T07:47:18Z

✅ Build DirectXShaderCompiler 1.0.1620 completed (commit 52d9b4952b by @pow2clk)

lib/HLSL/DxilValidation.cpp

lib/HLSL/HLOperationLower.cpp

tools/clang/lib/Sema/SemaHLSL.cpp

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

tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl

tools/clang/test/DXILValidation/atomics.hlsl

add test for reviewer commented lib export func test remove unintended and fairly weird unsubscripted references to resources in restypes test add more specific checks for unfound atomics for resources in restypes

AppVeyorBot · 2022-05-13T00:14:14Z

✅ Build DirectXShaderCompiler 1.0.1624 completed (commit 93afc07e4a by @pow2clk)

pow2clk

Respond to feedback

lib/HLSL/DxilValidation.cpp

lib/HLSL/HLOperationLower.cpp

tools/clang/lib/Sema/SemaHLSL.cpp

tools/clang/test/DXILValidation/atomics.hlsl

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

Add location information for overload mismatch notes. Their absence was preventing verifier tests from working. For note referring to declaration location, skip if ther is no location due to being builtin Convert all new error tests to verifier tests Reword an erorr message revise test to be valid HLSL

AppVeyorBot · 2022-05-14T03:40:50Z

❌ Build DirectXShaderCompiler 1.0.1634 failed (commit 5f0d0fd41e by @pow2clk)

Add drilling through GEP and BC for groupshared atomic validation Add validation test for const groupshared atomic used through a GEP Enhance error detection for atomics at lowering time by accounting for any sequence of GEP and subscript operations that might end with a valid resource. Add various new testing for atomic operations performed on destination values that are attached to a string of GEP and subscripts.

AppVeyorBot · 2022-05-16T04:34:15Z

❌ Build DirectXShaderCompiler 1.0.1635 failed (commit d1c4caa2e6 by @pow2clk)

AppVeyorBot · 2022-05-16T07:22:44Z

❌ Build DirectXShaderCompiler 1.0.1636 failed (commit c4dbf01bdd by @pow2clk)

AppVeyorBot · 2022-05-16T08:58:35Z

❌ Build DirectXShaderCompiler 1.0.1637 failed (commit b4a701ef50 by @pow2clk)

AppVeyorBot · 2022-05-16T12:37:07Z

✅ Build DirectXShaderCompiler 1.0.1639 completed (commit 15f597ffed by @pow2clk)

tex3d

Comments about strange/misleading error messages are really just to note, not for fixing in this PR. Makes sense to add new verifier tests to VerifierHelper though.

tools/clang/unittests/HLSL/VerifierTest.cpp

tex3d · 2022-05-16T16:28:51Z

tools/clang/test/HLSL/write-const-arrays.hlsl

+  // Assigning using dest param of atomics
+  // Distinct because of special handling of atomics dest param
+  InterlockedAdd(local[0], 1);                              /* expected-error {{no matching function for call to 'InterlockedAdd'}} expected-note {{candidate function not viable: 1st argument ('const uint') would lose const qualifier}} expected-note {{candidate function not viable: no known conversion from 'const uint' to 'unsigned long long &' for 1st argument}} */
+  InterlockedAdd(gs_val[0], 1);                             /* expected-error {{no matching function for call to 'InterlockedAdd'}} expected-note {{candidate function not viable: 1st argument ('const uint') would lose const qualifier}} expected-note {{candidate function not viable: no known conversion from 'const uint' to 'unsigned long long' for 1st argument}} */


Strange, why does this one (and next two) say to 'unsigned long long' instead of 'unsigned long long &' like the first one?
And why would it be unsigned long long in the first place, since I presume this should take a 32-bit reference?

Another "quirk" of how atomics are lowered. The dest param is not made a reference until the candidate function is first added to the used intrinsics. Subsequent candidates are found to match the existing so their params don't get converted. It's only an issue for matching failures. We should talk about why it was designed this way in the first place.

tex3d · 2022-05-16T16:31:31Z

tools/clang/test/HLSL/atomics-on-bitfields.hlsl

+  InterlockedOr(str[tid.y].q, 2);                           /* expected-error {{no matching function for call to 'InterlockedOr'}} expected-note {{candidate function not viable: no known conversion from 'uint' to 'unsigned int' for 1st argument}} expected-note {{candidate function not viable: no known conversion from 'uint' to 'unsigned long long' for 1st argument}} */
+  InterlockedCompareStore(str[tid.y].q, 3, 1);              /* expected-error {{no matching function for call to 'InterlockedCompareStore'}} expected-note {{candidate function not viable: no known conversion from 'uint' to 'unsigned int' for 1st argument}} expected-note {{candidate function not viable: no known conversion from 'uint' to 'unsigned long long' for 1st argument}} */
+
+  InterlockedOr(gs.q, 2);                                   /* expected-error {{no matching function for call to 'InterlockedOr'}} expected-note {{candidate function not viable: 1st argument ('__attribute__((address_space(3))) uint') is in address space 3, but parameter must be in address space 0}} expected-note {{candidate function not viable: no known conversion from '__attribute__((address_space(3))) uint' to 'unsigned long long' for 1st argument}} */


It's a shame about the address space error, since it's misleading.

Less so than the previous I'd say. Bitfields just make the compiler do your shifting and casting for you. The result of that operation is a local. Granted, shader authors may not know that. We could consider creating some custom mismatch types at some point. That would be useful in all versions of LLVM unless they've seriously overhauled this system

AppVeyorBot · 2022-05-16T23:08:04Z

✅ Build DirectXShaderCompiler 1.0.1647 completed (commit 2d493afa0b by @pow2clk)

Change some function signatures and local variables in SemaHLSL to try to avoid mistakenly comparing matching opcodes from different namespaces

AppVeyorBot · 2022-05-17T02:58:14Z

✅ Build DirectXShaderCompiler 1.0.1651 completed (commit 0d8242bcc6 by @pow2clk)

pow2clk requested review from gracejennings, llvm-beanz and tex3d May 12, 2022 05:43

pow2clk commented May 12, 2022

View reviewed changes

simplify atomic verifier shader

192090e

python3kgae reviewed May 12, 2022

View reviewed changes

tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl Outdated Show resolved Hide resolved

tex3d reviewed May 12, 2022

View reviewed changes

tools/clang/test/DXILValidation/atomics.hlsl Outdated Show resolved Hide resolved

add lib test, refine restype test

1cd4f13

add test for reviewer commented lib export func test remove unintended and fairly weird unsubscripted references to resources in restypes test add more specific checks for unfound atomics for resources in restypes

pow2clk commented May 13, 2022

View reviewed changes

Add Zi to get automated test to replace proper variable

f1a7a61

Make atomicsconst validation test reg name independent

a1beefd

remove hack left in by mistake

7efb0b0

tex3d approved these changes May 16, 2022

View reviewed changes

Include verifier tests in script

73049ac

Add some foolproofing to opcode checks in codegen

35d8c06

Change some function signatures and local variables in SemaHLSL to try to avoid mistakenly comparing matching opcodes from different namespaces

pow2clk merged commit caec6aa into microsoft:main May 17, 2022

pow2clk deleted the disallow_bad_atomics branch May 17, 2022 04:26

Disallow illegal Atomic targets + propagate consts #4453

Disallow illegal Atomic targets + propagate consts #4453

Conversation

pow2clk commented May 12, 2022

Uh oh!

pow2clk May 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented May 12, 2022

Uh oh!

AppVeyorBot commented May 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented May 13, 2022

Uh oh!

pow2clk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented May 14, 2022

Uh oh!

AppVeyorBot commented May 16, 2022

Uh oh!

AppVeyorBot commented May 16, 2022

Uh oh!

AppVeyorBot commented May 16, 2022

Uh oh!

AppVeyorBot commented May 16, 2022

Uh oh!

tex3d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tex3d May 16, 2022

Choose a reason for hiding this comment

Uh oh!

pow2clk May 16, 2022

Choose a reason for hiding this comment

Uh oh!

tex3d May 16, 2022

Choose a reason for hiding this comment

Uh oh!

pow2clk May 16, 2022

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented May 16, 2022

Uh oh!

AppVeyorBot commented May 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants