Skip to content

[IR] Disallow ZeroInit for spirv.Image #73887

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

Merged
merged 3 commits into from
Dec 19, 2023

Conversation

wenju-he
Copy link
Contributor

According to spirv spec, OpConstantNull's result type can't be image
type. So we can't generate zeroinitializer for spirv.Image.

According to spirv spec, OpConstantNull's result type can't be image
type. So we can't generate zeroinitializer for spirv.Image.
@wenju-he wenju-he marked this pull request as ready for review December 1, 2023 01:48
@llvmbot
Copy link
Member

llvmbot commented Dec 1, 2023

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-transforms

Author: Wenju He (wenju-he)

Changes

According to spirv spec, OpConstantNull's result type can't be image
type. So we can't generate zeroinitializer for spirv.Image.


Full diff: https://github.com/llvm/llvm-project/pull/73887.diff

2 Files Affected:

  • (modified) llvm/lib/IR/Type.cpp (+2)
  • (modified) llvm/unittests/Transforms/Utils/ValueMapperTest.cpp (+1-1)
diff --git a/llvm/lib/IR/Type.cpp b/llvm/lib/IR/Type.cpp
index 3d2e203a20dac77..e3a09018ad8b98f 100644
--- a/llvm/lib/IR/Type.cpp
+++ b/llvm/lib/IR/Type.cpp
@@ -841,6 +841,8 @@ struct TargetTypeInfo {
 static TargetTypeInfo getTargetTypeInfo(const TargetExtType *Ty) {
   LLVMContext &C = Ty->getContext();
   StringRef Name = Ty->getName();
+  if (Name.equals("spirv.Image"))
+    return TargetTypeInfo(PointerType::get(C, 0), TargetExtType::CanBeGlobal);
   if (Name.startswith("spirv."))
     return TargetTypeInfo(PointerType::get(C, 0), TargetExtType::HasZeroInit,
                           TargetExtType::CanBeGlobal);
diff --git a/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp b/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
index 17083b3846430dd..c0c9d383ac18166 100644
--- a/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
+++ b/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
@@ -423,7 +423,7 @@ TEST(ValueMapperTest, mapValuePoisonWithTypeRemap) {
 
 TEST(ValueMapperTest, mapValueConstantTargetNoneToLayoutTypeNullValue) {
   LLVMContext C;
-  auto *OldTy = TargetExtType::get(C, "spirv.Image");
+  auto *OldTy = TargetExtType::get(C, "spirv.Event");
   Type *NewTy = OldTy->getLayoutType();
 
   TestTypeRemapper TM(NewTy);

@wenju-he
Copy link
Contributor Author

wenju-he commented Dec 1, 2023

@jcranmer-intel could you please take a look?
Before this PR, there could be following ir generated:

store target("spirv.Image", void, 1, 0, 0, 0, 0, 0, 0) zeroinitializer, ptr addrspace(4) %MImageObj, align 8

llvm-spirv translator isn't able to handle it due to spec requirement.

@wenju-he
Copy link
Contributor Author

wenju-he commented Dec 8, 2023

kindly ping @jcranmer-intel

@yubingex007-a11y yubingex007-a11y merged commit 108989b into llvm:main Dec 19, 2023
@wenju-he wenju-he deleted the spirv.Image-no-ZeroInit branch December 19, 2023 05:54
@bogner
Copy link
Contributor

bogner commented Dec 19, 2023

This change causes a number of the spir-v backend tests to crash:

Failed Tests (5):
LLVM :: CodeGen/SPIRV/image-unoptimized.ll
LLVM :: CodeGen/SPIRV/read_image.ll
LLVM :: CodeGen/SPIRV/transcoding/get_image_num_mip_levels.ll
LLVM :: CodeGen/SPIRV/transcoding/image_with_access_qualifiers.ll
LLVM :: CodeGen/SPIRV/transcoding/spirv-types.ll

@wenju-he
Copy link
Contributor Author

wenju-he commented Dec 20, 2023

image-unoptimized.ll

backtrace:

 #0 0x0000000001d2f0e1 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (llvm-project/build/bin/llc+0x1d2f0e1)
 #1 0x0000000001d2c644 SignalHandler(int) Signals.cpp:0:0
 #2 0x00007fd516a7ddb0 __restore_rt (/lib64/libc.so.6+0x59db0)
 #3 0x00007fd516aca42c __pthread_kill_implementation (/lib64/libc.so.6+0xa642c)
 #4 0x00007fd516a7dd06 gsignal (/lib64/libc.so.6+0x59d06)
 #5 0x00007fd516a507d3 abort (/lib64/libc.so.6+0x2c7d3)
 #6 0x00007fd516a506fb _nl_load_domain.cold (/lib64/libc.so.6+0x2c6fb)
 #7 0x00007fd516a76c86 (/lib64/libc.so.6+0x52c86)
 #8 0x00000000014b3bf1 llvm::ConstantTargetNone::get(llvm::TargetExtType*) (llvm-project/build/bin/llc+0x14b3bf1)
 #9 0x0000000000abac68 (anonymous namespace)::SPIRVEmitIntrinsics::runOnFunction(llvm::Function&) (.part.0) SPIRVEmitIntrinsics.cpp:0:0

SPIRVEmitIntrinsics pass probably needs fix to not generate ConstantTargetNone for spirv.Image. @bogner WDYT?

darkbuck added a commit that referenced this pull request Dec 20, 2023
- After #73887, spirv.Image cannot has a zeroinitializer, even though
  it's only used in metadata to pass down the type info to the backend.
  Instead of creating zeros, create undef ones of that target-specific
  types.
dneto0 added a commit to dneto0/clspv that referenced this pull request Dec 20, 2023
The spirv.Image target type no longer supports zeroinitializer value
(See llvm/llvm-project#73887)

Use undef instead.

Add a new clspv::GetDummyValue that returns the zeroinitializer value
when it can, and otherwise returns the undef value.
dneto0 added a commit to google/clspv that referenced this pull request Dec 20, 2023
The spirv.Image target type no longer supports zeroinitializer value
(See llvm/llvm-project#73887)

Use undef instead.

Add a new clspv::GetDummyValue that returns the zeroinitializer value
when it can, and otherwise returns the undef value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants