In D80932#2085848, @Anastasia wrote:

In D80932#2083185, @bader wrote:

In D80932#2080631, @Anastasia wrote:

@Anastasia, if we make this change specific to SYCL mode, will it address your concerns?

I can't answer this question for the reasons I have explained above.

Sorry, I'm not sure that I get your concern correctly, so let me clarify: is it allowing conversion between pointers w/ and w/o address space annotation in non-SYCL mode or using OpenCL address space attributes in SYCL mode?

Just to help you to understand the proposed design, I created the full patch for address space handling in SYCL: https://github.com/bader/llvm/pull/18. There are few CodeGen tests validating LLVM IR for SPIR target. Fee free to ask any questions on this PR.

Thanks! This is good but it is only an implementation of the design. Deducing the design from an implementation is time-consuming and not sure it is even feasible. I don't want to waste our time to provide you detailed feedback based on my interpretation of your design and invalid assumptions. I don't want to bind you to any particular format and we don't have any strict requirement for this in LLVM either, but I would encourage you to take a look at some of RFC threads sent to cfe-dev that explain new design concepts. Perhaps, they can help you to understand what information can be provided as a starting point to new design discussions.

Particularly I would suggest covering these two points:

• SYCL specifies address spaces as classes and it is not very obvious how did you come from libraries classes to modifications in Clang? I believe there are a number of design choices that you could make. One might think that you can just implement address space classes using existing Clang functionality. But if it is not sufficient it is important to understand what modifications are required and why.

• Other aspects that are important to highlight whether your design has any limitations. For example, I don't quite understand your need for SPIR*SYCLDeviceTargetInfo. Is there anything in your design that limits compilation to one particular target?

Overall, I see that there are a lot of changes in CodeGen that are related to the language semantic. I believe that CodeGen is supposed to be dialing primarily with target-specific logic and I don't know if you should change them to query target specific details instead. Also most of your CodeGen changes are not related to OpenCL so I would make sure to loop @rjmccall in this thread.

In D81641#2097433, @Fznamznon wrote:

Seems that test/OpenMP/nvptx_target_codegen.cpp is completely not formatted. If I apply suggestion from pre-merge checks, this will look like a big unrelated to this patch change and it will contradict with the whole file style.

In D80932#2080631, @Anastasia wrote:

@Anastasia, if we make this change specific to SYCL mode, will it address your concerns?

I can't answer this question for the reasons I have explained above.

In D80932#2074792, @Anastasia wrote:

I think my biggest problem is that I don't understand how you can just run C++ libraries on OpenCL devices. If we were able to compile plain C++ for OpenCL devices would we not just do it? But OpenCL devices have certain constraints. Also aren't libraries typically customized for performance depending on the range of HW they are being run on? I see the modifications are inevitable.

In D80932#2068863, @Anastasia wrote:

Fix formatting in clang/test/Sema/address_spaces.c

• Added C test case with address_space attribute.
• Move isAddressSpaceOverlapping from PointerType to QualType.
• Move C++ test case to clang/test/SemaCXX

Thanks for the review.
I've enabled the diagnostics for all the modes.
I also applied the refactoring suggested by @rjmccall. Hopefully I understand it correctly.

Enable diagnostics for non-OpenCL modes and applied refactoring proposed by John.

In D77220#1954909, @erichkeane wrote:

@bader : Is this what you meant? Feel free to add any reviewers you want for this. Note it is still dependent on D71016.

LGTM. Thanks!

LGTM. Thanks!

In D72857#1895649, @bader wrote:

In D72857#1895625, @thakis wrote:

This landed here: https://github.com/llvm/llvm-project/commit/bd97704eaaaab5a95ecb048ce343c1a4be5d94e5

It broke tests on mac: http://45.33.8.238/mac/9011/step_7.txt

Please take a look, and if it takes a while please revert while you investigate.

@thakis, thank for letting me know. I've reverted it at 740ed617f7d4d16e7883636c5eff994f8be7eba4. Sorry for inconvenience.

In D72857#1895625, @thakis wrote:

This landed here: https://github.com/llvm/llvm-project/commit/bd97704eaaaab5a95ecb048ce343c1a4be5d94e5

It broke tests on mac: http://45.33.8.238/mac/9011/step_7.txt

Please take a look, and if it takes a while please revert while you investigate.

Any other comments?

Rebase to ToT and address comments from Ronan.

Address comments from Victor and Alexey.

Ping.

Applied review commits from Alexey.

Applied code review comments.

Ignore -sycl-std if it used in non-SYCL mode.

Applied Alexey's comments.

Applied suggestions from Alexey and Ruyman and rebased on ToT.

Applied clang-format.

Refactor clang/lib/Driver/ToolChains/Clang.cpp in accordance with Alexey's suggestion.

Thanks for the prompt review.
I've applied your suggestions.

Applied suggestions from Alexey

Rebased on ToT.

Fix clang-format and clang-tidy issues reported by merge_guards_bot

Applied suggestion from Ruyman.

@lebedev.ri, thanks for the suggestion. We will investigate this option when we ready to upload clang diagnostics patch.

Thanks!

In D73651#1847862, @yaxunl wrote:

Added handling of SYCL kernels by Alexey's comments. I cannot add a codegen test for SYCL since I cannot find a way to instantiate a SYCL kernel.

Maybe we should use the year of issue (2015 instead of 1.2.1) for the -sycl-std version? That would be more stable for the upcoming SYCL versions, and match somehow the C++ versioning.

In D72362#1817682, @lebedev.ri wrote:

In D72362#1817599, @bader wrote:

Does it make sense to implement such diagnostics in clang Sema, considering that OpenCL does not allow recursion?
We implemented similar diagnostics for SYCL programming model and would be like to upstream it to clang later (https://github.com/intel/llvm/commit/4efe9fcf2dc6f6150b5b477b0f8320ea13a7f596). Can we somehow leverage this work for the compiler?

Implementing it elsewhere will be more restrictive in the future - somehow i suspect
it will be easier to make clang-tidy CTU-aware rather than clang sema.

That being said, is SYCL inherently single-TU, does it not support
linking multiple separately compiled object files together?

Does it make sense to implement such diagnostics in clang Sema, considering that OpenCL does not allow recursion?
We implemented similar diagnostics for SYCL programming model and would be like to upstream it to clang later (https://github.com/intel/llvm/commit/4efe9fcf2dc6f6150b5b477b0f8320ea13a7f596). Can we somehow leverage this work for the compiler?

LGTM, but I'd like someone who works on OpenCL front-end to approve.
+@AlexeySotkin

Added LIT test.

In D71286#1777894, @ABataev wrote:

Most probably, just missed the testing of the frontend part here

I'm looking for test suggestions as I was not able to find tests covering similar changes for OpenMP compiler.

@aaron.ballman, thank you for fixing the problem with documentation generation.

is there a reason this should be clang::opencl_private as opposed to opencl::private?

I'm okay with [[opencl::private]] as well. I have only one problem - currently OpenCL address spaces are exposed as keywords and using them in C++ breaks valid C++ code.

I'm not certain who controls the OpenCL spec, but this seems like it should be a decision that comes from there. Or is this functionality outside of the OpenCL spec?

In D71005#1770338, @Anastasia wrote:

LGTM! Thanks!

I presume OpenCL addr space logic won't apply in all cases in non-OpenCL compilations i.e. for example C++ because we enclose some of the logic under LangOpts checks.