-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[SPIRV] Support -fembed-bitcode=marker for non-shader modules #162082
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-spir-v Author: Juan Manuel Martinez Caamaño (jmmartinez) Changes-fembed-bitcode=marker gets lowered as a [0 x i8] zeroinitialized global To work around this, we replace the [0 x i8] by a zeroinitialized I'm not sure this is the way we want to support this, neither for shaders (using runtime arrays) nor for kernels (using the 1-sized zeroinitialized array). Depends on #162081 Full diff: https://github.com/llvm/llvm-project/pull/162082.diff 4 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp b/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
index 9f2e07508a36a..447fbb9d1ac0a 100644
--- a/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
@@ -236,6 +236,8 @@ class SPIRVEmitIntrinsics
Instruction *buildLogicalAccessChainFromGEP(GetElementPtrInst &GEP);
+ bool promoteEmbddedBitcodeMarker(Module &M) const;
+
public:
static char ID;
SPIRVEmitIntrinsics(SPIRVTargetMachine *TM = nullptr)
@@ -3007,9 +3009,43 @@ void SPIRVEmitIntrinsics::parseFunDeclarations(Module &M) {
}
}
+bool SPIRVEmitIntrinsics::promoteEmbddedBitcodeMarker(Module &M) const {
+ const SPIRVSubtarget *STI = TM->getSubtargetImpl();
+ if (STI->isShader())
+ return false;
+
+ GlobalVariable *EmbeddedBitcode = M.getNamedGlobal("llvm.embedded.module");
+ if (!EmbeddedBitcode)
+ return false;
+
+ ArrayType *AT = cast<ArrayType>(EmbeddedBitcode->getValueType());
+ if (AT->getNumElements() != 0)
+ return false;
+
+ // When compiling with -fembed-bitcode=marker, LLVM generates a [0 x i8]
+ // zeroinitialized global variable containing the bitcode. This results in an
+ // assert outside of shaders. As a workaround, we replace this global with a
+ // zeroinitialized [1 x i8].
+ ArrayType *AT1 = ArrayType::get(AT->getElementType(), 1);
+ Constant *ZeroInit = Constant::getNullValue(AT1);
+ GlobalVariable *NewEmbeddedBitcode = new GlobalVariable(
+ AT1, EmbeddedBitcode->isConstant(), EmbeddedBitcode->getLinkage(),
+ ZeroInit, "", EmbeddedBitcode->getThreadLocalMode(),
+ EmbeddedBitcode->getAddressSpace(),
+ EmbeddedBitcode->isExternallyInitialized());
+ NewEmbeddedBitcode->setSection(NewEmbeddedBitcode->getSection());
+ NewEmbeddedBitcode->takeName(EmbeddedBitcode);
+
+ M.insertGlobalVariable(NewEmbeddedBitcode);
+ EmbeddedBitcode->replaceAllUsesWith(NewEmbeddedBitcode);
+ EmbeddedBitcode->eraseFromParent();
+ return true;
+}
+
bool SPIRVEmitIntrinsics::runOnModule(Module &M) {
bool Changed = false;
+ Changed |= promoteEmbddedBitcodeMarker(M);
parseFunDeclarations(M);
insertConstantsForFPFastMathDefault(M);
diff --git a/llvm/test/CodeGen/SPIRV/fembed-bitcode-marker-shader.ll b/llvm/test/CodeGen/SPIRV/fembed-bitcode-marker-shader.ll
new file mode 100644
index 0000000000000..20ac543486766
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/fembed-bitcode-marker-shader.ll
@@ -0,0 +1,21 @@
+; RUN: llc -verify-machineinstrs -mtriple=spirv-vulkan-unknown %s -o - | FileCheck %s
+
+@llvm.embedded.module = private constant [0 x i8] zeroinitializer, section ".llvmbc", align 1
+@llvm.cmdline = private constant [5 x i8] c"-cc1\00", section ".llvmcmd", align 1
+@llvm.compiler.used = appending global [2 x ptr] [ptr @llvm.embedded.module, ptr @llvm.cmdline], section "llvm.metadata"
+
+; CHECK-DAG: OpName [[FOO:%[0-9]+]] "foo"
+; CHECK-DAG: OpName [[MODULE:%[0-9]+]] "llvm.embedded.module"
+; CHECK-DAG: [[INT8:%[0-9]+]] = OpTypeInt 8 0
+; CHECK-DAG: [[RUNTIME_ARRAY_INT8x1:%[0-9]+]] = OpTypeRuntimeArray [[INT8]]
+; CHECK-DAG: [[POINTER:%[0-9]+]] = OpTypePointer Function [[RUNTIME_ARRAY_INT8x1]]
+; CHECK-DAG: [[EMBEDDED_MODULE_INIT:%[0-9]+]] = OpConstantNull [[RUNTIME_ARRAY_INT8x1]]
+; CHECK: [[FOO]] = OpFunction {{.*}} None {{.*}}
+; CHECK-DAG: {{%[0-9]+}} = OpVariable [[POINTER]] Function [[EMBEDDED_MODULE_INIT]]
+
+define void @foo() #1 {
+entry:
+ ret void
+}
+
+attributes #1 = { "hlsl.numthreads"="4,8,16" "hlsl.shader"="compute" }
diff --git a/llvm/test/CodeGen/SPIRV/fembed-bitcode-marker.ll b/llvm/test/CodeGen/SPIRV/fembed-bitcode-marker.ll
new file mode 100644
index 0000000000000..e3d21fcdc8084
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/fembed-bitcode-marker.ll
@@ -0,0 +1,21 @@
+; RUN: llc -verify-machineinstrs -mtriple=spirv-unknown-unknown %s -o - | FileCheck %s
+
+@llvm.embedded.module = private constant [0 x i8] zeroinitializer, section ".llvmbc", align 1
+@llvm.cmdline = private constant [5 x i8] c"-cc1\00", section ".llvmcmd", align 1
+@llvm.compiler.used = appending global [2 x ptr] [ptr @llvm.embedded.module, ptr @llvm.cmdline], section "llvm.metadata"
+
+; CHECK-DAG: OpName [[FOO:%[0-9]+]] "foo"
+; CHECK-DAG: OpName [[MODULE:%[0-9]+]] "llvm.embedded.module"
+; CHECK-DAG: [[INT8:%[0-9]+]] = OpTypeInt 8 0
+; CHECK-DAG: [[INT32:%[0-9]+]] = OpTypeInt 32 0
+; CHECK-DAG: [[CONST_1_32:%[0-9]+]] = OpConstant [[INT32]] 1
+; CHECK-DAG: [[ARRAY_INT8x1:%[0-9]+]] = OpTypeArray [[INT8]] [[CONST_1_32]]
+; CHECK-DAG: [[POINTER:%[0-9]+]] = OpTypePointer Function [[ARRAY_INT8x1]]
+; CHECK-DAG: [[EMBEDDED_MODULE_INIT:%[0-9]+]] = OpConstantNull [[ARRAY_INT8x1]]
+; CHECK: [[FOO]] = OpFunction {{.*}} None {{.*}}
+; CHECK-DAG: {{%[0-9]+}} = OpVariable [[POINTER]] Function [[EMBEDDED_MODULE_INIT]]
+
+define spir_kernel void @foo() {
+entry:
+ ret void
+}
diff --git a/llvm/test/CodeGen/SPIRV/fembed-bitcode.ll b/llvm/test/CodeGen/SPIRV/fembed-bitcode.ll
new file mode 100644
index 0000000000000..0044e09aceaab
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/fembed-bitcode.ll
@@ -0,0 +1,25 @@
+; RUN: llc -verify-machineinstrs -mtriple=spirv-unknown-unknown %s -o - | FileCheck %s
+
+@llvm.embedded.module = private constant [4 x i8] c"BC\C0\DE", section ".llvmbc", align 1
+@llvm.cmdline = private constant [5 x i8] c"-cc1\00", section ".llvmcmd", align 1
+@llvm.compiler.used = appending global [2 x ptr] [ptr @llvm.embedded.module, ptr @llvm.cmdline], section "llvm.metadata"
+
+; CHECK-DAG: OpName [[FOO:%[0-9]+]] "foo"
+; CHECK-DAG: OpName [[MODULE:%[0-9]+]] "llvm.embedded.module"
+; CHECK-DAG: [[INT8:%[0-9]+]] = OpTypeInt 8 0
+; CHECK-DAG: [[INT32:%[0-9]+]] = OpTypeInt 32 0
+; CHECK-DAG: [[CONST_4_32:%[0-9]+]] = OpConstant [[INT32]] 4
+; CHECK-DAG: [[ARRAY_INT8x4:%[0-9]+]] = OpTypeArray [[INT8]] [[CONST_4_32]]
+; CHECK-DAG: [[POINTER:%[0-9]+]] = OpTypePointer Function [[ARRAY_INT8x4]]
+; CHECK-DAG: [[CONST_B_8:%[0-9]+]] = OpConstant [[INT8]] 66
+; CHECK-DAG: [[CONST_C_8:%[0-9]+]] = OpConstant [[INT8]] 67
+; CHECK-DAG: [[CONST_0xC0_8:%[0-9]+]] = OpConstant [[INT8]] 192
+; CHECK-DAG: [[CONST_0xDE_8:%[0-9]+]] = OpConstant [[INT8]] 222
+; CHECK-DAG: [[EMBEDDED_MODULE_INIT:%[0-9]+]] = OpConstantComposite [[ARRAY_INT8x4]] [[CONST_B_8]] [[CONST_C_8]] [[CONST_0xC0_8]] [[CONST_0xDE_8]]
+; CHECK: [[FOO]] = OpFunction {{.*}} None {{.*}}
+; CHECK-DAG: {{%[0-9]+}} = OpVariable [[POINTER]] Function [[EMBEDDED_MODULE_INIT]]
+
+define spir_kernel void @foo() {
+entry:
+ ret void
+}
|
@@ -0,0 +1,21 @@ | |||
; RUN: llc -verify-machineinstrs -mtriple=spirv-vulkan-unknown %s -o - | FileCheck %s |
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.
Please, add also a check with spirv-val. Applies to all tests.
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.
Done in the latest update. I'll also update the parent PR #162081
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 test started failing once I added the spirv-val line. I'm checking this
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 think that I have now more context about all of this.
The @llvm.embedded.module
stuff only make sense when the spirv flavour is AMD's, and not for the others. In this case, the variables are in the constant addrspace(1) and are addrspace-casted to the generic addrspace(4) when added to llvm.compiler.used
.
This is allowed only in amd's flavour but is outside of spirv standard.
I think I'm going to change my strategy:
- I'll add an AMD specific pass that is only added to the pipeline if the vendor is AMD to put this kind of stuff over there.
- Maybe put this kind of test not in
test/CodeGen/SPIRV
but in a subfoldertest/CodeGen/SPIRV/amd
or something like that.
What do you think ?
There was this thread in a related PR that might be relevant here: #149522 (comment). I'm not against this, but there were concerns in the past with a similar solution, although in this case, the variable might not actually be used. Did you try simply deleting it and seeing what happens? What would be the cons of deleting it? |
39d7fbf
to
8367a5c
Compare
When In the shader case, I'm leaning towards handling the bitcode-marker in the same way as for the kernel case. I would have liked to have an |
-fembed-bitcode=marker gets lowered as a [0 x i8] zeroinitialized global variable. This is interpreted as a runtime-array and is not supported in non-shaders. To work around this, we replace the [0 x i8] by a zeroinitialized single-element array.
8367a5c
to
5c2271c
Compare
|
||
Instruction *buildLogicalAccessChainFromGEP(GetElementPtrInst &GEP); | ||
|
||
bool promoteEmbeddedBitcodeMarkear(Module &M) const; |
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.
"Markear" (possible typo)
@@ -0,0 +1,22 @@ | |||
; RUN: llc -verify-machineinstrs -mtriple=spirv-vulkan-unknown %s -o - | FileCheck %s | |||
; RUN: %if spirv-tools %{ llc -mtriple=spirv-unknown-unknown %s -o - -filetype=obj | spirv-val %} |
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.
; RUN: %if spirv-tools %{ llc -mtriple=spirv-unknown-unknown %s -o - -filetype=obj | spirv-val %} | |
; RUN: %if spirv-tools %{ llc -mtriple=spirv-vulkan-unknown %s -o - -filetype=obj | spirv-val %} |
I'm closing this PR (and its parent #162081). I think I got this wrong. The AS of the variables in the tests should not be the default one, it should be 1. But that introduces an addrspace cast from I'm going to rethink about this and tackle this issue later. Thanks a lot for the comments still ! |
-fembed-bitcode=marker gets lowered as a [0 x i8] zeroinitialized global
variable. This is interpreted as a runtime-array and is not supported in
non-shaders.
To work around this, we replace the [0 x i8] by a zeroinitialized
single-element array.
I'm not sure this is the way we want to support this, neither for shaders (using runtime arrays) nor for kernels (using the 1-sized zeroinitialized array).
A part of me would like to just delete the variable since we do not really need the delimiter.
Depends on #162081