Signal when SPIR-V legalization needs targeted cleanup#8283
Open
AnastaZIuk wants to merge 2 commits intomicrosoft:mainfrom
Open
Signal when SPIR-V legalization needs targeted cleanup#8283AnastaZIuk wants to merge 2 commits intomicrosoft:mainfrom
AnastaZIuk wants to merge 2 commits intomicrosoft:mainfrom
Conversation
Contributor
You can test this locally with the following command:git-clang-format --diff 54afd2c6a5f1a5bcbef66967515fa69f5a3650fd 9ac57982725c4264348b20ad3fb271acc0993754 -- tools/clang/lib/Frontend/Rewrite/RewriteObjC.cpp tools/clang/lib/SPIRV/SpirvEmitter.cpp tools/clang/lib/SPIRV/SpirvEmitter.hView the diff from clang-format here.diff --git a/tools/clang/lib/SPIRV/SpirvEmitter.cpp b/tools/clang/lib/SPIRV/SpirvEmitter.cpp
index 4a616a00..0920fc1c 100644
--- a/tools/clang/lib/SPIRV/SpirvEmitter.cpp
+++ b/tools/clang/lib/SPIRV/SpirvEmitter.cpp
@@ -593,8 +593,7 @@ SpirvEmitter::SpirvEmitter(CompilerInstance &ci)
constEvaluator(astContext, spvBuilder), entryFunction(nullptr),
curFunction(nullptr), curThis(nullptr), seenPushConstantAt(),
isSpecConstantMode(false), needsLegalization(false),
- needsLegalizationLoopUnroll(false),
- needsLegalizationSsaRewrite(false),
+ needsLegalizationLoopUnroll(false), needsLegalizationSsaRewrite(false),
beforeHlslLegalization(false), mainSourceFile(nullptr) {
// Get ShaderModel from command line hlsl profile option.
@@ -957,8 +956,7 @@ void SpirvEmitter::HandleTranslationUnit(ASTContext &context) {
!dsetbindingsToCombineImageSampler.empty() ||
spirvOptions.signaturePacking;
needsLegalizationSsaRewrite =
- needsLegalizationSsaRewrite ||
- !dsetbindingsToCombineImageSampler.empty();
+ needsLegalizationSsaRewrite || !dsetbindingsToCombineImageSampler.empty();
// Run legalization passes
if (spirvOptions.codeGenHighLevel) {
@@ -16679,9 +16677,9 @@ bool SpirvEmitter::spirvToolsLegalize(std::vector<uint32_t> *mod,
} else if (needsLegalizationSsaRewrite) {
legalizationSsaRewriteMode = spvtools::SSARewriteMode::OpaqueOnly;
}
- optimizer.RegisterLegalizationPasses(
- spirvOptions.preserveInterface, needsLegalizationLoopUnroll,
- legalizationSsaRewriteMode);
+ optimizer.RegisterLegalizationPasses(spirvOptions.preserveInterface,
+ needsLegalizationLoopUnroll,
+ legalizationSsaRewriteMode);
// Add flattening of resources if needed.
if (spirvOptions.flattenResourceArrays) {
optimizer.RegisterPass(
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SpirvEmitterbits for legalization-time loop unroll and legalization-time SSA rewriteexternal/SPIRV-Headersandexternal/SPIRV-Toolssubmodule pointers to the matching branch stateSPIRV-Toolslegalization API and the current MSVCgetFunctionTypesignatureCodeGenSPIRVchecks to match the accepted SPIR-V shapeRoot cause
The expensive blanket policy lives in the SPIR-V optimizer recipe, not in HLSL parsing or the DXC CLI front-end.
DXC is still the producer that knows when a specific lowering path genuinely needs those expensive legalization transforms for correctness, so the optimizer should not have to guess that globally.
Two confirmed correctness-sensitive cases are:
ConstOffsetThis branch also fixes one separate producer-side correctness bug: vector splat of bool spec constants was being emitted as
OpConstantCompositeinstead of the legalOpSpecConstantComposite.The same narrowing also exposed one existing image-atomic cleanup dependency. The matching
LocalSingleStoreElimfollow-up lives in the companion KhronosGroup/SPIRV-Tools#6612 branch and is rolled here through the updated submodule pointer.Spec basis
The hard legality rules here are much narrower than the old blanket cleanup.
Spec:
The involved types are explicitly opaque:
Spec:
For the image-offset case, the relevant SPIR-V rule is also explicit:
Spec:
For the retained-loop case,
OpSelectis also explicitly allowed on pointer-typed objects:Spec:
For the image-atomic follow-up,
OpImageTexelPointeris also explicit:Spec:
For the spec-constant splat fix, the composite rules are also explicit:
Spec:
Vulkan tightens this further:
Spec:
Benchmark
19.161 s -> 3.02282 s(median of 3 runs)Validation
CodeGenSPIRVon this branch pair passes with1438expected passes,2expected failures, and0unexpected failuresUpdated test notes
vk.spec-constant.reuse.hlsl: the old check expectedOpConstantCompositefrom a bool spec-constant splat. That shape is invalid. This branch now emits the legalOpSpecConstantComposite.15-loop-var-unroll-ok.hlsl: the old check implicitly depended on eager optimizer-side full unroll. This branch keepsLoopControl::Unrollin the IR and uses a legal pointerOpSelectpath instead.cast.flat-conversion.matrix.hlsl: the old check assumed the matrix-to-struct path stayed as pre-folded constant composites. This branch rebuilds the target values from the loaded matrix viaOpCompositeExtractandOpCompositeConstruct. The stored value is the same and the row-major or column-major decoration still carries the memory layout rule.type.rwstructured-buffer.array.counter.indirect.hlslandtype.rwstructured-buffer.array.counter.indirect2.hlsl: the old checks assumed a single flattened access chain directly to the counter or data element. This branch keeps an intermediate structured access chain and then indexes the member. The resource, binding, atomic, and data access semantics are unchanged.vk.binding.global-struct-of-resources.optimized.hlsl: the old regex overconstrained generated ids. This branch preserves the same resource names and bindings but uses different legal ids, so the test now checks the stable semantic parts instead.max_id.hlsl: the old check overconstrained an exactBoundvalue. This branch still exercises the same larger-limit success path, but the exact bound is no longer stable after the accepted optimizer recipe changes.Companion SPIR-V optimizer PR:
KhronosGroup/SPIRV-Tools#6612