[DXIL] Remove incompatible metadata types when preparing DXIL. #136386
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-hlsl @llvm/pr-subscribers-backend-directx Author: Joshua Batista (bob80905) ChangesThis PR introduces a Metadata Node Kind whitelist. The purpose is to prevent newer Metadata Node Kinds to be used and inserted into the outputted DXIL module. The Github DXC validator doesn't support these newer Metadata Node Kinds, so we need to filter them out. that loops up to and including the last metadata node kind that should be disallowed in the current context. I believe this code shows that there are no other metadata nodes to be allowed, setting aside the explicit list shown in The accompanying test would add the loop.llvm.mustprogress metadata node kind, but thanks to the whitelist, filters it out, and so the whitelist is proven to work. Full diff: https://github.com/llvm/llvm-project/pull/136386.diff 2 Files Affected:
diff --git a/clang/test/CodeGenHLSL/metadata-stripping.hlsl b/clang/test/CodeGenHLSL/metadata-stripping.hlsl
new file mode 100644
index 0000000000000..0178f5efe4c01
--- /dev/null
+++ b/clang/test/CodeGenHLSL/metadata-stripping.hlsl
@@ -0,0 +1,13 @@
+// RUN: %clang --driver-mode=dxc -T cs_6_0 -Fo x.dxil %s | FileCheck %s
+// CHECK-NOT: llvm.loop.mustprogress
+
+StructuredBuffer<uint4> X : register(t0);
+StructuredBuffer<float4> In : register(t1);
+RWStructuredBuffer<float4> Out : register(u0);
+
+[numthreads(1, 1, 1)]
+void main(uint3 dispatch_thread_id : SV_DispatchThreadID) {
+ for (uint I = 0; I < X[dispatch_thread_id].x; ++I) {
+ Out[dispatch_thread_id] = In[dispatch_thread_id];
+ }
+}
diff --git a/llvm/lib/Target/DirectX/DXILPrepare.cpp b/llvm/lib/Target/DirectX/DXILPrepare.cpp
index de97de209048b..c54a5ac5c1227 100644
--- a/llvm/lib/Target/DirectX/DXILPrepare.cpp
+++ b/llvm/lib/Target/DirectX/DXILPrepare.cpp
@@ -189,6 +189,26 @@ class DXILPrepareModule : public ModulePass {
for (auto &BB : F) {
IRBuilder<> Builder(&BB);
for (auto &I : make_early_inc_range(BB)) {
+
+ // TODO: Audit this list - is it enough? Too much?
+ static unsigned DXILCompatibleMDs[] = {
+ LLVMContext::MD_dbg,
+ LLVMContext::MD_tbaa,
+ LLVMContext::MD_prof,
+ LLVMContext::MD_fpmath,
+ LLVMContext::MD_range,
+ LLVMContext::MD_tbaa_struct,
+ LLVMContext::MD_invariant_load,
+ LLVMContext::MD_alias_scope,
+ LLVMContext::MD_noalias,
+ LLVMContext::MD_nontemporal,
+ LLVMContext::MD_mem_parallel_loop_access,
+ LLVMContext::MD_nonnull,
+ LLVMContext::MD_dereferenceable,
+ LLVMContext::MD_dereferenceable_or_null,
+ };
+ I.dropUnknownNonDebugMetadata(DXILCompatibleMDs);
+
if (I.getOpcode() == Instruction::FNeg) {
Builder.SetInsertPoint(&I);
Value *In = I.getOperand(0);
|
|
@llvm/pr-subscribers-clang Author: Joshua Batista (bob80905) ChangesThis PR introduces a Metadata Node Kind whitelist. The purpose is to prevent newer Metadata Node Kinds to be used and inserted into the outputted DXIL module. The Github DXC validator doesn't support these newer Metadata Node Kinds, so we need to filter them out. that loops up to and including the last metadata node kind that should be disallowed in the current context. I believe this code shows that there are no other metadata nodes to be allowed, setting aside the explicit list shown in The accompanying test would add the loop.llvm.mustprogress metadata node kind, but thanks to the whitelist, filters it out, and so the whitelist is proven to work. Full diff: https://github.com/llvm/llvm-project/pull/136386.diff 2 Files Affected:
diff --git a/clang/test/CodeGenHLSL/metadata-stripping.hlsl b/clang/test/CodeGenHLSL/metadata-stripping.hlsl
new file mode 100644
index 0000000000000..0178f5efe4c01
--- /dev/null
+++ b/clang/test/CodeGenHLSL/metadata-stripping.hlsl
@@ -0,0 +1,13 @@
+// RUN: %clang --driver-mode=dxc -T cs_6_0 -Fo x.dxil %s | FileCheck %s
+// CHECK-NOT: llvm.loop.mustprogress
+
+StructuredBuffer<uint4> X : register(t0);
+StructuredBuffer<float4> In : register(t1);
+RWStructuredBuffer<float4> Out : register(u0);
+
+[numthreads(1, 1, 1)]
+void main(uint3 dispatch_thread_id : SV_DispatchThreadID) {
+ for (uint I = 0; I < X[dispatch_thread_id].x; ++I) {
+ Out[dispatch_thread_id] = In[dispatch_thread_id];
+ }
+}
diff --git a/llvm/lib/Target/DirectX/DXILPrepare.cpp b/llvm/lib/Target/DirectX/DXILPrepare.cpp
index de97de209048b..c54a5ac5c1227 100644
--- a/llvm/lib/Target/DirectX/DXILPrepare.cpp
+++ b/llvm/lib/Target/DirectX/DXILPrepare.cpp
@@ -189,6 +189,26 @@ class DXILPrepareModule : public ModulePass {
for (auto &BB : F) {
IRBuilder<> Builder(&BB);
for (auto &I : make_early_inc_range(BB)) {
+
+ // TODO: Audit this list - is it enough? Too much?
+ static unsigned DXILCompatibleMDs[] = {
+ LLVMContext::MD_dbg,
+ LLVMContext::MD_tbaa,
+ LLVMContext::MD_prof,
+ LLVMContext::MD_fpmath,
+ LLVMContext::MD_range,
+ LLVMContext::MD_tbaa_struct,
+ LLVMContext::MD_invariant_load,
+ LLVMContext::MD_alias_scope,
+ LLVMContext::MD_noalias,
+ LLVMContext::MD_nontemporal,
+ LLVMContext::MD_mem_parallel_loop_access,
+ LLVMContext::MD_nonnull,
+ LLVMContext::MD_dereferenceable,
+ LLVMContext::MD_dereferenceable_or_null,
+ };
+ I.dropUnknownNonDebugMetadata(DXILCompatibleMDs);
+
if (I.getOpcode() == Instruction::FNeg) {
Builder.SetInsertPoint(&I);
Value *In = I.getOperand(0);
|
|
Created a new issue to improve range metadata validation here: |
| @@ -189,6 +201,9 @@ class DXILPrepareModule : public ModulePass { | |||
| for (auto &BB : F) { | |||
| IRBuilder<> Builder(&BB); | |||
| for (auto &I : make_early_inc_range(BB)) { | |||
|
|
|||
| I.dropUnknownNonDebugMetadata(DXILCompatibleMDs); | |||
|
|
|||
| if (I.getOpcode() == Instruction::FNeg) { | |||
There was a problem hiding this comment.
DXILPrepare has a bunch of overlap with the legalization pass. Seems like MetaData might not be able to move because legalization is a function level pass and this is a module level one, but at a minimum it seems like FNeg -> FSub should move.
There was a problem hiding this comment.
I agree. I think DXILPrepare should do things like translate attributes and metadata, insert no-op bitcasts to appease the bitcode writer, and that sort of thing. Replacing ops with their equivalents should squarely be a part of legalization, not here.
I've filed #137685
…136386) This PR introduces a Metadata Node Kind allowlist. The purpose is to prevent newer Metadata Node Kinds to be used and inserted into the outputted DXIL module. Only the metadata kinds that are accepted in the DXIL Validator are on the allowlist. The Github DXC validator doesn't support these newer Metadata Node Kinds, so we need to filter them out. We introduce this restrictive allowlist into LLVM and strip all metadata that isn't found in the list. The accompanying test would add the `llvm.loop.mustprogress` metadata node kind, but thanks to the allowlist, filters it out, and so the whitelist is proven to work. The test also has two separate metadata kinds that are on the allowlist, and remain after the DXIL Prepare pass.
…136386) This PR introduces a Metadata Node Kind allowlist. The purpose is to prevent newer Metadata Node Kinds to be used and inserted into the outputted DXIL module. Only the metadata kinds that are accepted in the DXIL Validator are on the allowlist. The Github DXC validator doesn't support these newer Metadata Node Kinds, so we need to filter them out. We introduce this restrictive allowlist into LLVM and strip all metadata that isn't found in the list. The accompanying test would add the `llvm.loop.mustprogress` metadata node kind, but thanks to the allowlist, filters it out, and so the whitelist is proven to work. The test also has two separate metadata kinds that are on the allowlist, and remain after the DXIL Prepare pass.
…136386) This PR introduces a Metadata Node Kind allowlist. The purpose is to prevent newer Metadata Node Kinds to be used and inserted into the outputted DXIL module. Only the metadata kinds that are accepted in the DXIL Validator are on the allowlist. The Github DXC validator doesn't support these newer Metadata Node Kinds, so we need to filter them out. We introduce this restrictive allowlist into LLVM and strip all metadata that isn't found in the list. The accompanying test would add the `llvm.loop.mustprogress` metadata node kind, but thanks to the allowlist, filters it out, and so the whitelist is proven to work. The test also has two separate metadata kinds that are on the allowlist, and remain after the DXIL Prepare pass.
…136386) This PR introduces a Metadata Node Kind allowlist. The purpose is to prevent newer Metadata Node Kinds to be used and inserted into the outputted DXIL module. Only the metadata kinds that are accepted in the DXIL Validator are on the allowlist. The Github DXC validator doesn't support these newer Metadata Node Kinds, so we need to filter them out. We introduce this restrictive allowlist into LLVM and strip all metadata that isn't found in the list. The accompanying test would add the `llvm.loop.mustprogress` metadata node kind, but thanks to the allowlist, filters it out, and so the whitelist is proven to work. The test also has two separate metadata kinds that are on the allowlist, and remain after the DXIL Prepare pass.
…136386) This PR introduces a Metadata Node Kind allowlist. The purpose is to prevent newer Metadata Node Kinds to be used and inserted into the outputted DXIL module. Only the metadata kinds that are accepted in the DXIL Validator are on the allowlist. The Github DXC validator doesn't support these newer Metadata Node Kinds, so we need to filter them out. We introduce this restrictive allowlist into LLVM and strip all metadata that isn't found in the list. The accompanying test would add the `llvm.loop.mustprogress` metadata node kind, but thanks to the allowlist, filters it out, and so the whitelist is proven to work. The test also has two separate metadata kinds that are on the allowlist, and remain after the DXIL Prepare pass.
This PR introduces a Metadata Node Kind allowlist. The purpose is to prevent newer Metadata Node Kinds to be used and inserted into the outputted DXIL module. Only the metadata kinds that are accepted in the DXIL Validator are on the allowlist. The Github DXC validator doesn't support these newer Metadata Node Kinds, so we need to filter them out.
We introduce this restrictive allowlist into LLVM and strip all metadata that isn't found in the list.
The accompanying test would add the
llvm.loop.mustprogressmetadata node kind, but thanks to the allowlist, filters it out, and so the whitelist is proven to work.The test also has two separate metadata kinds that are on the allowlist, and remain after the DXIL Prepare pass.