We replace getting the id of a poitner type with a specific funciton
call to FindPointerToType. Also, FindPointerToType is updated to not
indirectly call GetId. This leads to a linear search for an existing
type in all cases, but it is necessary.
Note that this function could have a similar problem. There could be two
pointer types with the same pointee and storage class, and the first one
will be returned. I have checked the ~20 uses, and they are all used in
situations where the id is used to create something new, and it does not
have to match an existing type. These will not cause problems.
Part of #5691
* Avoid use of type manager in extact->construct folding
When dealing with structs the type manager merge two different structs
into a single entry if they have all of the same decorations and
element types. This is because they hash to the same value in the hash
table. This can cause problems if you need to get the id of a type from
the type manager because you could get either one. In this case, it
returns the wrong one.
The fix avoids using the type manager in one place. I have not
looked closely at other places the type manager is used to make
sure it is used safely everywhere.
Fixes#5624
* Remove use of TypeManager::GetId
This removes a use of TypeManager::GetId by keeping the id around. This
avoid a potential problem if the type manager gets confused. These types
of bugs are hard to generate test cases for, so I do not have a test.
However, existing tests make sure that do not regress.
* Basic support for SPV_EXT_replicated_composites
Validation will follow as a separate PR (still need to write a test suite)
Change-Id: Ic95fa6ce39d32f5ac2787bc38dba2748c9cc58f7
Signed-off-by: Kevin Petit <kevin.petit@arm.com>
* Update SPIRV-Headers
Change-Id: I6c0df248d99c13b49d78528d035a4222027c0232
---------
Signed-off-by: Kevin Petit <kevin.petit@arm.com>
This removes the folding rules added in #4783 and #4808. They lead to
poor code generation on Adreno devices when 16-bit floating point values
were used. Since this change is transformation is suppose to be neutral,
there is no general reason to continue doing it.
I have talked to the owners of SwiftShader, and they do not mind if the
transform is removed. They were the ones the requested the change in the
first place.
Fixes#5658
The Scope operand of `OpReadClockKHR` was always validated using the
Vulkan environment rules, which only allow `Subgroup` or `Device`.
For the OpenCL environment, `Workgroup` is also a valid Scope, so
`Workgroup` should not be rejected in the universal environment.
Guard the existing Scope check behind `spvIsVulkanEnv` and add a new
Scope check for the OpenCL environment.
Signed-off-by: Sven van Haastregt <sven.vanhaastregt@arm.com>
* Check for matrix decorations on arrays of matrices
* MatrixStide, RowMajor and ColMajor can be applied to matrix or
arrays of matrix members
* Check that matrix stride satisfies alignment in arrays
Reject `OpCooperativeMatrixStoreKHR` with a `MakePointerVisibleKHR`
MemoryAccess operand, as `MakePointerVisibleKHR` is not supposed to be
used with store operations.
The `CoopMatKHRStoreMemoryAccessFail` test failed to catch this
because it used the helper function `GenCoopMatLoadStoreShader` which
generates `...NV` instead of `...KHR` instructions. Add a new helper
function to generate similar shaders for the KHR extension, as the NV
and KHR extensions have various subtle differences that makes
parameterizing the original helper function non-trivial.
Signed-off-by: Sven van Haastregt <sven.vanhaastregt@arm.com>
* Disallow duplicate decorations generally
* Only FuncParamAttr and UserSemantic can be applied to the same target
multiple times
* Unchecked: completely duplicate UserSemantic and FuncParamAttr
* Disallow duplicate execution modes generally
* Exceptions for float controls, float controls2 and some intel
execution modes
* Fix invalid fuzzer transforms
This fixes the problem reported in #5623 using the observation that if
we are re-building a type that already exists in the type pool, we
should just return that type.
This makes type rebuilding more efficient, and it also prevents the
type builder from getting itself into infinite recursion (as reported in
this issue).
In fixing this, I found a couple of other bugs in the type builder:
- When rebuilding an Array type, we were not re-building the element
type. This caused stale type references in the rebuilt type.
- This bug had not been caught by the test, because the test itself had
a bug in it: the test was rebuilding types on top of the same ID (the
ID counter was never incremented).
Initially, the bug in the test caused a failure with the new logic in
the builder because we now return types from the pool directly, which
causes a failure when two incompatible types are registered under the
same ID.
Fixing that issue in the test exposed another bug in the rebuilder: we
were not re-building the element type for Array types. This was causing
a stale type reference inside Array types which was later caught by the
type removal logic in the test.
All tests treat kAnalysisEnd like STL end iterators, which
means its value must be greater than that of the last valid
Analysis.
Change-Id: Ibfaaf60bb450c508af0528dbe9c0729e6aa07b3b
Signed-off-by: Kevin Petit <kevin.petit@arm.com>
Flagged by -Wredundant-decls
I'm assuming the declarations in libspirv.h are part of the external
interface and need to be kept.
Change-Id: I6b138d3322a7a4ee49ee33b0fbcf0ca35dd92261
Signed-off-by: Kevin Petit <kevin.petit@arm.com>
DXC does not do a good job of recognizing which variables need to be
on the entry point for which functions. This is because it does not
want to have to walk the call tree to determine which instructions
are reachable from which entry points.
This is also useful if the same input variable gets used from two
different shader, but the uses in one get optimized away.
Will parially fix
https://github.com/microsoft/DirectXShaderCompiler/issues/4621. Will not
fix code compiled with -fcgl.
* [OPT] Identify arrays with unknown length in copy prop arrays
The code in copy propagate arrays assumes that the length of an
OpTypeArray is known at compile time, but that is not true when the size
is an OpSpecConstant. We try to fix that assumption.
Fixes https://crbug.com/oss-fuzz/66634
* [OPT] Use new instruction folder for for all opcodes in spec consti folding
When folding and OpSpecConstantOp, we use the new instruction folder for
a small number of opcodes. This enable the new instruction folder for
all opcodes and uses the old one as a fall back. This allows us to
remove some code from the older folder that is now covered by the new
one.
Fixes#5499
Adds folding rules that will fold basic artimetic for signed and
unsigned integers of all sizes, including 64-bit.
Also folds OpSConvert and OpUConvert.
The extension SPV_KHR_maximal_reconvergence adds more constraints
around the merge blocks, and how the control flow can be altered.
The one we address here is explained in the following part of the spec:
Note: This means that the instructions in a break block will execute as if
they were still diverged according to the loop iteration. This restricts
potential transformations an implementation may perform on the IR to match
shader author expectations. Similarly, instructions in the loop construct
cannot be moved into the continue construct unless it can be proven that
invocations are always converged.
Until the optimizer is clever enough to determine if the invocation
have already converged, we shall not meld a block which branches to a
merge block into it, as it might move some instructions outside of the
convergence region.
This behavior being only required with the extension, this commit
behavior change is gated by the extension.
This means using wave operations without the maximal reconvergence
extension might lead to undefined behaviors.
Co-authored-by: Natalie Chouinard <chouinard.nm@gmail.com>
Add OpEntryPoint to the list of instructions processed by the
DescriptorScalarReplacement pass. This is necessary for SPIR-V 1.4 and
above where global variables must be included in the interface.
Fixesmicrosoft/DirectXShaderCompiler#5962
Add the VulkanMemoryModelDeviceScope capability to the capability
trimming pass. According the the spec, "If the Vulkan memory model is
declared and any instruction uses Device scope, the
VulkanMemoryModelDeviceScope capability must be declared." Since this
case, based on the type of an operand, is not covered by the JSON
grammar, it is added explicitly.
The optimizer is able to preserve the interface variables of the
shaders, but that feature has not been exposed to the command line
tool.
This commit adds an option `--preserve-interface` to spirv-opt that will
cause all calls to ADCE to leave the input and output variables, even if
the variable is unused. It will apply regardless of where the option
appears on the command line.
Fixes#5522
Certain versions of GCC warn about these variables being potentially uninitialized when used.
I believe this is a false-positive, but zero-init'ing them is a safe way to fix this.
The PhysicalStorageBufferAddresses capability can now be
trimmed. From the spec, it seems any instruction enabled by this
required some operand to have the PhysicalStorageBuffer storage class.
This means checking the storage class is enough.
Now, because the pass uses the grammar, we don't need to add any
new logic.
Signed-off-by: Nathan Gauër <brioche@google.com>
The StorageImageReadWithoutFormat capability is only required when
an image type with the format set to Unknown is used with some specific
OpImageRead or OpImageSparseRead instructions.
This patch adds the required code to the capability trimming pass to
remove the StorageImageReadWithoutFormat capability when not required.
Signed-off-by: Nathan Gauër <brioche@google.com>
The function that get the number of elements in a composite variable
returns an incorrect values for the arrays. This is fixed, so that it
returns the correct number of elements for arrays where the number of
elements is represented as a 32-bit integer and is known at compile
time.
Fixes#4953
* Remove references to __FILE__
Uses of `__FILE__` leak the directory structure of the machine used to
build because it adds a string to the string table with the full path
name. I've removed the uses that show up in the release builds.
Fixes#5416
The SPIR-V specification allows any scalar integer type as an index. DXC
usually emits indexes as 32-bit integer types, however, in some cases it
is possible to make it emit 64-bit indexes instead (as in
https://github.com/microsoft/DirectXShaderCompiler/issues/5638).
* Add ComputeDerivativeGroup*NV capabilities to trim capabilities pass.
* Add SPV_NV_compute_shader_derivatives to allow lists
No tests needed for this. The code path is well tested. Just adding new
data.
spirv-link requires that memory models match between its input files.
Ensure this is the case by always returning SuccessWithChange and
always changing the memory model to match the instrumentation
code in Vulkan-ValidationLayers.
Also, disable the DCE pass in the --inst-* command line options, since
it will only work after linking.
Add a new legalization pass to dedupe invocation interlock instructions
DXC will be adding support for HLSL's rasterizer ordered views by using
the SPV_EXT_fragment_shader_interlock_extension. That extension
stipulates that if an entry point has an interlock ordering execution
mode, it must dynamically execute OpBeginInvocationInterlockEXT and
OpEndInvocationInterlockEXT, in that order, exactly once. This would be
difficult to determine in DXC's SPIR-V backend, so instead we will emit
these instructions potentially multiple times, and use this legalization
pass to ensure that the final SPIR-V follows the specification.
This PR uses data-flow analysis to determine where to place begin and
end instructions; in essence, determining whether a block contains or is
preceded by a begin instruction is similar to a specialized case of a
reaching definitions analysis, where we have only a single definition,
such as `bool has_begun = false`. For this simpler case, we can compute
the set of blocks using BFS to determine the reachability of the begin
instruction.
We need to do this for both begin and end instructions, so I have
generalized portions of the code to run both forward and backward over
the CFG for each respective case.
These functions are getting far too complicated to code in SPIRV-Tools
C++. Replace them with import stubs so that the real implementations
can live in Vulkan-ValidationLayers where they belong.
VVL will need to define these functions in spirv and link them to the
instrumented version of the user's shader.
From here on out, VVL can redefine the functions and any data they use
without updating SPIRV-Tools. Changing the function declarations will
still require both VVL and SPIRV-Tools to be updated in lock step.
From the Capability's text in the SPIRV spec:
```
An MS operand in OpTypeImage indicates multisampled, used with an
OpTypeImage having Sampled == 2 and Arrayed == 1.
```
Adding this logic to the capability trimming pass.
Adds the RayTracingKHR and RayQueryKHR capabilities to
the supported capabilities list (this includes the linked extension).
(NV and KHR capabilities/extensions shared the same IDs, so it also
works for NV flavors of those).
Currently spirv-link fails if all input files don't use the same
SPIR-V version. Add an option to instead use the highest input
version as the output version. Note that if one of the 'old'
input files uses an opcode that is deprecated in the 'new'
version, the output spirv will be invalid.
Some operands are not simple values, but bitmasks.
The lookup in the table for required decomposing the mask into
single values.
This commit adds support for such operands, like MinLod|Offset.
A token is allowed to parse even when it's from the wrong
version, or is not enabled by a capability or extension.
This allows more modules to parse.
Version/capability/extension checking is fully moved to
validation instead.
Fixes: #5364
* opt: fix StorageInputOutput16 trimming.
While integrating this pass into DXC, I found a lot of missing
cases. This PR fixes a few issues centered around this capability
while laying out fondations for more fixes.
1. The grammar can define extensions in operand & opcode tables.
- opcode can rely on common capabilities, but require a new
extension.
- opcode can also rely on a capability which requires an extension.
Sometimes, the extension is listed twice, in the opcode, and
capability. But this redundancy is not guaranteed.
2. minVersion check. The condition was flipped: we added the extension
when the minVersion was less than current.
Didn't noticed the issue as I only tests on the default env.
3. Capability/Extension instructions were not ignored.
- `OpCapability Foo` will require the `Foo` capability.
- it doesn't mean the module requires the `Foo` capability.
Same for extensions.
This commit adds disabled tests, for fixes which are too large to
be brought into this already large PR.
Multiple calls to this function were causing vkCreateGraphicsPipelines
to be 3x slower on some driver. I suspect this was because each
call had to be inlined separately which bloated the code and caused
more work in the driver's SPIRV -> native instruction compilation.
This commit adds a new optimization which tries to remove unnecessary
capabilities from a SPIR-V module.
When compiling a SPIR-V module, you may have some dead-code using
features gated by a capability.
DCE will remove this code, but the capability will remain. This means
your module would still require some capability, even if it doesn't
require it. Calling this pass on your module would remove obsolete
capabilities.
This pass wouldn't be enabled by default, and would only be usable
from the API (at least for now).
NOTE: this commit only adds the basic skeleton/structure, and
doesn't mark as supported many capabilities it could support.
I'll add them as supported as I write tests.
Signed-off-by: Nathan Gauër <brioche@google.com>
The code currently tries to get the value of the floating point constant
to see if it is -0.0. However, we are not able to get the value for
16-bit floating point value, and we hit an assert.
To avoid this, we add an early check for the width to make sure it is
either 32 or 64.
Fixes https://github.com/microsoft/DirectXShaderCompiler/issues/5413.
Expanding a bit the EnumSet API to have iterator-based
insert and constructors (like the STL).
This is also a pre-requisite from the capability-trimming pass as
it allows to build a const set from a constexpr std::array easily.
Signed-off-by: Nathan Gauër <brioche@google.com>
GetCapabilities returned a const*, and GetExtensions did not exist.
This commit adds GetExtensions, and changes the return value to
be a const&.
This commit also removes the overload to GetCapabilities which returns
a mutable set, as it is unused.
Signed-off-by: Nathan Gauër <brioche@google.com>
When using PhysicalStorageBuffer it is possible for a function to
return a pointer type. This was not being handled correctly in
`GetLoadedVariablesFromFunctionCall` in the DCE pass because
`IsPtr` returns the wrong result.
Fixes#5270.
