When processing global values to determine when to flatten vectors, this
pass was only checking the immdiate users of the value for non-dynamic
indexing of the vector. But this would fail in the case of a dynamic
indexed GEP of a constant indexed GEP (e.g. h[0][a]) because the first
level GEP was constant indexed, but not the second. We fix this by
checking the full User tree of the value in `hasDynamicVectorIndexing`.
Before doing any major surgery on an exit from loop L, ensure that if an
exit edge from L goes to block X, then X is in L's parent loop or no
loop at all.
Add test cases:
- a reduced test case where the exiting block does not dominate its own
loop latch.
- a reduced test case where the exiting block is the latch for its own
loop. This reproduces the assert triggered by the original HLSL.
- the original HLSL that triggered this bug fix.
- the intermediate module from the original HLSL, taken just before the
attempt to remove unstructured loop exits.
This PR pulls the upstream change, Use accessors for ValueHandleBase::V;
NFC
(6f08789d30),
into DXC.
Here's the summary of the change:
> This changes code that touches ValueHandleBase::V to go through
getValPtr and (newly added) setValPtr. This functionality will be used
later, but also seemed like a generally good cleanup.
>
> I also renamed the field to Val, but that's just to make it obvious
that I fixed all the uses.
This is part 1 of the fix for #6659.
---------
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This is to fix compile error like
```
../../third_party/dxc/utils/TableGen/AsmWriterEmitter.cpp(974): error C2666: '`anonymous-namespace'::IAPrinter::operator ==': overloaded functions have similar conversions
../../third_party/dxc/utils/TableGen/AsmWriterEmitter.cpp(739): note: could be 'bool `anonymous-namespace'::IAPrinter::operator ==(const `anonymous-namespace'::IAPrinter &)'
../../third_party/dxc/utils/TableGen/AsmWriterEmitter.cpp(739): note: or 'bool `anonymous-namespace'::IAPrinter::operator ==(const `anonymous-namespace'::IAPrinter &)' [synthesized expression 'y == x']
../../third_party/dxc/utils/TableGen/AsmWriterEmitter.cpp(974): note: while trying to match the argument list '(`anonymous-namespace'::IAPrinter, `anonymous-namespace'::IAPrinter)'
```
in dawn project which is updating to newer VS toolchain.
This imports fix from
4ab57cd9ab.
more context is in https://issues.chromium.org/issues/341890053#comment2
Add a pass to run hlsl::RemoveUnstructuredLoopExits in isolation
Example: opt -dxil-r-u-l-e a.ll -S
Add some basic tests.
No functional change to the pass itself.
Sampler feedback resource types are not supported by the SPIR-V backend,
but they would previously fail silently until a function was called on
them. This change makes the error message more explicit on the type.
Related to #6614
The SkipBlockWithBranch function does the following:
- Splits the block into three blocks with an if-then-endif structure.
- Moves most instructions from the original block into the "then" block
- If any of those values are used outside the original block, they are
propagated through newly-constructed phis in the 'endif' block.
This algorithm had a bug where the uses of a value were being scanned
while the uses were also being updated. In some cases a downstream
out-of-block use could be skipped. That results in an invalid module
because now the original definition is now in the 'then' block, which
does not dominate the downstream out-of-block use.
Add a test that demonstrates the problem.
LoopDeletion was incorrectly updating PHI nodes in the target block when
it had duplicate input edges. This happens, for example, when deleting a
loop that uses a switch with multiple cases that exit the same way.
After determining that this was the bug, I found this fix in LLVM:
https://reviews.llvm.org/D34516 and applied it here.
When flattening the global for a groupshared matrix, the alignment
information was getting lost. As a result, the alignments of the loads
and stores were calculating their own alignment based on preferred
alignment and trailing zeros of the index. The preferred alignment
switched to 16 when the type size was over 128 bits due to a heuristic
whose rationale is lost to time. When the global has its own alignment,
that gets used, so by calculating it at lowering, the alignments are
consistent and reliable.
Includes testing for a few matrix variants and a pass test.
fixes#6416
We have a special case to that the the size and alignment for an empty
struct is `{1,0}`. However that is not correct. See
https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#interfaces-alignment-requirements.
> An empty structure has a base alignment equal to the size of the
smallest
scalar type permitted by the capabilities declared in the SPIR-V module.
(e.g.,
for a 1 byte aligned empty struct in the StorageBuffer storage class,
StorageBuffer8BitAccess or UniformAndStorageBuffer8BitAccess must be
declared
in the SPIR-V module.
I'm not 100% sure how DXC handle this minimum alignment, but I figured I
would inialize the alignment to 1. If there are not members, then it
will remain 1, and I would let the rest of the logic happen. No special
case.
Fixes#2882
SampleCmpLevel is similar to SampleCmpLevel0, except the LOD level can
be specified using either a const-offset, or a variable. This should be
available starting SM6.7
Fixes#6613
---------
Signed-off-by: Nathan Gauër <brioche@google.com>
Although DXC applied the upstream change, Reassociate: add global
reassociation algorithm
(https://github.com/llvm/llvm-project/commit/b8a330c) in this PR
(https://github.com/microsoft/DirectXShaderCompiler/pull/6598), it still
might overlook some obvious common factors.
One case has been observed is:
```
%Float4_0 = call %dx.types.CBufRet.f32 @dx.op.cbufferLoadLegacy.f32(i32 59, %dx.types.Handle %1, i32 1)
%Float4_0.w = extractvalue %dx.types.CBufRet.f32 %Float4_0, 3
%Float2_0 = call %dx.types.CBufRet.f32 @dx.op.cbufferLoadLegacy.f32(i32 59, %dx.types.Handle %1, i32 0)
%Float2_0.y = extractvalue %dx.types.CBufRet.f32 %Float2_0, 1
/* %Float4_1 is redundant with %Float4_0 since they invokes cbufferLoadLegacy with same parameters */
%Float4_1 = call %dx.types.CBufRet.f32 @dx.op.cbufferLoadLegacy.f32(i32 59, %dx.types.Handle %1, i32 1)
/* %Float4_1.w is redundant with %Float4_0.w */
%Float4_1.w = extractvalue %dx.types.CBufRet.f32 %Float4_1, 3
/* %Float2_1 is redundant with %Float2_0 since they invokes cbufferLoadLegacy with same parameters */
%Float2_1 = call %dx.types.CBufRet.f32 @dx.op.cbufferLoadLegacy.f32(i32 59, %dx.types.Handle %1, i32 0)
/* %Float2_1.y is redundant with %Float2_0.y */
%Float2_1.y = extractvalue %dx.types.CBufRet.f32 %Float2_1, 1
....
%11 = fmul fast float %Float4_0.w, %10
%12 = fmul fast float %11, %Float2_0.y
....
%14 = fmul fast float %Float4_1.w, %13
%15 = fmul fast float %14, %Float2_1.y
(%Float4_0.w * %Float2_0.y) equals to (%Float4_1.w * %Float2_1.y), they should be reassociated to a common factor
```
The upstream change can't identify this common factor because DXC
doesn't know (%Float4_0.w, %Float4_1.w) and (%Float2_0.y, %Float2_1.y)
are redundant when running Reassociate pass. Those redundancies will be
eliminated in GVN pass.
For DXC can identify more common factors, this PR will aggressively run
Reassociate pass again after GVN pass and then run GVN pass again to
remove the redundancies generared in this run of Reassociate pass.
Changing the order of floating point operations causes the precision
issue. In case some shaders get unexpected results due to this PR, use
"-opt-disable aggressive-reassociation" to disable this PR and roll
back.
This is part 3 of the fix for
https://github.com/microsoft/DirectXShaderCompiler/issues/6593.
This PR (https://github.com/microsoft/DirectXShaderCompiler/pull/6598)
pulls the upstream global reassociation algorithm change in DXC and can
reduce redundant calculations obviously.
However, from the testing result of a large offline suite of shaders,
some shaders got worse compilation results and couldn't benefit from
this upstream change.
This PR adds a flag for the upstream global reassociation change. It
would be easier to roll back if a shader get worse compilation result
due to this upstream change.
This is part 2 of the fix for #6593.
This PR pulls the upstream change, Reassociate: add global reassociation
algorithm
(b8a330c42a),
into DXC with miminal changes.
For the code below:
foo = (a * b) * c
bar = (a * d) * c
As the upstream change states, it can identify the a*c is a common
factor and redundant.
This is part 1 of the fix for #6593.
This test was marked DISABLED_ in gtest at the time it was added in PR
#3155, so it appears that it was never passing. Specifically, the CHECK
for `DebugFunction [[func1]]` fails. I don't think it's a priority to
implement debug info for unreferenced functions at this point, so opting
to simply remove it.
This is the last test in the unsupported directory so it can now be
removed entirely.
Fixes#6616
The following tests only required minor test syntax changes to pass:
- tools/clang/test/CodeGenSPIRV/cast.2float.interlocked.hlsl
-
tools/clang/test/CodeGenSPIRV/meshshading.nv.error.fncall.amplification.vulkan1.2.hlsl
(+ replacing NV ext with EXT)
- tools/clang/test/CodeGenSPIRV/var.init.extvector.hlsl
Issue #6621 has been filed to track the failure of
tools/clang/test/CodeGenSPIRV/oo.class.static.member.hlsl.
Related to #6616
These tests were previously marked as "unsupported" because they were
misconfigured at the time they were added and never run. Minor changes
have been made to make them passing tests.
Note that rayquery_assign.cs.hlsl has been removed because it no longer
produces an error. A similar non-erroring check exists in
rayquery_init_expr.hlsl
Related to #6616
This test is marked as unsupported (ignored), but it currently fails to
compile both for SPIR-V and DXIL with the same Sema error: "cannot
implicitly convert from 'SecondStruct' to 'FirstStruct'". If it would
have succeeded for SPIR-V at some point in the past it's no longer valid
anyways, so removing it.
Related to #6616
When SimplifySwitchOnSelect calls SimplifyTerminatorOnSelect, it holds
onto the select's condition value to use for the conditional branch it
replaces the switch with. When removing the switch's unused
predecessors, it must make sure not to delete PHIs in case one of them
is used by the condition value, otherwise the condition value itself may
get deleted, resulting in an use-after-free.
Note that this was fixed in LLVM as well:
dc3b67b4ca
Given a switch with a constant condition and all cases the same
(branching to the same successor), dxil-remove-dead-blocks would
incorrectly remove the switch when replacing it with a branch, by
forgetting to remove the N-1 incoming values to the PHIs in the
successor block.
The implementation of index_is_valid was incorrect returning false when
the input index was equal to Term->getNumSuccessors(). The end iterator
will have such an index, and it is valid to construct such an iterator.
For example, assume we have a block with two successors:
```
BasicBlock* bb = ...;
SuccIterator b(bb); // Index 0
SuccIterator e(bb, true); // Index 2, for example
SuccIterator v = b;
b += 2; // Without this fix, this asserts
assert(b == e);
```
Note that this was also fixed upstream in
https://reviews.llvm.org/D47467
---------
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Natalie Chouinard <chouinard.nm@gmail.com>
This change copes with the AS->MS payload being placed in group-shared
by the application (and MSFT's samples do indeed do this). (TIL, thanks
to pow2clk, that the spec says that the payload counts against the
group-shared total, implying, if not explicitly stating, that at least
on some platforms, the payload will be in group-shared anyway.)
The MS pass needs to be given data from the AS about the AS's thread
group topology, and this is done by extending the payload struct to add
three uints. This can't be done when the payload is resident in
group-shared, of course, because that would change the layout of
group-shared memory.
So the new approach here is to copy the payload to a new alloca (in the
default address space) struct with the members of the base struct plus
the extended data the MS needs, and then to copy piece-wise because
llvm.memcpy isn't appropriate for group-shared-to-normal address space
copies.
A follow-up change will use the PartitionedExclusiveScanNV
GroupOperation, which requires that an additional operand is added to
all GroupNonUniformArithmetic instructions. This means that some of the
SPIR-V opcodes which are currently categorized as unary will become
either unary or binary depending on the GroupOp. Since the arity
distinctions between the OpGroupNonUniform* instructions were already
somewhat arbitrary, I'm prefacing that change by refactoring them into a
single SpirvGroupNonUniformOp instruction type for better reusability.
Follow up: #6608
Two intertwined issues:
1:
The shader-debug-instrumentation and pixel-hit-counter passes add
SV_VertexId+SV_InstanceId to the input sig for a VS if they are not
present, and SV_Position for the PS. The changes in PixPassHelpers.cpp
centralize the SV_Position case and fix up a few missing fields whose
absence was breaking PIX shader debugging on WARP. The calller now has
to pass the upstream shader's SV_Position row, if it exists. (PIX has
been updated to do this.)
2:
However, independent of these tweaks, adding these new values means that
the view id metadata would be incorrect. A long-standing assert would
fire, but is herein fixed, since the above work exacerbated this
problem. The assert in question is this one in
CopyViewIDStateForOutputToPSV that previously fired when a
debug-instrumented shader was subsequently wrapped into a container:
``` DXASSERT_NOMSG((InputScalars <= IOTable.InputVectors * 4) &&
(IOTable.InputVectors * 4 - InputScalars < 4));
```
(InputScalars, which comes from the serialized state in PSV0, was too
small, since it did not include the added system values.)
To fix up these data, the caller now has to invoke the
generate-view-id-state pass, and follow that with the emit-metadata
pass, since emitting the metadata in the debug pass itself is now too
early (and that call has been deleted). (PIX has been updated to do
invoke these passes.)
These changes together are sufficient to allow PIX shader debugging to
operate on WARP in those cases where these system-values were not
included by the application, with correct view-id metadata, and the
assert is happy.
RWByteAddressBuffer has overloads for InterlockedMin and InterlockedMax
for signed ints that were failing to compile due to mismatched types in
the generated SPIR-V instruction. This adds the missing cast if
necessary.
At the same time, some redundant code is removed from the
InterlockedMin/Max intrinsic non-member functions' codegen to modify the
opcode. If it was necessary in the past, the frontend has since been
fixed and it is no longer necessary. Tests to verify these combinations
and the necessary implicit casts have also been added.
Fixes#3196
Related to #4189, #6254, #5707
If a parameter annotated with `[[vk::ext_literal]]` is not lowered to a
SpirvConstant, make a best-effort attempt to evaluate it to a constant
literal value. If unsuccessful, fail gracefully rather than crash.
Fixes#6586
As tested in the accompanying test, the DebugLoc for a class method
contains a DICompositeType as part of its scope hierarchy. Lack of a
test for this was breaking PIX shader debugging for class methods.
PIX requires that all vertex information generated by these passes be
uniquely identified by vertex id and MS thread id.
This change fixes the MS thread id part in two places: the amplification
shader and the mesh shader.
To be unique across an entire DispatchMesh call, we must uniquify the AS
thread group, the AS thread, the MS thread group and the MS thread. This
is a lot of multiplying and adding, and there wasn't quite enough math
going on here before.
In the AS case, we now generate a unique "flat" thread id from the
flat-thread-id-in-group (the already-available system value) and the
"flat group id", which we synthesize by multiplying together the group
id components with the DispatchMesh API's thread group counts, and then
multiplying that by the number of threads each AS group launches, then
add the flat-thread-id-in-group. (This flat id then goes into an
expanded version of the AS->MS payload, the code for which was
pre-existing.)
The MS will either treat the incoming AS thread id as its unique
thread-group-within-the-whole-dispatch id.
If the AS is not active, the instrumentation herein will synthesize a
flat id in the same way as the AS did before it passed that id through
the payload, again from the DispatchMesh parameters (newly-added params
to that pass) and the flat-thread-in-group.
In addition to the new filecheck tests for this, there is also a new
filecheck test to cover coercion of non-i32 types to i32 before being
written to PIX's output UAV, which I happened to notice wasn't
adequately tested.
- This change adds a switch case for TK_AccelerationStructureNV in
lowerToDebugType
- Before, compiling a shader for vulkan containing an acceleration
structure and using -fspv-debug=vulkan-with-source would cause a crash.
Fixes#5113
Also, emit a better message when the desired pass does not exist (or is
mis-spelled). Before this change, there would only be a confusing
message about 'after' being None.
When indexing a swizzled bool vector, some HLSL-specific code in
EmitCXXMemberOrOperatorMemberCallExpr kicks in to handle the
HLSLVecType. In this case, we’re dealing with an ExtVectorElt because of
the swizzle, so this function creates a GEP, Load, and Store on the
vector. However, boolean scalars are returned as type i11 while the
store is storing to a bool, which is an i32, so we need to insert a cast
before the store.
Now that WARP is actually getting bugfixes and features, and has a way
for folks to get new versions without waiting years for an OS update,
stop hiding the places where it's failing. Having these hardcoded skips
in place makes it impossible to run the tests to see if it's still
failing. If we really need these skips, we at least need some kind of
command line option to override them for debugging and fixing the
failures.
This commit adds a new test that validates the help text output contains
the -Qsource_in_debug_module option recently enabled by a previous PR
#6597 .
This is a follow up commit for Issue: #6028
Co-authored-by: Cooper Partin <coopp@ntdev.microsoft.com>
This commit adds the compiler option (-Qsource_in_debug_module) to the
help text.
New help text output below:
```
Utility Options:
-dumpbin Load a binary file rather than compiling
-extractrootsignature Extract root signature from shader bytecode (must be used with /Fo <file>)
-getprivate <file> Save private data from shader blob
-link Link list of libraries provided in <inputs> argument separated by ';'
-P Preprocess to file
-Qembed_debug Embed PDB in shader container (must be used with /Zi)
-Qsource_in_debug_module
Embed source info in PDB
-Qstrip_debug Strip debug information from 4_0+ shader bytecode (must be used with /Fo <file>)
-Qstrip_priv Strip private data from shader bytecode (must be used with /Fo <file>)
-Qstrip_reflect Strip reflection data from shader bytecode (must be used with /Fo <file>)
-Qstrip_rootsignature Strip root signature data from shader bytecode (must be used with /Fo <file>)
-setprivate <file> Private data to add to compiled shader blob
-setrootsignature <file>
Attach root signature to shader bytecode
-verifyrootsignature <file>
Verify shader bytecode with root signature
```
fixes#6028
---------
Co-authored-by: Cooper Partin <coopp@ntdev.microsoft.com>
This adds new literal warnings to identify integer literals that may
have breaking behavior changes between HLSL 2021 and 202x as a result of
the conforming literals change.
The spec update for this is in:
https://github.com/microsoft/hlsl-specs/pull/229Resolves#6581
ReplaceConstantWithInst(C, V) replaces uses of C in the current function
with V. If such a use C is an instruction I, the it replaces uses of C
in I with V. However, this function did not make sure to only perform
this replacement if V dominates I. As a result, it may end up replacing
uses of C in instructions before the definition of V.
The fix is to lazily compute the dominator tree in
ReplaceConstantWithInst so that we can guard the replacement with that
dominance check.
Adds support for the `WaveMatch()` intrinsic function from Shader Model
6.5 using the `OpGroupNonUniformPartitionNV` instruction from the
`SPV_NV_shader_subgroup_partitioned` extension.
SPIRV-Tools bumped to include:
https://github.com/KhronosGroup/SPIRV-Tools/pull/5648Fixes#6545
This adds the Clang 3.8 `-Wdouble-promotion` warning group to DXC. This
is added as part of the mitigations for the HLSL 202x conforming
literals feature which (among other things) changes the conversion rank
of unsuffixed literal values.
Resolves#6571
Antonio Maiorano
David Neto
Zhengxing li
Takuto Ikuta
Tex Riddell
Natalie Chouinard
Greg Roth
Steven Perron
Nielsbishere
Nathan Gauër
dependabot[bot]
Jeff Noyle
rj12122
Chris B
Jesse Natalie
Cooper Partin
Xiang Li