-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[HLSL] Fix Root signature test error #127261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-backend-directx Author: None (joaosaffran) ChangesFixing error detected in build bot in file closes: 127260 Full diff: https://github.com/llvm/llvm-project/pull/127261.diff 1 Files Affected:
diff --git a/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-MultipleEntryFunctions.ll b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-MultipleEntryFunctions.ll
index 652f8092b7a69..0547b0bae7a7e 100644
--- a/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-MultipleEntryFunctions.ll
+++ b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-MultipleEntryFunctions.ll
@@ -23,17 +23,16 @@ attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" }
!6 = !{ !7 } ; list of root signature elements
!7 = !{ !"RootFlags", i32 2 } ; 1 = allow_input_assembler_input_layout
-
-; CHECK-LABEL: Definition for 'main':
-; CHECK-NEXT: Flags: 0x000001
+; CHECK-LABEL: Definition for 'anotherMain':
+; CHECK-NEXT: Flags: 0x000002
; CHECK-NEXT: Version: 2
; CHECK-NEXT: NumParameters: 0
; CHECK-NEXT: RootParametersOffset: 0
; CHECK-NEXT: NumStaticSamplers: 0
; CHECK-NEXT: StaticSamplersOffset: 0
-; CHECK-LABEL: Definition for 'anotherMain':
-; CHECK-NEXT: Flags: 0x000002
+; CHECK-LABEL: Definition for 'main':
+; CHECK-NEXT: Flags: 0x000001
; CHECK-NEXT: Version: 2
; CHECK-NEXT: NumParameters: 0
; CHECK-NEXT: RootParametersOffset: 0
|
I don't think this is correct. It looks to me like this is printing the root signature flags in a non-deterministic order. The problem is that we're iterating over a map in RootSignatureAnalysisPrinter - this isn't guaranteed any particular order. Instead, we should iterate over the functions in the module and look them up in the map. |
const auto &[Function, RSD] = P; | ||
OS << "Definition for '" << Function->getName() << "':\n"; | ||
for (const auto &F : M) { | ||
const auto RSD = RSDMap.at(&F); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will crash if there are functions in the module without root signatures (ie, if the root signature metadata isn't present, or if there are helper functions without their own root signatures)
@@ -155,6 +155,7 @@ analyzeModule(Module &M) { | |||
|
|||
if (RootElementListNode == nullptr) { | |||
reportError(Ctx, "Missing Root Element List Metadata node."); | |||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to fix this in a separate PR (also are there any tests for this error? I'm surprised they don't crash or otherwise have issues)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a test for this at some point in time, might get lost during one of the iterations from the original PR. Will make a new one with this fix, a test and I will double-check the current tests.
for (const auto &P : RSDMap) { | ||
const auto &[Function, RSD] = P; | ||
OS << "Definition for '" << Function->getName() << "':\n"; | ||
for (const auto &F : M) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better style to spell out the type if it doesn't show up elsewhere (such as in a cast).
for (const auto &F : M) { | |
for (const Function &F : M) { |
const auto &[Function, RSD] = P; | ||
OS << "Definition for '" << Function->getName() << "':\n"; | ||
for (const auto &F : M) { | ||
const auto &RSD = RSDMap.find(&F); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type here is SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc>::iterator
- since this is actually a copy we shouldn't use a const reference here.
const auto &RSD = RSDMap.find(&F); | |
auto It = RSDMap.find(&F); |
3d24980
to
fb2aeee
Compare
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/10911 Here is the relevant piece of the build log for the reference
|
Fixing error detected in build bot in file `RootSignature-MultipleEntryFunctions.ll` closes: [127260](llvm#127260) --------- Co-authored-by: joaosaffran <[email protected]>
Fixing error detected in build bot in file
RootSignature-MultipleEntryFunctions.ll
closes: 127260