-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[DirectX] Use an allow-list of DXIL compatible module metadata #165290
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
Changes from 2 commits
0c6079a
c781496
fe497d3
34d6d61
0019e62
7966dad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -364,6 +364,16 @@ static void cleanModuleFlags(Module &M) { | |
| M.addModuleFlag(Flag.Behavior, Flag.Key->getString(), Flag.Val); | ||
| } | ||
|
|
||
| using GlobalMDList = std::array<StringLiteral, 7>; | ||
|
|
||
| // The following are compatible with DXIL but not emit with clang, they can | ||
| // be added when applicable: | ||
| // dx.typeAnnotations, dx.viewIDState, dx.dxrPayloadAnnotations | ||
| static GlobalMDList CompatibleNamedModuleMDs = { | ||
| "llvm.ident", "llvm.module.flags", "dx.resources", "dx.valver", | ||
| "dx.shaderModel", "dx.version", "dx.entryPoints", | ||
| }; | ||
|
|
||
| static void translateGlobalMetadata(Module &M, DXILResourceMap &DRM, | ||
| DXILResourceTypeMap &DRTM, | ||
| const ModuleShaderFlags &ShaderFlags, | ||
|
|
@@ -426,19 +436,17 @@ static void translateGlobalMetadata(Module &M, DXILResourceMap &DRM, | |
|
|
||
| cleanModuleFlags(M); | ||
|
|
||
| // dx.rootsignatures will have been parsed from its metadata form as its | ||
| // binary form as part of the RootSignatureAnalysisWrapper, so safely | ||
| // remove it as it is not recognized in DXIL | ||
| if (NamedMDNode *RootSignature = M.getNamedMetadata("dx.rootsignatures")) | ||
| RootSignature->eraseFromParent(); | ||
|
|
||
| // llvm.errno.tbaa was recently added but is not supported in LLVM 3.7 and | ||
| // causes all tests using the DXIL Validator to fail. | ||
| // | ||
| // This is a temporary fix and should be replaced with a allowlist once | ||
| // we have determined all metadata that the DXIL Validator allows | ||
| if (NamedMDNode *ErrNo = M.getNamedMetadata("llvm.errno.tbaa")) | ||
| ErrNo->eraseFromParent(); | ||
| // Finally, strip all module metadata that is not explicitly specified in the | ||
| // allow-list | ||
| SmallVector<NamedMDNode *> ToStrip; | ||
|
|
||
| for (NamedMDNode &NamedMD : M.named_metadata()) | ||
| if (!NamedMD.getName().starts_with("llvm.dbg.") && | ||
| !llvm::is_contained(CompatibleNamedModuleMDs, NamedMD.getName())) | ||
| ToStrip.push_back(&NamedMD); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Is the reason you don't erasefromparent right here because the for loop's iteration would skip over the next node?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, it caused a crash when I removed it in place. Okay, sounds good. I will take look for it. edit: it seems for this case for this case it isn't possible |
||
|
|
||
| for (NamedMDNode *NamedMD : ToStrip) | ||
| NamedMD->eraseFromParent(); | ||
| } | ||
|
|
||
| PreservedAnalyses DXILTranslateMetadata::run(Module &M, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| ; RUN: opt -S -dxil-translate-metadata < %s | FileCheck %s | ||
|
|
||
| ; Ensures that only metadata explictly specified on the allow list, or debug | ||
| ; related, metadata is emitted | ||
|
|
||
| target triple = "dxil-unknown-shadermodel6.0-compute" | ||
|
|
||
| ; CHECK-NOT: !dx.rootsignatures | ||
| ; CHECK-NOT: !llvm.errno.tbaa | ||
|
|
||
| ; CHECK-DAG: !llvm.dbg.cu | ||
|
|
||
| ; CHECK-DAG: !llvm.module.flags = !{![[#DWARF_VER:]], ![[#DEBUG_VER:]]} | ||
| ; CHECK-DAG: !llvm.ident = !{![[#IDENT:]]} | ||
|
|
||
| ; CHECK-DAG: !dx.shaderModel | ||
| ; CHECK-DAG: !dx.version | ||
| ; CHECK-DAG: !dx.entryPoints | ||
| ; CHECK-DAG: !dx.valver | ||
| ; CHECK-DAG: !dx.resources | ||
|
|
||
| ; CHECK-NOT: !dx.rootsignatures | ||
| ; CHECK-NOT: !llvm.errno.tbaa | ||
|
|
||
| ; Check allowed llvm metadata structure to ensure it is still DXIL compatible | ||
| ; If this fails, please ensure that the updated form is DXIL compatible before | ||
| ; updating the test. | ||
|
|
||
| ; CHECK-DAG: ![[#IDENT]] = !{!"clang 22.0.0"} | ||
| ; CHECK-DAG: ![[#DWARF_VER]] = !{i32 2, !"Dwarf Version", i32 2} | ||
| ; CHECK-DAG: ![[#DEBUG_VER]] = !{i32 2, !"Debug Info Version", i32 3} | ||
|
|
||
| ; CHECK-NOT: !dx.rootsignatures | ||
| ; CHECK-NOT: !llvm.errno.tbaa | ||
|
|
||
| @BufA.str = private unnamed_addr constant [5 x i8] c"BufA\00", align 1 | ||
|
|
||
| define void @main () #0 { | ||
| entry: | ||
| %typed0 = call target("dx.TypedBuffer", <4 x float>, 1, 0, 0) | ||
| @llvm.dx.resource.handlefrombinding.tdx.TypedBuffer_v4f32_1_0_0( | ||
| i32 3, i32 5, i32 1, i32 0, ptr @BufA.str) | ||
| ret void | ||
| } | ||
|
|
||
| attributes #0 = { noinline nounwind "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" } | ||
|
|
||
| ; Incompatible | ||
| !dx.rootsignatures = !{!2} | ||
| !llvm.errno.tbaa = !{!5} | ||
|
|
||
| ; Compatible | ||
| !llvm.dbg.cu = !{!8} | ||
| !llvm.module.flags = !{!11, !12} | ||
| !llvm.ident = !{!13} | ||
| !dx.valver = !{!14} | ||
|
|
||
| !2 = !{ ptr @main, !3, i32 2 } | ||
| !3 = !{ !4 } | ||
| !4 = !{ !"RootFlags", i32 1 } | ||
|
|
||
| !5 = !{!6, !6, i64 0} | ||
| !6 = !{!"omnipotent char", !7} | ||
| !7 = !{!"Simple C/C++ TBAA"} | ||
|
|
||
| !8 = distinct !DICompileUnit(language: DW_LANG_C99, file: !9, producer: "Some Compiler", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !10, splitDebugInlining: false, nameTableKind: None) | ||
| !9 = !DIFile(filename: "hlsl.hlsl", directory: "/some-path") | ||
| !10 = !{} | ||
|
|
||
| !11 = !{i32 2, !"Dwarf Version", i32 2} | ||
| !12 = !{i32 2, !"Debug Info Version", i32 3} | ||
|
|
||
| !13 = !{!"clang 22.0.0"} | ||
|
|
||
| !14 = !{i32 1, i32 1} |
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 is a very nit-picky thing but I would put each element of the list on its own line.
One of the benefits is if there is a new commit, the diff will show the line(s) added or removed as opposed to a reviewer needing to scan for a change in a single line.
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.
I agree, unfortunately clang-format does not :(