DXIL operations should use is_const=True for constant arguments. This
allows for convenience methods to retrieve the constant value, and could
(should, but currently doesn't) result in validation that the argument
is constant.
`BarrierByNodeRecordHandle` SemanticFlags argument must be constant.
`MetadataIdx` for both `createNodeOutputHandle` and
`CreateNodeInputRecordHandle` must be constant.
PIX's code for parsing debug data operates at the module level. When the
same global is referenced by multiple functions in a module, that
variable is referred to by multiple dbg.value/dbg.declare statements,
and those are mapped (by the PIX passes) to multiple fake allocas using
its usual scheme. This code was written before libraries were a thing,
and wasn't expecting this duplication. A little more attention to the
variable's scope fixes the issue.
Also, the changed code's original "return false" broke the whole process
of discovering variables with the results that PIX's shader debugger
locals window was completely empty. Makes more sense to ignore the one
variable and keep going.
The spir-v backend currently treats each bitfield as a separate member
in the spir-v type when processing an initializer list. This commit
changes that so that it matches the behaviour of the DXIL backend.
In particular, if `{bf1, bf2}` is used to initialize a struct with two
bitfield, then `bf1` and bf2` will be merged into a single field when
constructing the struct.
Fixes#5688
Made the debug info type for the `this` argument a reference type.
`DxilRewriteOutputArgDebugInfo` then turns the reference type into a
value type to preserve the value mapping when the alloca that `this`
points to becomes registers.
When SV_CULLPRIMITIVE is the output of a mesh shader, its type is
changed to uint because a Vulkan interface variable cannot be a bool.
However, we do not change the access to the variable to match.
This change will modify stores to the variable to be stores of a uint
instead of a bool.
Fixes#6042
This change switches from instrumentation per instruction to
instrumentation per basic block. Furthermore, not every instruction in a
basic block needs to write debug data to the output UAV- it's enough to
know that the basic block was entered, as long as the calling
application can figure out what instructions were in that block. To
support that knowledge, the pass now emits a text "precis" of each basic
block.
Also, the previous branchless UAV bounds enforcement was replaced with
something similar that emits fewer instructions at the cost of a larger
UAV. This tradeoff is WELL worth it.
Additionally, the debug pass used to add extra blocks in order to
solidify the arguments to phi instructions. This work was unnecessary,
and added a lot of complexity to the resulting instrumented shader. The
debugger application is only interested in the value of the phi itself
and the actual value produced via the actual preceding edge.
Full details are in the comments in the code.
This change reduces driver-side compilation overhead from "overnight" to
2 minutes on a 160k-instruction shader.
This change adds a ground truth path normalization helper:
- All slashes are changed to system native (`\` for windows, `/` for all
others)
- All repeated slashes are removed (except for leading slashes, so
windows UNC paths are not broken)
- All relative paths (including the main file, and ones that begin with
`..`) are prepended with `./` or `.\` if not already
The path normalization is applied in the following places:
- Main file path in dxcompilerobj
- Paths passed into IDxcIncludeHandler
- Paths written into all DI* debug info
- Paths written into dx.content metadata and Pdb SourceInfo
- All paths loaded up by DxcPdbUtils
The reason for this change is to make it easier to tools authors to
implement recompilation (example: PIX edit-and-continue). When the paths
in all the above places match, the files can be matched with a normal
string equal instead of having to path normalization.
In implementing this PR:
https://github.com/microsoft/DirectXShaderCompiler/pull/6243
somehow, the condition to only emit the warning for library shaders was
left out.
This PR is a follow up on #6243 to properly restrict the emission of
this warning for non-library shaders.
This should properly fix#6100
Metadata loading for node record properties assumed fixed positions for
values in a (key,value) property list, skipping and ignoring the keys.
This was not correct, fragile, and not the intended implementation of
the key,value property pattern.
There is no functional change, but this makes the loader robust to
additional properties instead of crashing or loading the wrong thing.
Fixes#6276
During flat convertion, we have to pick the bit width for literals
because they could be used in multiple places as different sizes, and
that breaks the literal visitor. We currently use 32-bit types because
that is the most common use case, and we don't want to unnecessarily
add a capability.
However, if we pick a 64-bit width, we should be able to fold all of
the operations on literals. If they are only used as 32-bit value, then
ultimatlly fold them to 32-bit constants and remove the 64-bit
capabilities.
This commit updates the spir-v submodule to pull in the 64-bit interger
folding changes, and then starts to use 64-bit integer literal in flat
conversion.
Note that the problem still exists for floating point values. Spirv-opt
does not fold OpFConvert yet, we still treat the literals like a float.
Fixes#6188
---------
Co-authored-by: Natalie Chouinard <chouinard.nm@gmail.com>
We are able to pass a variable as the index to GetAttributeAtVertex, but
only if it is signed. If the value is unsigned, no cast is needed, and
the current code will skip the LValueToRValue cast. This causes the
load to be skipped generating invalid code.
Upon further discussion, the team has agreed that in certain degenerate
cases, the current diagnostics are insufficient.
In the case that min == max in the wave size range attribute, a
defaultError warning should be emitted. Additionally, there should be an
explicit way to handle the case where 0,0,0 is passed to the wavesize
range attribute.
This PR directly handles and tests both of these cases.
Fixes#6161
---------
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Add ShaderKind::Last_1_8 for shader mask
Add shader model comments before flag groupings in DxilConstants.h and DxilShaderFlags.
Add missing flag checks for min shader model in RDAT function info. Move ShaderCompatInfo computation into DxilModule, propagate callee info.
Move computation of shader model requirements based on flags into DxilModule. Finalize requirements for entry functions in AdjustMinimumShaderModelAndFlags.
Fixes for function level flag tracking:
- DerivativesInMeshAndAmpShaders: use flag to track deriv use, then adjust for entry functions.
- hasUAVs: based on resource use in function instead of global resources.
- WriteableMSAATextures: based on use in function instead of global resources.
- Also catch cases for dynamic res from any use by looking at create/annotate handle, not just the TextureStoreSample op.
- RaytracingTier1_1: move module-level detection to CollectShaderFlagsForModule
- Marked deriv and quad ops as being supported in node.
- Fixed SampleCmpBias to be considered gradient op.
- Update RDAT definitions to dump more useful info for testing
Fixes#6218.
For every attribute that is only meaningful on an entry-point, produce a
warning indicating that this could be a problem.
The attributes are ignored, so it's harmless, but it still might
indicate a mistake on the part of the user.
Previously, some of these would cause the function to be implicitly made
a certain kind of entry point.
These warnings will soften the impact of that change.
Fixes#6100
---------
Co-authored-by: Joshua Batista <jbatista@microsoft.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
According to this section of the spec:
https://microsoft.github.io/DirectX-Specs/d3d/WorkGraphs.html#node-shader-system-values
There should be diagnostics announcing the unsupported use of certain
system value semantics in combination with certain supported launch
modes. There were previously no diagnostics for these incompatible
combinations.
This PR adds the diagnostics into Sema.
According to issue
https://github.com/microsoft/DirectXShaderCompiler/issues/5827, there is
no path for proper flattened signature construction. Parameters won't be
fully flattened at the time that Sema runs, so the diagnostics in this
PR only trigger on simple parameters with low dimensionality.
The team has agreed to leave the full implementation (which can address
higher-dimensional parameters) to the implementation of a proper
parameter parsing infrastructure, which will take place in the
modernization effort.
Fixes https://github.com/microsoft/DirectXShaderCompiler/issues/5827
---------
Co-authored-by: Greg Roth <grroth@microsoft.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
The SPIR-V path currently fails when a variable is passed at the index
to GetAttributeAtVertex. Not only should we not crash, but the variable
does not seem to be a problem for the SPIR-V validator. So we can simply
generate an access chain with a variable idx.
In the current implementation, a single OpAccessChain is generated for
all array subscripts and member accesses. This causes a problem if the
member access is an aliased variable, and we need to generate and
OpAccessChains for the member, a load, and then another OpAccessChain
to index into the aliased array.
This change cause an array subscript to be process as a single
OpAccessChian. Note that we still process array sub scripts with the
member access when it is a subnode of the member access. We will
probably need to fix that up later as well, but I want to keep the
change small.
Fixes https://github.com/microsoft/DirectXShaderCompiler/issues/6110.
Mesh shader output variables may be decorated with the `nointerpolation`
attribute, which should be translated to a `Flat` decoration in the
SPIR-V backend.
Fixes#6250
This option is not enabled by default. When enabled, this option adds
the MaximallyReconvergesKHR execution mode to the module, and the
associated extension.
Related to #6222
---------
Signed-off-by: Nathan Gauër <brioche@google.com>
One of the current test has an out of bounds memory access in a vector.
We access `scalars[0]` vector after all of the elements have been
removed. This is used to get the source location. The solution is to get
the source location earlier, and use the local variable.
No new test is needed because a current test exposed the problem on a
platform and build that asserts when the access happens.
These TODOs were intended for if we were to implement deep RDAT
validation rather than relying on the exact binary comparison.
This isn't happening for SM 6.8, so these are no longer
PRERELEASE-TODOs.
The comment in RDAT_Macros.inl didn't really belong there in the first
place, so I removed it.
Fixes#5699.
An "aliased" variable is on where the compiler has implicitly added an
extra level indirection. So when we access one of these variable, we
have to do an extra load.
When we do an array access on an aliased variable, we do the extra load
before doing the access chain, but we still treat the result of the
access chain as if it is an aliased value. This causes an extra load and
generates invalid spir-v.
The fix is to change `isReferencingNonAliasStructuredOrByteBuffer` so
that we do not distinguish between externally and internally visible
variable if we see an array subscript.
Fixes#6110
Vulkan allows SubpassInputs without an input attachment index specified,
so the error when it is missing has been removed and tests have been
updated.
As far as I can tell, there aren't earlier Vulkan or SPIR-V versions
where the presence of the the input attachment index was explicitly
required, so allowing it to be omitted in all cases.
Also added a test to verify that this fixes#2808 at the same time.
Fixes#6238, #2808
According to the Vulkan spec:
OpTypeImage must declare a scalar 32-bit float, 64-bit integer, or
32-bit integer type for the “Sampled Type” (RelaxedPrecision can be
applied to a sampling instruction and to the variable holding the result
of a sampling instruction)
We want to avoid translating a true HLSL 16-bit floating point texture
object to a 32-bit texture object with relaxed precision, as this will
break backwards compatibility if Vulkan does support 16-bit texture
types in the future.
This change adds an error message in cases where the code generated
would otherwise fail validation for the above reasons.
Closes#4372
When not updating the in-tree sources, the compare step would always run
because the comparison was part of the target. This prevents that by
having the comparison be its own custom command that produces an output
and the target depends on the "output" of the comparison step.
- This changes the FeatureManager constructor, so that EXT_mesh_shader
is still the default choice for mesh shaders when SPIR-V 1.4 or above is
supported, but is not forced even when NV_mesh_shader is requested.
Before, it was impossible to choose NV_mesh_shader in that case.
Fixes#6193
Two related sets of changes:
1) Modify the value-to-declare pass to tolerate bitfields, which are
detected by their habit of being smaller than the type in which they are
contained. The pass produces dbg.declare statements that map each HLSL
variable to some storage. With bitfields, that mapping becomes
many-to-one, because many bitfields can be stored in the same basic
type. Most of the changes are to preserve the size of the bitfield as
the code descends the type hierarchy into the underlying basic type,
which is what motivates these new "type overrride" parameters to some
functions. Complications to watch out for are min16 types, which may or
may not be their own purpose-made (16-bit) types depending on whether or
not they are enabled.
2) Modify the PIX debug-info API to return enough data to PIX to allow
it to extract (by bit shifting) a bitfield from the underlying type.
This bit is also about preserving knowledge of the field's size as
opposed to its containing type's size, hence the new dxcapi
interface/method.
---------
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This PR improves the experience for debugging tools that consume SPIR-V
produced by passing in-memory strings to dxcompiler.dll. When dxc.exe is
used, OpSource and OpDebugSource are populated by reading original
source from the filesystem. dxcompiler.dll users instead find their
input string is preprocessed, which is confusing behavior.
Contrary to the leading comment in the code this change modifies,
debugging tools do not want preprocessed source. Such tools show and
allow editing of include files. If everything is pasted into a single
source string, finding what to edit is difficult. Editing the
preprocessed source has the additional complication that a user would
have to edit also the #line markers, which is hard to get right
manually. In the preprocessed source, all comments are lost, which makes
it hard to read the code. Finally in preprocessed source, all macros and
code under conditional compilation are lost. That will make it
impossible to modify those parts in an edit.
Co-authored-by: Steve Urquhart <stevur01@ntd.nintendo.com>
Fix#6230
The literal type visitor is asserting that the AST result type of an
OpLoad should not be literal. The following expression was encountering
this assertion.
float_vec_4 *= condition ? -1.0 : 1.0;
This patch visits OpVectorTimesScalar operations to change the AST
result type of an instruction from a literal type to a non-literal type
where possible.
Also, fix some mispellings I encountered while navigating the code.
When the env isn't valid, we emit an error. But instead of returning, we
continued and still tried to load the optional value, which triggered an
assert.
Signed-off-by: Nathan Gauër <brioche@google.com>
The implementation of flat conversion in the spir-v codegen is very
adhoc. This changes the implemetation to match what is done in DXIL. For
now I have only reimplemented to catch all case. I will leave the
special cases as they seem to be correct for what they do, and will
generate less verbose code.
Fixes#4906
---------
Co-authored-by: Nathan Gauër <github@keenuts.net>
The VulkanMemoryModelDeviceScope capability is now covered by the
SPIRV-Tools capability trimming pass, so it should be added here
unconditionally now.
Fixes#6066
We recently removed the FileTest infrstructure because we were able to
replace all of the tests with LIT tests. However, we now have to test
the a case were the source code is in memory and not in a file (see
`FileTest`, buti specializes it so that it cannot be given a file name,
and
remove the uses of EFFCEE.
One test is added as an example on how to use a REGEX matcher to check
the assemble output.
Other options we considers:
1. Adding new infrastructure based on the code in HLSL/CompilerTest.cpp.
However that was more complex that we need. I wanted a simple
interface so that it is easy for people to add new tests. I also wanted
to make it spir-v specific.
2. Modifying the DXC executable to not pass along the file name. This
was rejected because it would place an extra option, just for
testing, the public facing executable.
3. Have the dxc executable accept `-` at the input file to have it read
the input file from stdin. Similar to 2. Rejected for similar
reasons.
This moves functions for evaluating constant expressions and translating
LLVM AP types to SPIR-V constants into a new class. These functions need
to be used from LowerTypeVisitor in #6156, so I am factoring them out to
avoid duplication.
Tex Riddell
Jeff Noyle
Steven Perron
Adam Yang
Cassandra Beckley
Xiang Li
Joshua Batista
Greg Roth
Natalie Chouinard
Nathan Gauër
Chris B
rj12122
Steve Urquhart
Jeremy Hayes
Helena Kotas