[SYCL] Emit integration footer and header for device_global variables

[Fznamznon]

The implementation is based on the design doc
https://github.com/intel/llvm/blob/sycl/sycl/doc/design/DeviceGlobal.md

[Fznamznon]

@erichkeane I'd appreciate review from you as well, since it seems that no one have never made any changes to integration footer except you.

[Fznamznon]

Review request for runtime seems like a bug.

[bader]

Review request for runtime seems like a bug.

# DPC++ runtime library
sycl/ @intel/llvm-reviewers-runtime

This rule matches any directory named "sycl" i.e. it's not necessary must be a root directory.
clang/test/CodeGenSYCL/Inputs/CL/sycl/detail/defines_elementary.hpp - matches this rule.

[erichkeane]

Seems alright to me!

[Fznamznon]

Review request for runtime seems like a bug.

# DPC++ runtime library
sycl/ @intel/llvm-reviewers-runtime

This rule matches any directory named "sycl" i.e. it's not necessary must be a root directory. clang/test/CodeGenSYCL/Inputs/CL/sycl/detail/defines_elementary.hpp - matches this rule.

Ok, thanks!

[Fznamznon]

Ping @intel/dpcpp-cfe-reviewers for review.

[premanandrao]

Some minor nits, but looks good to me.

[elizabethandrews]

What happens when we capture a device_global variable with regard to openCL kernel generation. We decompose the type?

[Fznamznon]

What happens when we capture a device_global variable with regard to openCL kernel generation. We decompose the type?

My understanding was that device_global variables won't be captured, because lambdas don't capture variables with static storage duration. device_global variables can be declared only at namespace scope or as a static member variable, but only if the member variable is publicly accessible from namespace scope. So device_global variables will have static storage duration.

[bader]

Please, remove clang/test/CodeGenSYCL/Inputs/CL/sycl/detail/ directly and move all the files from this directory to clang/test/CodeGenSYCL/Inputs. This will also resolve the problem with code owners.

Without this directory I can't compile integration header and footer from clang tests. Do you suggest to remove this kind of testing?

Are you saying that we hardcoded DPC++ runtime library headers directory path in clang sources? If so, it looks like a bug to me. I don't think clang must rely on header file locations and especially for "detail" functionality, which is not documented in the specification.

Yes, paths to some headers are hardcoded in emitted integration header and footer.

Okay.

Do you suggest to remove this kind of testing?

Yes. I think checking the content of generated integration header/footer should be enough. Compilation of generated files is already covered by check-sycl test. Right?

[Fznamznon]

Compilation of generated files is already covered by check-sycl test. Right?

I think this is not yet covered. I can't find declaration of device_global class in SYCL headers and it's usage in sycl/test directory, that means new content is not generated in check-sycl tests now and therefore compilation is not checked there. I assume that it should happen in the future when headers implement device_global though.

[bader]

Compilation of generated files is already covered by check-sycl test. Right?

I think this is not yet covered. I can't find declaration of device_global class in SYCL headers and it's usage in sycl/test directory, that means new content is not generated in check-sycl tests now and therefore compilation is not checked there. I assume that it should happen in the future when headers implement device_global though.

Anyway, the test suite for testing DPC++ headers is check-sycl, not check-clang.

[bader]

Please, remove clang/test/CodeGenSYCL/Inputs/CL/sycl/detail/ directly and move all the files from this directory to clang/test/CodeGenSYCL/Inputs. This will also resolve the problem with code owners.

Without this directory I can't compile integration header and footer from clang tests. Do you suggest to remove this kind of testing?

Are you saying that we hardcoded DPC++ runtime library headers directory path in clang sources? If so, it looks like a bug to me. I don't think clang must rely on header file locations and especially for "detail" functionality, which is not documented in the specification.

Yes, paths to some headers are hardcoded in emitted integration header and footer. It's been there for a long time, see -

llvm/clang/lib/Sema/SemaSYCL.cpp

Line 4626 in 0cff80e

O << "#include <CL/sycl/detail/defines_elementary.hpp>\n";

.

I can file a bug, but this is not a problem of this concrete change.

I think we should do this. It looks like a design issue to me. We better avoid dependencies like this in the compiler.

BTW, it looks like it should be easy to do. https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/detail/defines_elementary.hpp defines some number of macro. I think we should just expand all of them in emitted integration header and remove this include.

O << "#include <CL/sycl/detail/defines_elementary.hpp>\n";

[Fznamznon]

Please, remove clang/test/CodeGenSYCL/Inputs/CL/sycl/detail/ directly and move all the files from this directory to clang/test/CodeGenSYCL/Inputs. This will also resolve the problem with code owners.

Without this directory I can't compile integration header and footer from clang tests. Do you suggest to remove this kind of testing?

Are you saying that we hardcoded DPC++ runtime library headers directory path in clang sources? If so, it looks like a bug to me. I don't think clang must rely on header file locations and especially for "detail" functionality, which is not documented in the specification.

Yes, paths to some headers are hardcoded in emitted integration header and footer. It's been there for a long time, see -

llvm/clang/lib/Sema/SemaSYCL.cpp

Line 4626 in 0cff80e

O << "#include <CL/sycl/detail/defines_elementary.hpp>\n";

.

I can file a bug, but this is not a problem of this concrete change.

I think we should do this. It looks like a design issue to me. We better avoid dependencies like this in the compiler.

BTW, it looks like it should be easy to do. https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/detail/defines_elementary.hpp defines some number of macro. I think we should just expand all of them in emitted integration header and remove this include.

O << "#include <CL/sycl/detail/defines_elementary.hpp>\n";

It is just one of them. I think it is not that simple for others. I've submitted an internal tracker.

[Fznamznon]

I don't think I broke L0 GEN9 LLVM Test Suite (pull_request) this time either. The latest commit is different from previous one (which passed this check) only by changes in clang LIT tests. Some part of the tests are failing on sycl-ls command.

[premanandrao]

@Fznamznon, I have a concern with the removal of footer and header compilation tests on host. I think it leaves a hole in our testing, for example, if there is a typo or bad code generated in the header and footer, we won't notice it. If there are tests elsewhere that would catch this, that would be sufficient, but without one we have a gap in coverage.

This happened with a recent PR, accidental missing quotes slipped through on first review.

[Fznamznon]

I have a concern with the removal of footer and header compilation tests on host. I think it leaves a hole in our testing, for example, if there is a typo or bad code generated in the header and footer, we won't notice it.

Yeah, I had the same thoughts when did compilation testing. I didn't believe to my eyes and my c++ knowledge enough, so I added compiler's check. In fact, I even caught missing conversion to void * in integration footer emission. But if there were tests in check-sycl with device_global, they would have caught it.

If there are tests elsewhere that would catch this, that would be sufficient, but without one we have a gap in coverage.

As @bader pointed out, check-sycl suite is supposed to perform host compilation and catch this. I'm ok with that.

[bader]

I don't think I broke L0 GEN9 LLVM Test Suite (pull_request) this time either. The latest commit is different from previous one (which passed this check) only by changes in clang LIT tests. Some part of the tests are failing on sycl-ls command.

@Fznamznon, could you create a tracker to investigate this problem? I suggest re-running the tests to check if this issue is sporadic.

[premanandrao]

As @bader pointed out, check-sycl suite is supposed to perform host compilation and catch this. I'm ok with that.

Sounds good.

[Fznamznon]

@Fznamznon, could you create a tracker to investigate this problem?

@bader , @alexbatashev , this is an issue with GitHub actions. Should I create a GitHub issue or an internal tracker?
BTW, #5592 also experiences the same problem.

[alexbatashev]

@Fznamznon, could you create a tracker to investigate this problem?

@bader , @alexbatashev , this is an issue with GitHub actions. Should I create a GitHub issue or an internal tracker? BTW, #5592 also experiences the same problem.

I don't see it as a GitHub Actions problem. I see that a few tests have failed for unrelated changes. Please, create internal tracker for those issues and disable those tests.

[Fznamznon]

I don't see it as a GitHub Actions problem.

Sorry, my bad.

Please, create internal tracker for those issues and disable those tests.

Done. Created intel/llvm-test-suite#886 .
It seems all recent PRs are affected by this fail.