Skip to content

[LLVM][NFC] Use used's element type if available #116804

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 5 commits into from
Nov 20, 2024

Conversation

AlexVlx
Copy link
Contributor

@AlexVlx AlexVlx commented Nov 19, 2024

When embedding, if compiler.used exists, we should re-use it's element type instead of blindly assuming it's an unqualified pointer.

@pmatos
Copy link
Contributor

pmatos commented Nov 19, 2024

I haven't touched LLVM source code for almost a year now and will defer review to others more active atm.

@jvoung
Copy link
Contributor

jvoung commented Nov 20, 2024

Mind adding a test case? Perhaps something like the embed-bitcode.ll, but instead checking the round-tripping of compiler.used ?

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Nov 20, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 20, 2024

@llvm/pr-subscribers-clang

Author: Alex Voicu (AlexVlx)

Changes

When embedding, if compiler.used exists, we should re-use it's element type instead of blindly assuming it's an unqualified pointer.


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

2 Files Affected:

  • (added) clang/test/CodeGen/embed-bitcode-marker-with-nonzero-as.c (+8)
  • (modified) llvm/lib/Bitcode/Writer/BitcodeWriter.cpp (+2-1)
diff --git a/clang/test/CodeGen/embed-bitcode-marker-with-nonzero-as.c b/clang/test/CodeGen/embed-bitcode-marker-with-nonzero-as.c
new file mode 100644
index 00000000000000..5cd0c74a03c200
--- /dev/null
+++ b/clang/test/CodeGen/embed-bitcode-marker-with-nonzero-as.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple spirv64-amd-amdhsa -emit-llvm -fcuda-is-device -fembed-bitcode=marker -x hip %s -o - \
+// RUN:   | FileCheck %s --check-prefix=CHECK
+
+// CHECK: @llvm.embedded.module = private addrspace(1) constant [0 x i8] zeroinitializer, section ".llvmbc", align 1
+// CHECK-NEXT: @llvm.cmdline = private addrspace(1) constant [387 x i8] c"{{.*}}", section ".llvmcmd", align 1
+// CHECK-NEXT: @llvm.compiler.used = appending addrspace(1) global [5 x ptr addrspace(4)] [ptr addrspace(4) addrspacecast (ptr addrspace(1) @foo.managed to ptr addrspace(4)), ptr addrspace(4) addrspacecast (ptr addrspace(1) @foo to ptr addrspace(4)), ptr addrspace(4) addrspacecast (ptr addrspace(1) @__hip_cuid_ to ptr addrspace(4)), ptr addrspace(4) addrspacecast (ptr addrspace(1) @llvm.embedded.module to ptr addrspace(4)), ptr addrspace(4) addrspacecast (ptr addrspace(1) @llvm.cmdline to ptr addrspace(4))], section "llvm.metadata"
+
+__attribute__((managed)) int foo = 42;
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index 24a4c2e8303d5a..80e12bef502ace 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -5501,8 +5501,9 @@ void llvm::embedBitcodeInModule(llvm::Module &M, llvm::MemoryBufferRef Buf,
   // Save llvm.compiler.used and remove it.
   SmallVector<Constant *, 2> UsedArray;
   SmallVector<GlobalValue *, 4> UsedGlobals;
-  Type *UsedElementType = PointerType::getUnqual(M.getContext());
   GlobalVariable *Used = collectUsedGlobalVariables(M, UsedGlobals, true);
+  Type *UsedElementType = Used ? Used->getValueType()->getArrayElementType()
+                               : PointerType::getUnqual(M.getContext());
   for (auto *GV : UsedGlobals) {
     if (GV->getName() != "llvm.embedded.module" &&
         GV->getName() != "llvm.cmdline")

Copy link
Contributor Author

@AlexVlx AlexVlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind adding a test case? Perhaps something like the embed-bitcode.ll, but instead checking the round-tripping of compiler.used ?

Done, although slightly differently; it does reflect that when there is a pre-existing compiler.used its element type is now retained.

@AlexVlx AlexVlx merged commit 53a6a11 into llvm:main Nov 20, 2024
8 checks passed
@AlexVlx AlexVlx deleted the bitcode_writer_should_be_defensive branch November 20, 2024 23:58
searlmc1 added a commit to ROCm/llvm-project that referenced this pull request Dec 5, 2024
Adds the following patches
AMDGPU: Remove wavefrontsize64 feature from dummy target llvm#117410
[LLVM][NFC] Use used's element type if available llvm#116804
[llvm][AMDGPU] Fold llvm.amdgcn.wavefrontsize early llvm#114481
[clang][Driver][HIP] Add support for mixing AMDGCNSPIRV & concrete offload-archs. llvm#113509
[clang][llvm][SPIR-V] Explicitly encode native integer widths for SPIR-V llvm#110695
[llvm][opt][Transforms] Replacement calloc should match replaced malloc llvm#110524
[clang][HIP] Don't use the OpenCLKernel CC when targeting AMDGCNSPIRV llvm#110447
[cuda][HIP] constant should imply constant llvm#110182
[llvm][SPIRV] Expose fast popcnt support for SPIR-V targets llvm#109845
[clang][CodeGen][SPIR-V] Fix incorrect SYCL usage, implement missing interface llvm#109415
[SPIRV][RFC] Rework / extend support for memory scopes llvm#106429
[clang][CodeGen][SPIR-V][AMDGPU] Tweak AMDGCNSPIRV ABI to allow for the correct handling of aggregates passed to kernels / functions. llvm#102776

Change-Id: I2b9ab54aba1c9345b9b0eb84409e6ed6c3cdb6cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category llvm:bitcode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants