Skip to content

[Driver] Set default value for opaque pointers in offload bundler #9933

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.

Sign up for GitHub

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

Closed
GeorgeWeb wants to merge 13 commits into from
Closed

[Driver] Set default value for opaque pointers in offload bundler #9933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c7db9d6
[SYCL] Set default value for opaque pointers in offload bundler
Jun 16, 2023
c96918c
Add tests to verify clang-offload-bundler bundle won't fail allowing …
Jul 6, 2023
ec1f09e
Merge remote-tracking branch 'upstream/sycl' into georgi/sycl_fix_opa…
Jul 7, 2023
69a3d82
Update driver test.
Jul 7, 2023
93fa364
Merge remote-tracking branch 'upstream/sycl' into georgi/sycl_fix_opa…
Jul 14, 2023
6e4eb68
Update test.
Jul 14, 2023
af2022e
Merge remote-tracking branch 'upstream/sycl' into georgi/sycl_fix_opa…
Aug 15, 2023
ce453d3
Merge remote-tracking branch 'upstream/sycl' into georgi/sycl_fix_opa…
Aug 16, 2023
0744ea6
Try fixing the test.
Aug 16, 2023
52ccb85
Update test since spir targets output support opaque ptrs
Aug 16, 2023
6b85d26
Remove unsued commented code
Aug 18, 2023
1c7b777
[WIP] Fixup for simpler device code emitting.
Aug 22, 2023
3fa4db3
Make the driver test independent of sycl mode.#
Aug 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions clang/lib/Driver/OffloadBundler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,18 @@ class ObjectFileHandler final : public FileHandler {
raw_svector_ostream SymbolsOS(SymbolsBuf);
LLVMContext Context;

// Set the default value for opaque pointers prior to parsing the bitcode.
#if !SPIRV_ENABLE_OPAQUE_POINTERS
const bool UseOpaquePointers =
llvm::any_of(BundlerConfig.TargetNames, [&](const auto &Target) {
const Triple TT = getTargetTriple(Target, BundlerConfig);
return !TT.isSPIR() && !TT.isSPIRV();
});
Context.setOpaquePointers(UseOpaquePointers);
Copy link
Contributor

@jcranmer-intel jcranmer-intel Aug 29, 2023

Choose a reason for hiding this comment

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

After poking around in more detail (I don't regularly develop this tool), I've come to the conclusion that this code is an elaborate way to set Context.setOpaquePointers(true). The host is included in TargetNames, and can't be SPIRV anyways, so there's always a non-SPIR-V module to cause it to be set to true. This of course obscures the failure modes of what would happen if UseOpaquePointers were false, since there's no realistic mode where it could be false.

Given that SPIRV_ENABLE_OPAQUE_POINTERS is enabled now and we're starting to remove support for typed pointers, I'm not sure there's any need for this PR anymore.

Copy link
Contributor

@bader bader Aug 29, 2023

Choose a reason for hiding this comment

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

Do you mean LLVMContext Context; uses opaque pointers by default and don't need to switch to non-opaque mode anymore?
@GeorgeWeb, are you okay to drop this change?

Copy link
Contributor

@jcranmer-intel jcranmer-intel Aug 29, 2023

Choose a reason for hiding this comment

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

#10888 means INTEL_SYCL_OPAQUEPOINTER_READY is now true and therefore everything is always in opaque pointer mode.

Copy link
Contributor

@bader bader Aug 29, 2023

Choose a reason for hiding this comment

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

Always?
Why do we add ifdef like this:

#ifndef INTEL_SYCL_OPAQUEPOINTER_READY
  if (!CLANG_ENABLE_OPAQUE_POINTERS_INTERNAL)
    CmdArgs.push_back("-no-opaque-pointers");
  else if ((Triple.isSPIRV() || Triple.isSPIR()) &&
           !SPIRV_ENABLE_OPAQUE_POINTERS)
    CmdArgs.push_back("-no-opaque-pointers");
#endif

?
Shouldn't we just remove this code?

Copy link
Contributor

@jcranmer-intel jcranmer-intel Aug 29, 2023

Choose a reason for hiding this comment

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

In principle, it's a cmake option you can turn off. However, I suspect it doesn't actually work anymore. And I also suspect @jsji is likely to start ripping the code out in the near future, after the code bakes for a little bit more (the PR landed only yesterday).

Copy link
Contributor

@jsji jsji Aug 29, 2023

Choose a reason for hiding this comment

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

Yes, the plan is to remove the code in #ifndef INTEL_SYCL_OPAQUEPOINTER_READY once our downstream code works well.
ETA 1-2 weeks.

Keep it by guarding it within macros just in case we need to compare to INTEL_SYCL_OPAQUEPOINTER_READY=0 in some case for now.

Copy link
Contributor

@jsji jsji Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

therefore everything is always in opaque pointer mode.

Yes. Also I believe setOpaquePointers is now an assert, so don't call it anymore.

void LLVMContext::setOpaquePointers(bool Enable) const {
  assert(Enable && "Cannot disable opaque pointers");
}

Copy link
Contributor Author

@GeorgeWeb GeorgeWeb Aug 30, 2023

Choose a reason for hiding this comment

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

I see! So, assuming INTEL_SYCL_OPAQUEPOINTER_READY goes away soon-ish then it's all gonna look cleaner. My patch was I guess really a temporary mitigation for an awkward issue and from the insights of this discussion - thanks @jsji and @jcranmer-intel I am happy to close this PR as it seems unneeded for intel:sycl. Thanks!

Copy link
Contributor Author

@GeorgeWeb GeorgeWeb Aug 30, 2023

Choose a reason for hiding this comment

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

@bader If you are convinced too, I am happy to close this given the future direction.

#else
Context.setOpaquePointers(true);
#endif

for (unsigned I = 0; I < NumberOfInputs; ++I) {
if (I == BundlerConfig.HostInputIndex)
continue;
Expand Down
44 changes: 44 additions & 0 deletions clang/test/Driver/sycl-clang-offload-bundler-opaque-pointers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
///
/// Checks that opaque-pointers are being set for llvm context used accordingly
/// to the bundle targets and clang-offload-bundler is able to read the bitcode.
///

// REQUIRES: x86-registered-target
// UNSUPPORTED: system-windows

void foo(int *p) {
*p = 1;
}

//
// Generate inputs for clang-offload-bundler.
//
// RUN: %clangxx -target openmp-x86_64-pc-linux-gnu -Xclang -no-opaque-pointers %s -S -emit-llvm -o %s.tgt1.ll
// RUN: %clangxx -target openmp-x86_64-pc-linux-gnu -Xclang -no-opaque-pointers %s -c -emit-llvm -o %s.tgt1.bc
// RUN: %clangxx -target openmp-nvptx64-nvidia-cuda %s -S -emit-llvm -o %s.tgt2.ll
// RUN: %clangxx -target openmp-nvptx64-nvidia-cuda %s -c -emit-llvm -o %s.tgt2.bc
// RUN: %clangxx -target x86_64-unknown-linux-gnu %s -c -o %s.host.o

//
// Check clang-offload-bundler obj for opaque pointers support error when typed
// pointer module is the first input target, followed by an opaque pointers one.
//
// RUN: clang-offload-bundler -type=o -targets=openmp-x86_64-pc-linux-gnu,openmp-nvptx64-nvidia-cuda,host-x86_64-unknown-linux-gnu -output=%s.bundle.o -input=%s.tgt1.bc -input=%s.tgt2.bc -input=%s.host.o 2>&1 \
// RUN: | FileCheck %s -check-prefix=CHECK-STD --allow-empty
// CHECK-STD-NOT: error: Opaque pointers are only supported in -opaque-pointers mode

//
// Verify the target 1 module contains typed pointers (set -no-opaque-pointers).
//
// RUN: FileCheck %s --check-prefix CHECK-TYPED_POINTERS_TARGET --input-file=%s.tgt1.ll
// CHECK-TYPED_POINTERS_TARGET: target triple = "x86_64-pc-linux-gnu-openmp"
// CHECK-TYPED_POINTERS_TARGET: i32* noundef
// CHECK-TYPED_POINTERS_TARGET-NOT: ptr noundef

//
// Verify the target 2 module contains opaque pointers.
//
// RUN: FileCheck %s --check-prefix CHECK-OPAQUE_POINTERS_TARGET --input-file=%s.tgt2.ll
// CHECK-OPAQUE_POINTERS_TARGET: target triple = "nvptx64-nvidia-cuda-openmp"
// CHECK-OPAQUE_POINTERS_TARGET: ptr noundef
// CHECK-OPAQUE_POINTERS_TARGET-NOT: i32* noundef