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

GeorgeWeb · 2023-06-16T09:25:54Z

Without it set, the logic in BitcodeReader::parseTypeTableBody takes place and when targeting multiple backends (including spirv) would result in compilation errors.

Fixes a current issue with -c and multiple targets, where spir64 is set as the first target in the list.

maksimsab · 2023-06-16T11:42:01Z

Could you please add tests?

asudarsa · 2023-06-19T22:07:35Z

Adding @jcranmer-intel for review. Thanks

…typed and opaque pointers

GeorgeWeb · 2023-07-06T14:56:52Z

Could you please add tests?

Hey, Just came back off holidays.
Do you think the tests I've just pushed are okay and would suffice or you'd like me to test any more aspects?

TLDR; Pretty much needs to see if clang-offload-bundler works okay in the above explained scenario and allows mixing typed with opaque ptrs when the first input module contains typed ptrs.

jcranmer-intel

This is going to use the same LLVM context to parse all of the IR modules, so if any one of those modules uses opaque pointers, it's going to crash the offload.

Setting the context to unconditionally use opaque pointers is probably a better move, and rely on a separate LLVM context which autodetects for pointer mode for the SPIR-specific bitcode munging.

GeorgeWeb · 2023-07-07T10:05:38Z

This is going to use the same LLVM context to parse all of the IR modules, so if any one of those modules uses opaque pointers, it's going to crash the offload.

Setting the context to unconditionally use opaque pointers is probably a better move, and rely on a separate LLVM context which autodetects for pointer mode for the SPIR-specific bitcode munging.

@jcranmer-intel I understand the suggestion, thank you. But I still am not sure why that would be better and not saying it won't but I am trying to find an example that exhibits the problem you are referring to. Would it be possible clarify/elaborate more on this and/or share an example. Thanks again!

…que_ptrs_offload_bundler

GeorgeWeb · 2023-07-19T09:18:13Z

@jcranmer-intel Hi again. I was wondering if it's okay to elaborate a little more wrt requested changes. Thanks!

bader · 2023-08-15T15:20:29Z

@GeorgeWeb, please, fix the clang lit test failing in pre-commit.
You will need to update the branch as well.

…que_ptrs_offload_bundler

bader · 2023-08-16T17:30:40Z

@intel/dpcpp-tools-reviewers, @intel/dpcpp-clang-driver-reviewers, @jcranmer-intel, ping.

bader · 2023-08-16T18:29:53Z

clang/test/Driver/sycl-clang-offload-bundler-opaque-pointers.cpp

+/// reader will not fail because opaque pointers cannot be converted to typed pointers.
+///
+
+#include <sycl/sycl.hpp>


This test won't work unless SYCL project is built before.
There is much simpler way to get a device code - use SYCL_EXTERNAL attribute (see https://github.com/intel/llvm/blob/sycl/clang/test/Driver/sycl-bitfield-accesses.cpp as an example).

@GeorgeWeb, please, remove the dependency on DPC++ headers from clang tests.

Absolutely. I was on it earlier today, but I could not find a nice way due to:

// RUN: %clangxx -fsycl -target x86_64-unknown-linux-gnu %s -c -o %s.host.o

I need host.o as an input to clang-offload-bundler. I may need to rework the test to not be tied to DPC++/sycl at all as I get error: unknown type name 'SYCL_EXTERNAL' when I have to compile and link the object.

Do you have any suggestions? @bader Thanks!

__attribute__((sycl_device)) should work without dpcpp headers.

Suggestion from @sarnex should work.

You can also define the macro for the host

#ifndef __SYCL_DEVICE_ONLY__ #define SYCL_EXTERNAL #endif

On the other hand, it looks like the test is not specific to SYCL at all. You use SYCL mode to create inputs for clang-offload-bundler, but you can use regular C++ mode to create inputs as well. You just need a LLVM bitcode file and an object file. Am I right?
I think we can use the C code from https://github.com/intel/llvm/blob/sycl/clang/test/Driver/clang-offload-bundler.c, but instead of global variable, pass a pointer to the test_func to test opaque pointers.

bader · 2023-08-22T15:14:26Z

@jcranmer-intel, ping.

Thanks! My concern has been resolved.

bader · 2023-08-23T15:56:13Z

This is going to use the same LLVM context to parse all of the IR modules, so if any one of those modules uses opaque pointers, it's going to crash the offload.
Setting the context to unconditionally use opaque pointers is probably a better move, and rely on a separate LLVM context which autodetects for pointer mode for the SPIR-specific bitcode munging.

@jcranmer-intel I understand the suggestion, thank you. But I still am not sure why that would be better and not saying it won't but I am trying to find an example that exhibits the problem you are referring to. Would it be possible clarify/elaborate more on this and/or share an example. Thanks again!

@jcranmer-intel, your review is blocking this PR. Please, reply to @GeorgeWeb questions.

jcranmer-intel · 2023-08-29T18:40:03Z

clang/lib/Driver/OffloadBundler.cpp

+          const Triple TT = getTargetTriple(Target, BundlerConfig);
+          return !TT.isSPIR() && !TT.isSPIRV();
+        });
+    Context.setOpaquePointers(UseOpaquePointers);


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.

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?

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

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?

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).

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.

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"); }

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!

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

GeorgeWeb requested a review from a team as a code owner June 16, 2023 09:25

GeorgeWeb temporarily deployed to aws June 16, 2023 09:43 — with GitHub Actions Inactive

GeorgeWeb temporarily deployed to aws June 16, 2023 10:24 — with GitHub Actions Inactive

asudarsa requested a review from jcranmer-intel June 19, 2023 22:07

Georgi Mirazchiyski added 2 commits July 6, 2023 15:16

[SYCL] Set default value for opaque pointers in offload bundler

c7db9d6

Add tests to verify clang-offload-bundler bundle won't fail allowing …

c96918c

…typed and opaque pointers

GeorgeWeb force-pushed the georgi/sycl_fix_opaque_ptrs_offload_bundler branch from 299b443 to c96918c Compare July 6, 2023 14:54

GeorgeWeb requested a review from a team as a code owner July 6, 2023 14:54

GeorgeWeb temporarily deployed to aws July 6, 2023 15:22 — with GitHub Actions Inactive

GeorgeWeb temporarily deployed to aws July 6, 2023 17:23 — with GitHub Actions Inactive

jcranmer-intel requested changes Jul 6, 2023

View reviewed changes

Georgi Mirazchiyski added 2 commits July 7, 2023 14:12

Merge remote-tracking branch 'upstream/sycl' into georgi/sycl_fix_opa…

ec1f09e

…que_ptrs_offload_bundler

Update driver test.

69a3d82

GeorgeWeb temporarily deployed to aws July 7, 2023 13:30 — with GitHub Actions Inactive

GeorgeWeb temporarily deployed to aws July 7, 2023 15:24 — with GitHub Actions Inactive

Georgi Mirazchiyski added 2 commits July 14, 2023 14:47

Merge remote-tracking branch 'upstream/sycl' into georgi/sycl_fix_opa…

93fa364

…que_ptrs_offload_bundler

Update test.

6e4eb68

GeorgeWeb changed the title ~~[SYCL] Set default value for opaque pointers in offload bundler~~ [SYCL][Driver] Set default value for opaque pointers in offload bundler Aug 2, 2023

bader requested a review from jcranmer-intel August 15, 2023 15:18

Georgi Mirazchiyski added 4 commits August 15, 2023 17:36

Merge remote-tracking branch 'upstream/sycl' into georgi/sycl_fix_opa…

af2022e

…que_ptrs_offload_bundler

Merge remote-tracking branch 'upstream/sycl' into georgi/sycl_fix_opa…

ce453d3

…que_ptrs_offload_bundler

Try fixing the test.

0744ea6

Update test since spir targets output support opaque ptrs

52ccb85

mdtoguchi approved these changes Aug 16, 2023

View reviewed changes

bader previously requested changes Aug 16, 2023

View reviewed changes

Georgi Mirazchiyski added 2 commits August 18, 2023 12:36

Remove unsued commented code

6b85d26

[WIP] Fixup for simpler device code emitting.

1c7b777

Make the driver test independent of sycl mode.#

3fa4db3

bader changed the title ~~[SYCL][Driver] Set default value for opaque pointers in offload bundler~~ [Driver] Set default value for opaque pointers in offload bundler Aug 23, 2023

bader self-assigned this Aug 29, 2023

jcranmer-intel reviewed Aug 29, 2023

View reviewed changes

bader closed this Aug 30, 2023

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

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

Uh oh!

Conversation

GeorgeWeb commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maksimsab commented Jun 16, 2023

Uh oh!

asudarsa commented Jun 19, 2023

Uh oh!

GeorgeWeb commented Jul 6, 2023

Uh oh!

jcranmer-intel left a comment

Choose a reason for hiding this comment

Uh oh!

GeorgeWeb commented Jul 7, 2023

Uh oh!

GeorgeWeb commented Jul 19, 2023

Uh oh!

bader commented Aug 15, 2023

Uh oh!

bader commented Aug 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeorgeWeb Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarnex Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bader commented Aug 22, 2023

Uh oh!

bader commented Aug 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsji Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

GeorgeWeb commented Jun 16, 2023 •

edited

Loading

GeorgeWeb Aug 22, 2023 •

edited

Loading

sarnex Aug 22, 2023 •

edited

Loading

jsji Aug 29, 2023 •

edited

Loading