Fixes#2456
* When eliminating a structured construct that has an unreachable merge,
replace that unreachable terminator with an appropriate return
* New tests
Fixes#2488
* Validator doesn't identify back-edge of the loop, so the merge is
never set
* Construct::blocks() has safe uses of `merge` so the assert can be
removed
* Added a test
Fixes#2453
* Enable addition of OpPhi instructions when the loop has multiple
predecessors of the merge due to a break
* This can result in some values no longer dominating their uses
* Track return blocks in structured flow to produce OpPhis that have
multiple undef and non-undef arguments
* New tests to catch the bug
* When a block is predicated, mark the new body as a return if the old
block as already a return
* Fix#2478. The fix is to just not try to simplify such loops.
* Also added `BasicBlock::MergeBlockId()` and `BasicBlock::ContinueBlockId()`.
* Some minor changes to `structured_loop_to_selection_reduction_opportunity.cpp`.
* Added test.
Fix#2475. Fix#2476.
* Improve reducer algorithm: shrink granularity, remove an early return, no lazy initialization, notify pass if binary is interesting, add comments.
* Add fail-on-validation-error option to fail a reduction if an invalid state is reached; useful for tests.
* Set fail-on-validation-error in tests.
* Improve some documentation comments.
* Add Reducer::AddDefaultReductionPasses so tests (and other library consumers) can add the default reduction passes.
* Add CLIMessageConsumer in test_reduce so we can see messages for tricky tests.
* Remove test RemoveUnreferencedInstructionReductionPassTest_ApplyReduction because it was indirectly testing the reduction algorithm, not the RemoveUnreferencedInstruction pass.
* Tweak tests where needed.
Fix#2396
* Check that initial state is valid. Add kInitialStateInvalid.
* Fix RemoveOpnameAndRemoveUnreferenced test; turns out the original shader is invalid, but we never notice because we don't check this and the reduced shader is valid; fix original shader. Assert reduction status is kComplete.
* Always check return value from `Reducer::Run`.
* Change Reducer::Run to *not* immediately copy the input binary.
Changing the stored value for a sampled image consumer to be the
instruction instead of result ID, since not all instructions have
result IDs. Using result IDs led to a potential crash when using
OpReturnValue, which doesn't have a result ID. OpReturnValue is not a
legal consumer, but the validator needs to look at the instruction to
determine this, thus storing the pointer to the instruction, instead
of trying to fetch the pointer using the instruction.
Issue #1528 covers fixing the check.
Fixes#2463
If SPV_EXT_descriptor_indexing is enabled, add check that for a
descriptor-based reference, the descriptor is initialized. Initialization
data is stored in the debug input buffer, added to the length information
already there. This feature must be seperately enabled on the pass
creation routine. NOTE: Currently just supports image references; buffer
references are still TODO.
Adds an optimization pass to remove usages of AtomicCounterMemory
bit. This bit is ignored in Vulkan environments and outright forbidden
in WebGPU ones.
Fixes#2242
If relax-logical-pointer is enabled, this commit makes Validator
accept function param even when its Storage Class is different from
the expected one.
Related to #2423, #2430
In constant propagation, decoration are transfered from the original
expression to the constant that will replace it. This can be wrong
because there are no decorations that apply to constants. We choose to
simply delete the decorations.
Fixes#2441
In WebGPU all blocks are required to be reachable, unless they are one of two
specific degenerate cases for merge-block or continue-target. This PR adds in
checking for these conditions.
Fixes#2068
* Handle back edges better in dead branch elim.
Loop header must have exactly one back edge. Sometimes the branch
with the back edge can be folded. However, it should not be folded
if it removes the back edge.
The code to check this simply avoids folding the branch in the
continue block. That needs to be changed to not fold the back edge,
wherever it is.
At the same time, the branch can be folded if it folds to a branch to
the header, because the back edge will still exist.
Fixes#2391.
In relaxed addressing mode, we want to accept non memory objects
because this is a very natural translation of hlsl. It should be fixed
by legalization by inlining the calls.
* Fix OpDot folding of half float vectors.
The code that folds OpDot does not handle half floats correctly. After
trying to multiple the first components, we get a nullptr because we
don't fold half float values. This nullptr gets passed to the code that
does the addition, and causes an assert.
Fixes#2405.
The types of input and output variables must match for the pipeline. We
cannot see the uses in all of the shader, so dead member
elimination cannot safely change the type of input and output variables.
Add a pass that looks for members of structs whose values do not affects
the output of the shader. Those members are then removed and just
treated like padding in the struct.
This is required to properly handle uses of forward declared ids. Since forward
declared ids were not being properly covered by the validator this uncovered a
bunch of small issues that needed to be resolved to get tests passing again.
Fixes#2373
* Fixes#2358. Added to the reducer the ability to remove a function that is not directly called. Factored out some code from the optimizer to help with this.
Fixes#2120
Enhanced the reducer so that it can merge blocks together, leveraging the functionality extracted from the block_merge pass in the optimizer.
* Fixes#2338. Added check for phi node before merging blocks.
* Added functionality to merge blocks A and B even when B starts with OpPhi instructions, by replacing uses of the OpPhi results with the definitions coming from A. Added some tests for this.
* Fixed assertion.
* Remove the static maps from CheckDecorationsCompatibility
There are a few data structures in the function
`CheckDecorationsCompatibility` that are allocated using `new` and their
address is stored in a static pointer. This code pattern causes the
MSVC memory leak checker to say there is a memory leak. Some people
are interested in keeping that clean.
To work around it, I have replaced them with either a function or an
array of POD types. The array can be kept as a static directly because
it has a trivial destructor, and we don't have to worry about it being
destroyed too early.
Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/2317.
This CL adds in the specific checks required for WebGPU, enables
running the builtin checks for WebGPU, and refactors the existing
testing infrastructure to support testing the new checks.
This PR is part of resolving #2276
* Add SpirvTools::IsValid().
Add a method to determine if a SpirvTools object was successfully
constructed and can be used. It might not be depending on the parameter
to the constructor.
This is something a fuzzer wants to know before trying to use an
SpirvTools object constructed with a fuzzed parameter.
In a recent PR, we allowed a forward reference for the element type in
an array declaration. However, we do not have other check to make sure
the forward reference is a pointer type first reference in
OpTypeForawrdPointer. We add that check.
Fixes https://crbug.com/920074.
When looking at the uses of the result of an instruction, code sinking
assumes that all uses are in a basic block. However, this is not true
if there is a decoration or name for the result of that insturction.
This commit checks for this.
Fixes https://crbug.com/923243.
It is legal, but not generated by any SPIR-V producer: an OpCompositeExtract
with no indexes. This is essentially just a copy of the object, so we
treat them that way. We simply propagate the live variables of the
result to the operand.
Fixes https://crbug.com/919181.
It is legal, but not generated by any SPIR-V producer: an OpAccessChain
with no indexes. This is essentially just a copy of the pointer.
I have decided to treat it like an OpCopyObject. In CheckUses, we
return that it is not okay.
When looking at this I realized that we had code in GetUsedComponents
that cannot be reached. If there is a use in an OpCopyObject the it
will not call GetUsedComponents. I removed that dead code.
Fixes https://crbug.com/918311.
During unrolling a new loop is created, but its ownership is not clear
as it gets passed through the code. Changed something to unique_ptr to
make that clearer.
Fixes#2299.
Fixing other memory leaks at the same time.
Fixes#2296Fixes#2297
In C++, a bit shift of the same size as the type is undefined, but it is
defined in spir-v. When folding those cases, we have to be careful. We
cannot simply do the shift in C++.
Fixes https://crbug.com/917697.
Fixes#2138
* Modf and frexp are upgraded to use the struct version of the
instruction and generate an explicit store whose flags can be upgraded
separately
* Fixed major bug where availability and visibility were reversed for
non-copy memory instructions
* Fixed bug where availability and visibility scope operands were reversed for copy memory
* Upgraded all opt tests to use SPV_ENV_UNIVERSAL_1_3
* Upgrade tests moved into unified tests and removed standalone test
Broader check for ids that require a type
Fixes https://crbug.com/911700
* Adds a broader check for when id operands require a type
* updated a few tests
* added a test to catch the original issue
* Handle CompositeInsert with no indices in VDCE
In the spec, there it nothing that forces an OpCompositeInsert to have
an index, but VDCE assumes there is at least 1 in a couple places.
This commit updates VDCE to handle these cases.
* Added additional changes for the new AccelerationStructureNV type.
* Added NVIDIA ray tracing storage classes for checking in ValidateVariable.
* For NVIDIA ray tracing storage classes added test to load bool type (allowed) in new storage class.
This Cl changes the binary parser to keep track of the instruction count
being processed. The parser will then use that instruction number as the
error number, instead of the binary word.
This should make it easier to match the error up to what the
disassembler would output for the error.
Issue #2091
In CMake, we are not suppose to have multiple targets depend on the same
custom command. To avoid this, we have to add a custom target around
the command. Then we have add the appropriate dependencies.
Fixes#1941.
Currently it is impossible to invalidate the constnat and type manager.
However, the compact ids pass changes the ids for the types and
constants, which makes them invalid. This change will make them
analyses that have to been explicitly marked as preserved by passes.
This will allow compact ids to invalidate them.
Fixes#2220.
* Added additional changes for the new AccelerationStructureNV type.
* Added additional changes for the new AccelerationStructureNV type. Change tabs to space...
* Added additional changes for the new accelerationStructureNV type -- add proper type name.
Fix TypeManager.TypeStrings test:
[----------] 29 tests from TypeManager
[ RUN ] TypeManager.TypeStrings
[ OK ] TypeManager.TypeStrings (7 ms)
When we are predicating the continue target for a loop, it can no longer
be the continue target because it will have a branch that exits the loop
and is not the bach edge. The continue target will have to be the
target of that branch that is still in the loop.
Fixes#2211.
The function `UpdatePhiNodes` was being called inconsistently. In one
case, the cfg had already been updated to include the new edge, and in
another place the cfg was not updated. This caused the function to
miss flagging a block as needing new phi nodes. I picked that the cfg
should not be updated before making the call. I documented it, and
change the call sites to match.
Fixes#2207.
We initially assumed that if the type manager returned the correct id
for the pointee type, that we would get the correct pointer type back,
but that is not true. See the unit test added with this commit. We
need to fall back to the linear search any time we are looking for a
pointer to a type that may not be unique.
At the same time, SROA considered an OpName on a variable to be a use of
the entire variable. That has been fixed.
Fixes#2209.
We currently place the load instructions at the start of the basic block
that dominates all of the loads. If that basic block contains OpPhi
instructions, then this will generate invalid code. We just need to
search for a location that comes after all of the OpPhi instructions.
Fixes#2204.
* Add OperandToUndefReductionPass.
Fixes#2115.
Also added some tests that are similar to those in OperandToConstantReductionPassTest.
In addition, refactor FindOrCreateGlobalUndef into reduction_util.cpp. Fixes#2184.
Removed many documentation comments that were identical or very similar to the overridden function's documentation comment.
* Don't fold specialized branchs in loop unswitch
Folding branches can have a lot of special cases, and can be a little
error prone. So I only want it in one place. That will be in dead
branch elimination. I will change loop unswitching to set the branches
that were being folded to have a constant condition. Then subsequent
pass of dead branch elimination will be able to remove the code.
At the same time, I added a check that loop unswitching will not
unswitch a branch with a constant condition. It is not useful to do it
because dead branch elimination will simple fold the branch anyway.
Also it avoid an infinite loop that would other wise be introduced by my
first change.
Fixes#2203.
Loop unswitching is unswitching the conditional branch that creates the
back-edge. In the version of the loop, where the bachedge is not taken,
there is no back-edge. This is what causes the validator to complain.
The solution I will go with will be to now unswitch a condition with a
back-edge. At this time we do not now if loop unswitching is used. We do
not include it in the optimization sets provided, nor is it used in
glslang's set. When there are opportunities and no breaks from the loop,
the loop with either be a single iteration loop, or an infinite loop.
There is no performance advantage to performing loop unswitching in
either of those cases. If there is a break, maintaining structured
control flow will be tricky. Unless we see a clear advantage to handling
these case, I would go with the safer simpler solution.
Fixes#2201.
If there are multiple edges to a basic block, then the ssa rewriter will
create OpPhi instructions with duplicate entries. This is invalid, and
it is fixed in this commit.
Fixes#2202.
In the function `AssemblyContext::binaryEncodeString`, we want to copy
a nul terminated string to an instruction. When coping the string, we
did not copy the nul at the end of the source. It was added by setting
the entire last word to 0, which is mandated by the spir-v spec. This
is not a bug, but it does trigger a warning in GCC8 when doing a release
build.
To avoid the warning, we will copy the nul character at the end of the
string too.
Fixes#1541.
* Run validator during reduction.
* Added functionality to validate modules after each reduction step, and some tests to check this is working. Also fixed an issue where reduction passes were not guaranteed to be executed at their minimum granularities.
There is inconsistencies between the different specs about whether or
not this capability is required/allowed, so tooling like glslang
currently ignores it. Once this is resolved the check and test can be
re-enabled.
* Invalidate the decoration manager at the start of ADCE.
If the decoration manager is kept live the the contex will try to keep
it up to date. ADCE deals with group decorations by changing the
operands in |OpGroupDecorate| instructions directly without informing
the decoration manager. This puts it in an invalid state, which will
cause an error when the context tries to update it. To Avoid this
problem, we will invalidate the decoration manager upfront.
At the same time, the decoration manager is now considered when checking
the consistency of the decoration manager.
Add a spirv-reduce pass which removes OpName and OpMemberName instructions.
This is useful to enable other reduction passes, e.g. RemoveUnreferencedInstruction may not be able to remove an instruction creating an id whose only usage is an OpName for this id.
* Fix invalid OpPhi generated by merge-return.
When we create a new phi node for a value say %10, we have to replace
all of the uses of %10 that are no longer dominated by the def of %10
by the result id of the new phi. However, if the use is in a phi node,
it is possible that the bb contains the use is not dominated by either.
In this case, needs to be handled differently.
* Split loop headers before add a new branch to them.
In merge return, Phi node in loop header that are also merges for loop
do not get updated correctly. Those cases do not fit in with our
current analysis. Doing this will simplify the code by reducing the
number of cases that have to be handled.
Added documentation to the ir context to indicates that TakeNextId()
returns 0 when the max id is reached. TODOs were added to each call
sight so that we know where we have to start to handle this case.
Handle id overflow in |SplitLoopHeader|.
Handle id overflow in |GetOrCreatePreHeaderBlock|.
Handle failure to create preheader in LICM.
Part of https://github.com/KhronosGroup/SPIRV-Tools/issues/1841.
* Only check for binding and descriptor set on variables that are
statically used by an entry point
* updated tests and added a couple new ones
* new method for collecting entry points that statically reference an
id
* Validate OpForwardPointer
The validator does not have a a check that OpForwardPointer is giving
a forward reference to a pointer type. We add that check.
https://crbug.com/910852
* Remove more specialized check.
There was a check that the forward pointer is actually a poiner type,
but it was only done if it was used in a struct. This was too specific.
Remove it in favour of the more general check that was added.
* Format
* Check the storage type in OpTypeForwardPointer
* Fix typo is test case epxected results.
We currently simulate all shift operations when the two operand are
constants. The problem is that if the shift amount is larger than
32, the result is undefined.
I'm changing the folder to return 0 if the shift value is too high.
That way, we will have defined behaviour.
https://crbug.com/910937.
Fixes#2147
* Checks that device scope is not used for availability and visibility
operations unless VulkanMemoryModelDeviceScopeKHR capability is present
* implemented for atomics, barriers and memory instructions currently
This CL changes the id/name output from the validator to always use a
consistent id[%name] style. This removes the need for getIdOrName. The
name lookup is changed to use the NameMapper so the output is consistent
with what the disassembler will produce.
Fixes#2137
* Validate uses of ids defined in unreachable blocks.
For some reason we do not make sure the uses of ids that are defined
in unreachable blocks are dominated by their def. This is causing
invalid code to pass the validator.
Fixes#2143
* Add test for unreachable code after a return.
We want to allow code like:
```
void foo() {
a = ...;
...
return; // for debugging
<use of a>;
...
}
```
I added a test to make sure that something like this is still accepted
by the validator.
* Add test for unreachable def used in phi.
Upgrade to VulkanKHR memory model
* Converts Logical GLSL450 memory model to Logical VulkanKHR
* Adds extension and capability
* Removes deprecated decorations and replaces them with appropriate
flags on downstream instructions
* Support for Workgroup upgrades
* Support for copy memory
* Adding support for image functions
* Adding barrier upgrades and tests
* Use QueueFamilyKHR scope instead of device
* Move ProcessFunction* function from pass to the context.
There are a few functions that are used to traverse the call tree.
They currently live in the Pass class, but they have nothing to do with
a pass, and may be needed outside of a pass. They would be better in
the ir context, or in a specific call tree class if we ever have a need
for it.
* Don't inline recursive functions.
Inlining does not check if a function is recursive or not. This has
been fine as long as the shader was a Vulkan shader, which forbid
recursive functions. However, not all shaders are vulkan, so either
we limit inlining to Vulkan shaders or we teach it to look for recursive
functions.
I prefer to keep the passes as general as is reasonable. The change
does not require much new code in inlining and gives a reason to refactor
some other code.
The changes are to add a member function to the Function class that
checks if that function is recursive or not.
Then this is used in inlining to not inlining a function call if it calls
a recursive function.
* Add id to function analysis
There are a few places that build a map from ids to Function whose
result is that id. I decided to add an analysis to the context for this
to reduce that code, and simplify some of the functions.
* Add missing file.
Fixes#2104
* Checks the rules for logical addressing and variable pointers
* Has an out for relaxed logical pointers
* Updated PassFixture to expose validator options
* enabled relaxed logical pointers for some tests
* New validator tests
Restrict capabilities to WebGPU spec
This covers whitelisting Matrix, Shader, Sampled1D, Image1D,
DerivativeControl, and ImageQuery. These are the allowed capabilities
that don't require an extension. Whitelisting VulkanMemoryModelKHR
will be handled by whitelisting its extension in a seperate patch.
Fixes#2101
* Added a reduction pass to replace ids with ids of the same type that dominate them.
* Introduce helper method for querying whether an operand type is an input id.
Fixes https://crbug.com/906669
* Don't free diagnostics in spvBinaryParse
* When invoking the parser we wish to ignore the error messages from,
instead create a hijacked context and replace the message consumer with
a null consumer
When we want to set a the value of a HexFloat to inf or nan, we
construct the specific bit pattern in an appropriately sized integer.
That integer is copied to a FloatProxy object through a memcpy. GCC8
complains about the memcpy because it is overwriting a private member of
the class.
The original solution worked well because the template to the HexFloat
could be anything. However, we only used some instantiation of FloatProxy,
which has a construction from that takes its uint_type, so I decided to use
that constructor instead of the memcpy. This puts an extra requirement
on the templace for HexFloat, but it will be fine for us.
Part of #1541.
Make sure that initialized variable have correct storage class
For WebGPU and Vulkan environments, variables must have the storage
class; Output, Private, or Function, if they have an initializer.
Fixes#2071
Adding validation that the addressing declared by OpMemoryModel is
Logical and the memory model declared is VulkanKHR. Updating a bunch
of tests that were broken by this.
Fixes#2060
Check forbidden Annotation instructions for WebGPU env
From the WebGPU SPIR-V Execution Enviroment spec:
OpDecorationGroup, OpGroupDecorate, OpGroupMemberDecorate are not
allowed.
Fixes#2062
Validate that debugging instructions are not present for WebGPU
For WebGPU execution environments, check that all of the debug
instructions have already been stripped before validation.
Fixes#2063
Ban sequentially consistent with VulkanKHR
* Added validation check that SequentiallyConsistent memory semantics
are not used if the memory model is VulkanKHR
* Added tests
* Fixed a bug in evaluating constant 32-bit integers and updated some
handling to avoid inferring a value from a spec constant default
Remaining memory semantics validation
* Adds checks that OutputMemoryKHR, MakeAvailableKHR and MakeVisibleKHR
are only used if the VulkanMemoryModelKHR capabailty is present
* Added checks that MakeAvailableKHR requires release semantics
* Added checks that MakeVisibleKHR requires acquire semantics
* Added checks that MakeAvailableKHR and MakeVisibleKHR require a
storage class
Adds validator option to specify scalar block layout rules.
Both VK_KHR_relax_block_layout and VK_EXT_scalar_block_layout can be
enabled at the same time. But scalar block layout is as permissive
as relax block layout.
Also, scalar block layout does not require padding at the end of a
struct.
Add test for scalar layout testing ArrayStride 12 on array of vec3s
Cleanup: The internal getSize method does not need a round-up argument,
so remove it.
These are bookend passes designed to help preserve line information
across passes which delete, move and clone instructions. The propagation
pass attaches a debug line instruction to every instruction based on
SPIR-V line propagation rules. It should be performed before optimization.
The redundant line elimination pass eliminates all line instructions
which match the previous line instruction. This pass should be performed
at the end of optimization to reduce physical SPIR-V file size.
Fixes#2027.
From the Vulkan 1.1 spec 14.5.2:
Variables identified with the Uniform storage class are used to access
transparent buffer backed resources. Such variables must be typed as
OpTypeStruct, or an array of this type.
Fixes#1949
Validate variable types for UniformConstant storage in Vulkan (#2008)
From the Vulkan 1.1 spec 14.5.2:
Variables identified with the UniformConstant storage class are used
only as handles to refer to opaque resources. Such variables must be
typed as OpTypeImage, OpTypeSampler, OpTypeSampledImage, or an array
of one of these types.
Fixes#2008
The type manager in spirv-opt currently asserts if a function parameter
has type void. It is not exactly clear from the spec that this is
disallowed, even if it probably will be disallowed. In either case,
asserts should be used to verify assumptions that will actually make a
difference to the code. As far as the optimizer is concerned, a void
parameter does not matter. I don't see the point of the assert. I'll
just remove it and let the validator decide whether to accept it or not.
No test was added because it is not clear that it is legal, and should
not force us to accept it in the future unless the spec make it clear
that it is legal.
Fixes crbug.com/903088.
That function currently only handled OpPtrAccessChain if it was in the
middle of the chain, but not at the start. Fixing that up.
Fixes crbug.com/905271.
The Vulkan specification does not permit use of the VertexId and
InstanceId BuiltIn decorations, so add a check to ensure they are not
being used when the target environment is Vulkan.
This CL removes several asserts around determining the SPIR-V
environment. In each case we already return a default value if
assertions are compiled out, so just return the default value.
* Add base and core bindless validation instrumentation classes
* Fix formatting.
* Few more formatting fixes
* Fix build failure
* More build fixes
* Need to call non-const functions in order.
Specifically, these are functions which call TakeNextId(). These need to
be called in a specific order to guarantee that tests which do exact
compares will work across all platforms. c++ pretty much does not
guarantee order of evaluation of operands, so any such functions need to
be called separately in individual statements to guarantee order.
* More ordering.
* And more ordering.
* And more formatting.
* Attempt to fix NDK build
* Another attempt to address NDK build problem.
* One more attempt at NDK build failure
* Add instrument.hpp to BUILD.gn
* Some name improvement in instrument.hpp
* Change all types in instrument.hpp to int.
* Improve documentation in instrument.hpp
* Format fixes
* Comment clean up in instrument.hpp
* imageInst -> image_inst
* Fix GetLabel() issue.
If there is only 1 return and it is in a loop, then the function cannot be inlined.
Fix condition when inlined code needs one-trip loop wrapper. The dummy loop is needed when there is a return inside a selection construct. Even if there is only 1 return.
* Validate the id bound.
Validates that the id bound for the module is not larger than the max id
bound. Also adds an option to set the max id bound. Allows the
optimizer option to set the max id bound to also set the id bound for
the validation run done by the optimizer.
Fixes#2030.
When building C code with gcc and the
-Wstrict-prototypes option, function declarations
and definitions that don't specify their argument
types generate warnings. Functions that don't
take parameters need to specify (void) as their
parameter list, rather than leaving it empty.
Note this only applies to C, so only the functions
exported in C-compatible headers need fixing. In
C++ functions can't be declared/defined without a
parameter list, so C++ can safely allow an empty
parameter list to imply (void).
When looking for a break from a selection construct, we do not realize
that a jump to the continue target of a loop containing the selection
is a break. This causes and infinit loop, or possibly other failures.
Fixes#2004.
When looking for a break from a selection construct, we do not need to
look inside nested constructs. However, if a loop header has an
unconditional branch, then we enter the loop. Entering the loop causes
an infinite loop because we keep going through the loop.
The solution is to look for a merge block, if one exsits, even for block
terminated by an OpBranch.
Fixes#1979.
The SPV_KHR_8bit_storage extension does not permit 8-bit integers to be
cast directly to floating point types. We are seeing shaders in the
wild, being produced by toolchains like glslang, that are generating
invalid SPIR-V.
This change adds validation to check for the patterns not permitted, and
some tests that expose the failure.
In CMake, we are not suppose to have multiple targets depend on the same
custom command. To avoid this, we have to add a custom target around
the command.
Fixes#1941.
ADCE liveness algorithm should treat OpUnreachable at least like other
branch instructions. It was being treated as always live which was
preventing useless structured constructs from being eliminated.
OpUnreachable is generated by dead branch elimination which is now
being required by merge return, so this fix should accompany that
change.
We currently run merge-return on all functions, but
dead-branch-elimination only runs on function reachable from an entry
point or exported function. Since dead-branch-elimination is needed for
merge-return, they have to match.
Fixes#1976.
In logical addressing mode, we are not allowed to generate variables
pointers. There is already a check for OpSelect. However, OpPhi
and OpPtrAccessChain are not checked to make sure it does not
generate an variable pointer. I've added those checks.
Fixes#1957.
Was removing control structures which didn't have data dependency
with enclosed live loop and otherwise did not contain live code.
An example is a counting loop around a live loop.
Fixes#1967.
* MakePointerVisibleKHR cannot be used with OpStore
* MakePointerAvailableKHR cannot be used with OpLoad
* MakePointerAvailableKHR and MakePointerVisibleKHR both require
NonPrivatePointerKHR
* NonPrivatePointerKHR is limited to a subset of storage classes
* many tests
Consider atomics that load when analyzing live stores in ADCE.
Previously it asserted that the base of an OpImageTexelPointer should
be an image. It is actually a pointer to an image, so IsValidBasePointer
should suffice.
* Validation checks for new image operands MakeTexelAvailableKHR and
MakeTexelVisibleKHR
* added tests
* Tests that NonPrivateTexelKHR is accepted for all image operands
Updating test environments
* fixed build errors
* changed image types for *FetchSuccess tests to use a type defined in
1.3 shader body
Merge return assumes that the only unreachable blocks are those needed
to keep the structured cfg valid. Even those must be essentially empty
blocks.
If this is not the case, we get unpredictable behaviour. This commit
add a check in merge return, and emits an error if it is not the case.
Added a pass of dead branch elimination before merge return in both the
performance and size passes. It is a precondition of merge return.
Fixes#1962.
The current implementation in the folder when seeing a division by zero
is to assert. In the release build, the compiler will attempt to
compute the value, which causes its own problems.
The solution I will go with is to fold the division, and just give it
the value of 0. The same goes for remainder and mod operations.
Fixes#1961.
This commit checks the following when Shader capability exists:
"The FPRoundingMode decoration can be applied only to a width-only
conversion instruction that is used as the Object operand of an
OpStore storing through a pointer to a 16-bit floating-point object
in the StorageBuffer, Uniform, PushConstant, Input, or Output
Storage Classes.".
The HlslCounterBufferGOOGLE that was introduced changed the OpDecorateId
so that is can now reference an id other than the target. If that other
id is used only in the decoration, then the definition of the id will be
removed because decoration do not count as real uses.
However, if the target of the decoration is still live the decoration
will not be removed. This leaves a reference to an id that is not
defined.
There are two solutions to consider. The first is that is the decoration
is kept, then the definition of the id should be kept live. Implementing
this change would be involved because the way ADCE handles decorations
will have to be reimplemented.
The other solution is to remove the decoration the id is otherwise dead.
This works for this specific case. Also this is the more desirable
behaviour in this case. The id will always be the id of a variable that
belongs to a descriptor set. If that variable is not bound and we do
not remove it, the driver will complain.
I chose to implement the second solution. The first will be left to when
a case for it comes up.
Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/1885.
This commit will change the message for unknown extensions from an error
to a warning.
Code was added to limit the number of warning messages so that consummer
of the messages are not overwhelmed. This is standard practice in
compilers.
Many other issues were found at while looking into this. They have been
documented in #1950.
Fixes http://crbug.com/875547.
* Check rules from Execution Mode tables, 2.16.2 and the Vulkan
environment spec
* Allows MeshNV execution model with the following execution modes
* LocalSize, LocalSizeId, OutputPoints and OutputVertices
* Done to not break their validation
There are a few spots where copy propagate arrays is trying
to go from a Type to an id, but the type is not unique. When generating
code this pass needs specific ids, otherwise we get type mismatches.
However, the ambigous types means we can sometimes get the wrong type
and generate invalid code.
That code has been rewritten to not rely on the type manager, and just
look at the instructions instead.
I have opened https://github.com/KhronosGroup/SPIRV-Tools/issues/1939 to
try to get a way to make this more robust.
* Analyze uses for all instructions.
The def-use manager needs to fill in the `inst_to_used_ids_` field for
every instruction. This means we have to analyze the uses for every
instruction, even if they do not have any uses.
This mistake was not found earlier because there was a typo in the
equality check for def-use managers. No new tests are needed.
While looking into this I found redundant work in block merge. Cleaning
that up at the same time.
* Fix other transformations
Aggressive dead code elimination did not update the OpGroupDecorate
and the OpGroupMemberDecorate instructions properly when they are
updated. That is fixed.
Dead branch elimination did not analyze the OpUnreachable instructions
that is would add. That is taken care of.
In DecorationManager::RemoveDecorationsFrom, we do not remove the id
from a decoration group if the group has no decorations. This causes
problems because KillNamesAndDecorates is suppose to remove all
references to the id, but in this case, there is still a reference.
This is fixed by adding a special case.
Also, there is the possibility of a double free because
RemoveDecorationsFrom will delete the instructions defining |id| when
|id| is a decoration group. Later, KillInst would later write to memory
that has been deleted when trying to turn it into a Nop. To fix this,
we will only remove the decorations that use |id| and not its definition
in RemoveDecorationsFrom.
OpPhi instruction must appear before all non-OpPhi instructions
except for OpLine. Without this commit, Validator does not check
the case that an OpPhi is preceeded by an OpLine and the OpLine is
preceeded by a non-OpPhi instruction that is not OpLine.
Checked all instructions whose object is OpTypeSampledImage or
OpTypeImage as suggested in #487. OpImageTexelPointer instruction
is missing and others look good. This commit adds only
OpImageTexelPointer.
Add code to keep the def-use manger and the inst-to-block mapping up-to-date. This means we do not have to rebuild them later.
To make this work, we will have to have to find places to update the
def-use manager. Updating the def-use manager is not straight forward
because we are unrolling loops, and we have circular references.
This forces one pass to register all of the definitions. A second one
to analyze the uses. Also because there will be references to the new
instructions in the old code, we want to register the definitions of the
new instructions early, so we can update the uses of the older code as
we go along.
The inst-to-block mapping is not too difficult. It can be done as instructions are created.
Fixes#1928.
A limit of 0 for the scalar replacement options it used to indicate that
there is no limit. The current implementation does not allow 0. This
should be fixed.
It seems like the current implementation of KillNameAndDecorates does
not handle group decorations correctly. The id being removed is not
removed from the OpGroupDecorate instructions. Even worst, any
decorations that apply to that group are removed.
The solution is to use the function in the decoration manager that will
remove the decorations and update the instructions instead of doing the
work itself.
Adds unrolling to the legalization passes.
After enabling unrolling I found a bug when there is a self-referencing
phi node. That has been fixed.
The test that checks for that the order of optimizations is correct also
needed to be updated.
We currently register decorations in the first pass through the
instructions. This is a problem because the validator has not even
checked if the decoration instructions are valid yet. This can lead to
unexpected behaviour from these side table. For example, in
https://github.com/KhronosGroup/SPIRV-Tools/issues/1882, we use 5GB of
data to store 1 decoration for ids that are not even defined.
Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/1882.
The current implementation of merge return can create bad, but correct,
code. When it is not in a loop construct, it will insert a lot of
extra branch around code. The potentially large number of branches are
bad. At the same time, it can separate code store to variables from
its uses hiding the fact that the store dominates the load.
This hurts the later analysis because the compiler thinks that multiple
values can reach a load, when there is really only 1. This poorer
analysis leads to missed optimizations.
The solution is to create a dummy loop around the entire body of the
function, then we can break from that loop with a single branch. Also
only new merge nodes would be those at the end of loops meaning that
most analysies will not be hurt.
Remove dead code for cases that are no longer possible.
It seems like some drivers expect there the be an OpSelectionMerge
before conditional branches, even if they are not strictly needed.
So we add them.
* Create structed cfg analysis.
There are lots of optimization that have to traverse the CFG in a
structured order just because it wants to know which constructs a
basic block in contained in. This adds extra complexity to these
optimizations, for causes too much refactoring of older optimizations.
To help with this problem, I have written an analysis that can give this
information.
* Identify branches breaking from loops.
Dead branch elimination does a search for a conditional branch to the
end of the current selection construct. This search assumes that the
only way to leave the construct is through the merge node. But that is
not true. The code can jump to the merge node of a loop that contains
the construct.
The search needs to take this into consideration.
When using lldb and/or gdb I frequently get odd std::string failures
when using the IR printing instructions we have now. This adds the
methods Instruction::Dump(), BasicBlock::Dump() and Function::Dump() to
emit the output of the pretty print to stderr.
With this I can now reliably print IR from gdb and lldb sessions.
In merge blocks, we do not allow the merging of two blocks with merge
instructions. This is because if the two block are merged only 1 of
those instructions can exists. However, if the successor block is the
merge block of the predecessor, then we can delete the merge instruction
in the predecessor. In this case, we are able to merge the blocks.
* Create a new entry point for the optimizer
Creates a new struct to hold the options for the optimizer, and creates
an entry point that take the optimizer options as a parameter.
The old entry point that takes validator options are now deprecated.
The validator options will be one of the optimizer options.
Part of the optimizer options will also be the upper bound on the id bound.
* Add a command line option to set the max value for the id bound. The default is 0x3FFFFF.
* Modify `TakeNextIdBound` to return 0 when the limit is reached.
Support collapsed into one commit:
- Asm/Dis support for SPV_KHR_vulkan_memory_model
- Add Vulkan mem model image operands to switch
- Add TODO for source/validate_image.cpp
- val: Image operands NonPrivateTexelKHR, VolatileTexelKHR have no operands
This is required for memory model tests to pass SPIR-V validation.
- Round trip tests: Test new flags on OpCopyMemory*
* Validate all type ids.
The validator does not check if the type of an instruction is actually
a type unless the OpCode has a specific requirement. For example,
OpFAdd is checked, but OpUndef is not.
The commit add a generic check that if there is a type id then the id
defines a type.
http://crbug.com/876694
* Merge other checks for type into new one.
There are a couple check that the type id is a type for specific
opcodes. Those have been mereged into 1.
Small changes to other test cases to make them valid enough for the
purpose of the test.
In the specification of `OpTypeFunction`, it says
> OpFunction is the only valid use of OpTypeFunction.
This commit add a check in the validator for this rule.
A test started to fail because the new check happens before the check
the test case is testing. Updated the test case to still fail the
check it was suppose to fail originally.
http://crbug.com/874571
* Have the constant manager take ownership of constants.
Right now the owner of an object of type contant that is in the
|const_pool_| of the constant manager is unclear. The constant
manager does not delete them, there is no other reasonable owner. This
causes memory leaks.
This change fixes the memory leaks by having the constant manager
take ownership of the constant that is stores in |const_pool_|. Other
changes include interface changes to make it explicit that the constant
manager takes ownership of the object when a constant is registered
with the constant manager.
Fixes#1865.
Right now the owner of an object of type contant that is in the
|const_pool_| of the constant manager is unclear. The constant
manager does not delete them, there is no other reasonable owner. This
causes memory leaks.
This change fixes the memory leaks by having the constant manager
take ownership of the constant that is stores in |const_pool_|. Other
changes include interface changes to make it explicit that the constant
manager takes ownership of the object when a constant is registered
with the constant manager.
* Copy decorations when creating new ids.
When creating a new value based on an old value, we need to copy the
decorations to the new id. This change does this in 3 places:
1) The variable holding the return value of the function generated by
merge return should get decorations from the function.
2) The results of the OpPhi instructions should get decorations from the
variable they are replacing in the ssa writer.
3) In local access chain convert the intermediate struct (result of
OpCompositeInsert) generated for the store replacement should get its
decorations from the variable being stored to.
Fixes#1787.
If seems like at least 1 driver does not like a condition jump to the end
of a selection construct. We are generating these in the merge return
pass. This change stops merge return from generating this sequence.
Part of #1861.
When doing predicate blocks, we need to traverse every block in
structured order in order to keep track of which construct a block is
contained in. The standard way of traversing code in structured order
is to create a list with all of the nodes in order. However, when
predicating blocks, new blocks are created, and those blocks are missed.
This causes branches that go too far.
The solution is to update the order as new blocks are created. Since
we are using an std::list, we do not have to worry about invalidation of
iterators when changing the list.
* Split constant opcode validation out of idUsage and into
validate_constants.cpp
* minor style fixes
* reduced duplication
* fixed an issue with array sizing
* Refactor PredicateBlocks
Refactor PredicateBlocks so that we know which constructs a return
is contained in. Will be used later.
* Have PredicateBlocks jump the existing merge blocks.
In PredicateBlocks, we currently skip instructions with side effects,
but it still follows the same control flow (sort-of). This causes a
problem, when we are trying to predicate code in a loop. We skip all
of the code with side effects (IV increment), but still follow the
same control flow (jump back the start of the loop). This creates an
infinite loop because the code will keep jumping back to the start of
the loop without changing the values that effect the exit condition.
This is a large change to merge-return. When predicating a block that
is in a loop or merge construct, it will jump to the merge block of the
construct. Once out of all constructs we will generate code as we did
before.
* Handle breaks from structured-ifs in DCE.
dead code elimination assumes that are conditional branches except for
breaks and continues in loops will have an OpSelectionMerge before them.
That is not true when breaking out of a selection construct.
The fix is to look for breaks in selection constructs in the same place
we look for breaks and continues for loops.
When dead-branch-elim folds a conditional branch, it also deletes the
OpSelectionMerge instruction. If that construct contains a
conditional branch to the merge node, it will not have its own
OpSelectionMerge. When the headers merge instruction is deleted, the
the inner conditional branch will no longer be legal. It will be a
selection to a node that is not a merge node.
We fix this up by moving the OpSelectionMerge to a new location if it is
still needed.
This forks the testing harness from https://github.com/google/shaderc
to allow testing CLI tools.
New features needed for SPIRV-Tools include:
1- A new PlaceHolder subclass for spirv shaders. This place holder
calls spirv-as to convert assembly input into SPIRV bytecode. This is
required for most tools in SPIRV-Tools.
2- A minimal testing file for testing basic functionality of spirv-opt.
Add tests for all flags in spirv-opt.
1. Adds tests to check that known flags match the names that each pass
advertises.
2. Adds tests to check that -O, -Os and --legalize-hlsl schedule the
expected passes.
3. Adds more functionality to Expect classes to support regular
expression matching on stderr.
4. Add checks for integer arguments to optimization flags.
5. Fixes#1817 by modifying the parsing of integer arguments in
flags that take them.
6. Fixes -Oconfig file parsing (#1778). It reads every line of the file
into a string and then parses that string by tokenizing every group of
characters between whitespaces (using the standard cin reading
operator). This mimics shell command-line parsing, but it does not
support quoting (and I'm not planning to).
When doing the validator checks, an instruction is currently registered
at the end of IdPass. This creates an inconsistency. In IdPass, an
instruction that uses its own result will treat that use as a forward
reference. Then in the following passes it will not because the
definition can be found.
It seems best to update the state after all of the check have been done
for the current instruction. This makes it consistent for all of the
passes.
This makes a different when trying to verify OpTypeStruct.
Fixes https://crbug.com/874372.
In local-access-chain-convert, we replace loads by load the entire
variable, then doing the extract. The extract will have the same value
as the load. However, if the load has a decoration on it, the
decoration is lost because we do not copy any them to the new id.
This is fixed by rewritting the load into the extract and keeping the
same result id.
This change has the effect that we do not call DCEInst on the loads
because the load is not being deleted, but replaced. This could leave
OpAccessChain instructions around that are not used. This is not a
problem for -O and -Os. They run local_single_*_elim passes and then
dead code elimination. The dce will remove the unused access chains,
and the load elimination passes work even if there are unused access
chains. I have added test to them to ensure they will not loss
opportunities.
Fixes#1787.
The code in source/message was only used in a single set of tests to
format the output results. This CL changes the test to verify the
message instead of all the error values and removes the source/message
code.
In `TypeManager::RebuildType`, the base cases call `Clone`, which will
copy the decorations for the type. After that it breaks out of the
switch statement and copies the decorations again.
This has not causes any real problems yet because none of those types
are allowed to have decorations. However to make the code more robust
it is best to not copy twice because it should be empty.
This way if a new base type or decoration is added that changes this
rule the code will be correct.
* Moved function opcode validation out of idUsage and into new files
* minor style changes
* General opcode checking is in validate_function.cpp
* Execution limitation checking is in
validate_execution_limitations.cpp
* Execution limitations was split into a new pass as it requires other
validation to register those limitations first.
* Changed entry point validation to check storage class of variable
instead of pointer
* added a test
* Moved several checks after opcode validation
* These checks should be able to guarantee individual instructions are
ok
* Updated tests due to reordered checks
