[HLSL][RootSignature] Audit RootSignatureParser diagnostic production
#147800
Conversation
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
inbelic
force-pushed
the
inbelic/rs-simplify-diags
branch
from
7d8fa5d to
70448a3
Compare
6 tasks
joaosaffran
approved these changes
Jul 11, 2025
- It has been noted more than once that this diagnostic is confusing and misleading. It can be more concise
the order of the parameters is switched as opposed to either and this was not caught in the unit tests
inbelic
force-pushed
the
inbelic/rs-simplify-diags
branch
from
70448a3 to
8e55607
Compare
|
Hey, this seems to have broken https://green.lab.llvm.org/job/llvm.org/job/clang-stage1-cmake-RA-expensive/4708/testReport/Clang/SemaHLSL/RootSignature_resource_ranges_err_hlsl/, would you mind looking into it? |
|
@guy-david Thanks for pointing this out, I have a similar report here. I am taking a look into both now, but I suspect they might be the same issue. |
inbelic
added a commit
that referenced
this pull request
Jul 14, 2025
…ent association (#148649) - when there are duplicate (equivalent) `RangeInfo`s created we will attempt to `llvm::sort` or `llvm::stable_sort` them, it does not appear deterministic in which order they will be sorted (because they are equivalent) - when `DLLVM_ENABLE_EXSTENSIVE_CHECKS` is enabled, it appears to deal with this tie-breaker sorting the list differently than when it is not enabled, this causes one of the test cases to fail because the diagnostic is produced, not in a different order, but with a different root element associated with an identical `RangeInfo` - functionally this makes no difference to the diagnostic being produced and the removed test-case was added just as a nicety to demonstrate how the diagnostics might look - the test above the removed will correctly demonstrate that the `SourceLocation` will be set to the correct column, and, the `-verify` portion of this testcase will ensure that we still generate a diagnostic for each duplicate - therefore, it is safe to remove this portion of the test-case that can have a non-deterministic association of root element This resolves the build issues reported [here](#147115 (comment)) and [here](#147800 (comment))
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 14, 2025
…c root element association (#148649) - when there are duplicate (equivalent) `RangeInfo`s created we will attempt to `llvm::sort` or `llvm::stable_sort` them, it does not appear deterministic in which order they will be sorted (because they are equivalent) - when `DLLVM_ENABLE_EXSTENSIVE_CHECKS` is enabled, it appears to deal with this tie-breaker sorting the list differently than when it is not enabled, this causes one of the test cases to fail because the diagnostic is produced, not in a different order, but with a different root element associated with an identical `RangeInfo` - functionally this makes no difference to the diagnostic being produced and the removed test-case was added just as a nicety to demonstrate how the diagnostics might look - the test above the removed will correctly demonstrate that the `SourceLocation` will be set to the correct column, and, the `-verify` portion of this testcase will ensure that we still generate a diagnostic for each duplicate - therefore, it is safe to remove this portion of the test-case that can have a non-deterministic association of root element This resolves the build issues reported [here](llvm/llvm-project#147115 (comment)) and [here](llvm/llvm-project#147800 (comment))
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.
This pr audits the diagnostics produced in
RootSignatureParserdiagnostics.First, it has been noted more than once that the current
diag::err_hlsl_unexpected_end_of_paramsis not direct and can be misleading. For instance, here and here.This pr address this by removing this diagnostic and replacing it with a more direct
diag::err_expected_either. However, doing so removes the nuance between the case where it is a missing comma and when it was an invalid parameter.Hence, we introduce the
diag::err_hlsl_invalid_tokenfor the latter case, which does so in a direct way. Further, we can apply the same diagnostic to the parsing of parameter values.As part of this, we see that there was a test gap in testing the diagnostics produced from
diag::err_expected_afterand for the parsing of enum/flag values. As such, these are also addressed here to provide sufficient unit/sema test coverage.diag::err_hlsl_unexpected_end_of_paramsinRootSigantureParserdiag::err_hlsl_invalid_tokento provide a direct diagnosticdiag::err_hlsl_invalid_tokenordiag::err_expected_eitheraccordinglyHLSLRootSignatureParserTestto account for diagnostic changesHLSLRootSignatureParserTestfor enum/flag diagnosticsRootSignatures-errfor enum/flag diagnosticsdiag::err_expected_afterand add test to demonstrate usageResolves: #147799