[PATCH] D151361: [CUDA] bump supported CUDA version to 12.1/11.8

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:590
 
+- Clang now supports CUDA SDK up to 12.1
 

tra wrote:
> bader wrote:
> > @tra, could you update llvm/docs/CompileCudaWithLLVM.rst as well, please?
> Done in  d028188412fa54774e2c60e21f0929a0fede93bb
Great. Thank you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151361/new/

https://reviews.llvm.org/D151361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151361: [CUDA] bump supported CUDA version to 12.1/11.8

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:590
 
+- Clang now supports CUDA SDK up to 12.1
 

@tra, could you update llvm/docs/CompileCudaWithLLVM.rst as well, please?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151361/new/

https://reviews.llvm.org/D151361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147097: [SYCL] Always set NoUnwind attribute for SYCL.

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147097/new/

https://reviews.llvm.org/D147097

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147097: [SYCL] Always set NoUnwind attribute for SYCL.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

@hvdijk, thanks a lot for fixing this.

In D147097#4229121 , @hvdijk wrote:

> Is the rationale I gave in the description correct, or would it be better for 
> SYCL device code to unconditionally build without `-fexceptions` and get the 
> `nounwind` attribute added that way?

That's a good question. I haven't looked into this issue deep enough, but I 
think using `-fexceptions` requires using delayed diagnostics to avoid false 
diagnostics during host code analysis. 
Anyway, all GPU offloading single-source modes have the same restriction and 
design and we better have unified solution whether it's using `-fexceptions` or 
adding `nounwind` attribute in CodeGen.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147097/new/

https://reviews.llvm.org/D147097

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129507: [OffloadPackager] Add option to extract files from images

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/test/Driver/offload-packager.c:2-3
+// REQUIRES: x86-registered-target
+// REQUIRES: nvptx-registered-target
+// REQUIRES: amdgpu-registered-target
+// UNSUPPORTED: system-windows

Are nvptx and amdgpu target required for this test?
Latest version of the test invokes clang only for x86 target and 
clang-offload-packager just adds triple as metadata string without using llvm 
target. Right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129507/new/

https://reviews.llvm.org/D129507

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142583: [SPIR] Add support for __arithmetic_fence builtin for SPIR target.

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142583/new/

https://reviews.llvm.org/D142583

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142583: [SPIR] Add support for __arithmetic_fence builtin for SPIR target.

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/test/CodeGen/arithmetic-fence-builtin.c:73
 int subit(float a, float b, float *fp) {
-  // CHECKFAST: define {{.*}}@subit(float noundef %a, float noundef %b{{.*}}
+  // CHECKPRECISE: define {{.*}}@subit(float noundef %a, float noundef %b{{.*}}
   *fp = __arithmetic_fence(a - b);

Why is this check removed for SPIR target?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142583/new/

https://reviews.llvm.org/D142583

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142583: [SPIR-V] Add support for __arithmetic_fence builtin for SYCL targets.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

"[SPIR-V] Add support for __arithmetic_fence builtin for SYCL targets." -> 
"[SPIR] Add support for __arithmetic_fence builtin for SPIR target."




Comment at: clang/test/CodeGen/arithmetic-fence-builtin.c:16
+// Test with fast math on spir target
+// RUN: %clang_cc1 -triple spir64  -emit-llvm -fsycl-is-device \
+// RUN: -mreassociate -o - %s \





Comment at: clang/test/CodeGen/arithmetic-fence-builtin.c:74
 int subit(float a, float b, float *fp) {
-  // CHECKFAST: define {{.*}}@subit(float noundef %a, float noundef %b{{.*}}
+  // CHECKPRECISE: define {{.*}}@subit(float noundef %a, float noundef %b{{.*}}
   *fp = __arithmetic_fence(a - b);

What is different for SPIR target here?



Comment at: clang/test/Sema/arithmetic-fence-builtin.c:5
 // RUN:-fprotect-parens 2>&1 | FileCheck -check-prefix=PPC %s
+// RUN: %clang_cc1 -triple spir64  -emit-llvm -fsycl-is-device \
+// RUN: -o - -verify -x c++ %s




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142583/new/

https://reviews.llvm.org/D142583

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142033: [OpenCL] Always add nounwind attribute for OpenCL

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

Should we generalize and rename `clang/test/CodeGenOpenCL/convergent.cl` to 
validate function attributes other than `convergent`? It's not obvious that 
presence of `nounwind` attribute is validated by 
`clang/test/CodeGenOpenCL/convergent.cl`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142033/new/

https://reviews.llvm.org/D142033

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141375: [SYCL][OpenMP] Fix compilation errors for unsupported __bf16 intrinsics

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM.

I expect this to be a common issue for all single-source offloading programming 
models (i.e. CUDA and HIP in addition to SYCL and OpenMP offload). Probably we 
can generalize the code patterns used in this patch for all of them.

In addition to that, there are other built-in data types not supported either 
by host or device, which are handled similar way. Right?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141375/new/

https://reviews.llvm.org/D141375

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141008: [Clang][SPIR-V] Emit target extension types for OpenCL types on SPIR-V.

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/CodeGen/CGOpenCLRuntime.cpp:40
+static llvm::Type *getSPIRVType(llvm::LLVMContext &Ctx, StringRef BaseType,
+StringRef OpenCLName, StringRef ReadSuffix) {
+  SmallVector IntParams = {0, 0, 0, 0, 0, 0};

jcranmer-intel wrote:
> bader wrote:
> > I believe this can be done at "compile time" (i.e. during the clang build, 
> > not clang run).
> > Can we have a pre-computed map from an OpenCL built-in type to a SPIR-V 
> > type?
> > Another option is compile-time evaluated function. This should be possible, 
> > right?
> > 
> > If I get it right, here we take a string representation of an OpenCL image 
> > type and process it at runtime, which seems to be unnecessary as we have 
> > pre-defined (by the spec) set of the types.
> I can definitely switch the read suffix to use a compile-time enum, since 
> there are only 3 cases (plus, it's a static assert). Making the openCL name 
> to int param conversion be a compile-time constant might be doable with some 
> tricks, but I'll have to think about it for a little bit. It's a little 
> harder because we're taking a string to 6 array values.
I was going to suggest ripping of https://reviews.llvm.org/D108034, but it 
looks like it produces types which have OpenCL names with __spirv_* prefix. So 
unfortunately, I don't have a good example. The only thing coming to my mind is 
to build another table with SPIR-V type names, which can be obtained via OpenCL 
type id (offset?).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141008/new/

https://reviews.llvm.org/D141008

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141008: [Clang][SPIR-V] Emit target extension types for OpenCL types on SPIR-V.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

@jcranmer-intel, thanks a lot for working on this. I'm so excited to see these 
changes!
Overall, it looks good to me, but I'd like to avoid some runtime computations 
if possible.




Comment at: clang/lib/CodeGen/CGOpenCLRuntime.cpp:40
+static llvm::Type *getSPIRVType(llvm::LLVMContext &Ctx, StringRef BaseType,
+StringRef OpenCLName, StringRef ReadSuffix) {
+  SmallVector IntParams = {0, 0, 0, 0, 0, 0};

I believe this can be done at "compile time" (i.e. during the clang build, not 
clang run).
Can we have a pre-computed map from an OpenCL built-in type to a SPIR-V type?
Another option is compile-time evaluated function. This should be possible, 
right?

If I get it right, here we take a string representation of an OpenCL image type 
and process it at runtime, which seems to be unnecessary as we have pre-defined 
(by the spec) set of the types.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141008/new/

https://reviews.llvm.org/D141008

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138284: Fix incorrect cast in VisitSYCLUniqueStableNameExpr

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

Thanks for the fix!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138284/new/

https://reviews.llvm.org/D138284

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138284: Fix incorrect cast in VisitSYCLUniqueStableNameExpr

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1635
+  Context.getTargetInfo().getConstantAddressSpace().value_or(
+  LangAS::Default));
   llvm::Constant *GlobalConstStr = Builder.CreateGlobalStringPtr(

arichardson wrote:
> arichardson wrote:
> > bader wrote:
> > > > This changes the code generation for spir64 to place the globals in 
> > > > addrspace(4). I believe is correct, but it would be good for someone 
> > > > who is familiar with the target to confirm.
> > > 
> > > Globals must reside in `sycl_global` namespace, which is `addrspace(1)` 
> > > for spir* targets.
> > > `addrspace(4)` represents "generic" address space, which is a placeholder 
> > > for a specific address space. If we leave it `addrspace(4)` for global 
> > > definition, the compiler won't be able to infer genuine address space.
> > Okay that's interesting - I guess it means we should not be using 
> > `getConstantAddressSpace()` here? Or getConstantAddressSpace() should 
> > actually return a value that maps to `addrspace(1)`?
> Ah it looks like we should be using 
> `CodeGenModule::GetGlobalConstantAddressSpace()` instead of 
> `getTarget().getConstantAddressSpace()`. Is that correct?
> 
> 
> ```
> LangAS CodeGenModule::GetGlobalConstantAddressSpace() const {
>   // OpenCL v1.2 s6.5.3: a string literal is in the constant address space.
>   if (LangOpts.OpenCL)
> return LangAS::opencl_constant;
>   if (LangOpts.SYCLIsDevice)
> return LangAS::sycl_global;
>   if (LangOpts.HIP && LangOpts.CUDAIsDevice && getTriple().isSPIRV())
> // For HIPSPV map literals to cuda_device (maps to CrossWorkGroup in 
> SPIR-V)
> // instead of default AS (maps to Generic in SPIR-V). Otherwise, we end up
> // with OpVariable instructions with Generic storage class which is not
> // allowed (SPIR-V V1.6 s3.42.8). Also, mapping literals to SPIR-V
> // UniformConstant storage class is not viable as pointers to it may not 
> be
> // casted to Generic pointers which are used to model HIP's "flat" 
> pointers.
> return LangAS::cuda_device;
>   if (auto AS = getTarget().getConstantAddressSpace())
> return *AS;
>   return LangAS::Default;
> }
> ```
> 
> Another problem appears to be that the default implementation of 
> getConstantAddressSpace() returns `LangAS::Default` instead of None, so the 
> .value_or() will never be used.
> Ah it looks like we should be using 
> CodeGenModule::GetGlobalConstantAddressSpace() instead of 
> getTarget().getConstantAddressSpace(). Is that correct?

Yes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138284/new/

https://reviews.llvm.org/D138284

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138284: Fix incorrect cast in VisitSYCLUniqueStableNameExpr

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1635
+  Context.getTargetInfo().getConstantAddressSpace().value_or(
+  LangAS::Default));
   llvm::Constant *GlobalConstStr = Builder.CreateGlobalStringPtr(

> This changes the code generation for spir64 to place the globals in 
> addrspace(4). I believe is correct, but it would be good for someone who is 
> familiar with the target to confirm.

Globals must reside in `sycl_global` namespace, which is `addrspace(1)` for 
spir* targets.
`addrspace(4)` represents "generic" address space, which is a placeholder for a 
specific address space. If we leave it `addrspace(4)` for global definition, 
the compiler won't be able to infer genuine address space.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138284/new/

https://reviews.llvm.org/D138284

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137154: Adding nvvm_reflect clang builtin

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

Is binary size a concern here? NVIDIA, AMD and Intel GPUs are already have ~ 20 
different architectures each, so I want my app/library to run on any GPU from 
these vendors (which is quite reasonable expectation), I'll need to 
have/distribute ~ 60 different binaries. libdevice, libm, libc are quite small, 
but other apps (e.g. ML frameworks) might be quite large, so that distributed 
binary size is important to consider.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137154/new/

https://reviews.llvm.org/D137154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136160: [Attr][Doc] Fix pragma unroll documentation.

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG66bd6074c133: [Attr][Doc] Fix pragma unroll documentation. 
(authored by bader).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136160/new/

https://reviews.llvm.org/D136160

Files:
  clang/include/clang/Basic/AttrDocs.td


Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -3569,7 +3569,7 @@
   }
 
 ``#pragma unroll`` and ``#pragma unroll _value_`` have identical semantics to
-``#pragma clang loop unroll(full)`` and
+``#pragma clang loop unroll(enable)`` and
 ``#pragma clang loop unroll_count(_value_)`` respectively. ``#pragma nounroll``
 is equivalent to ``#pragma clang loop unroll(disable)``. See
 `language extensions


Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -3569,7 +3569,7 @@
   }
 
 ``#pragma unroll`` and ``#pragma unroll _value_`` have identical semantics to
-``#pragma clang loop unroll(full)`` and
+``#pragma clang loop unroll(enable)`` and
 ``#pragma clang loop unroll_count(_value_)`` respectively. ``#pragma nounroll``
 is equivalent to ``#pragma clang loop unroll(disable)``. See
 `language extensions
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136160: [Attr][Doc] Fix pragma unroll documentation.

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 468487.
bader added a comment.

Update commit message.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136160/new/

https://reviews.llvm.org/D136160

Files:
  clang/include/clang/Basic/AttrDocs.td


Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -3569,7 +3569,7 @@
   }
 
 ``#pragma unroll`` and ``#pragma unroll _value_`` have identical semantics to
-``#pragma clang loop unroll(full)`` and
+``#pragma clang loop unroll(enable)`` and
 ``#pragma clang loop unroll_count(_value_)`` respectively. ``#pragma nounroll``
 is equivalent to ``#pragma clang loop unroll(disable)``. See
 `language extensions


Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -3569,7 +3569,7 @@
   }
 
 ``#pragma unroll`` and ``#pragma unroll _value_`` have identical semantics to
-``#pragma clang loop unroll(full)`` and
+``#pragma clang loop unroll(enable)`` and
 ``#pragma clang loop unroll_count(_value_)`` respectively. ``#pragma nounroll``
 is equivalent to ``#pragma clang loop unroll(disable)``. See
 `language extensions
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136160: [Attr][Doc] Fix pragma unroll documentation.

[Alexey Bader via Phabricator via cfe-commits]

bader created this revision.
bader added a reviewer: aaron.ballman.
Herald added a subscriber: ebevhan.
Herald added a project: All.
bader requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

There is a contradiction in the #pragma unroll behavior documentation.
It says that specifying `#pragma unroll` without a parameter directs the
loop unroller to attempt to partially unroll the loop if the trip count
is not known at compile time. At the same time later it states that
`#pragma unroll` has identical semantics to `#pragma clang loop
unroll(full)`, which doesn't attempt to unroll partially if the trip
count is not known at compile time.

If unroll(enable) is specified the unroller will attempt to fully unroll the 
loop if the trip count is known at compile time. If the fully unrolled code 
size is greater than an internal limit the loop will be partially unrolled up 
to this limit. If the trip count is not known at compile time the loop will be 
partially unrolled with a heuristically chosen unroll factor.

If unroll(full) is specified the unroller will attempt to fully unroll the loop 
if the trip count is known at compile time identically to unroll(enable). 
However, with unroll(full) the loop will not be unrolled if the loop count is 
not known at compile time.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136160

Files:
  clang/include/clang/Basic/AttrDocs.td


Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -3569,7 +3569,7 @@
   }
 
 ``#pragma unroll`` and ``#pragma unroll _value_`` have identical semantics to
-``#pragma clang loop unroll(full)`` and
+``#pragma clang loop unroll(enable)`` and
 ``#pragma clang loop unroll_count(_value_)`` respectively. ``#pragma nounroll``
 is equivalent to ``#pragma clang loop unroll(disable)``. See
 `language extensions


Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -3569,7 +3569,7 @@
   }
 
 ``#pragma unroll`` and ``#pragma unroll _value_`` have identical semantics to
-``#pragma clang loop unroll(full)`` and
+``#pragma clang loop unroll(enable)`` and
 ``#pragma clang loop unroll_count(_value_)`` respectively. ``#pragma nounroll``
 is equivalent to ``#pragma clang loop unroll(disable)``. See
 `language extensions
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116266: [SPIR-V] Add linking of separate translation units using spirv-link

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/docs/UsersManual.rst:3602
 
+Linking is done using ``spirv-link`` from `the SPIRV-Tools project
+`_. Similar to other 
external

Anastasia wrote:
> bader wrote:
> > @Anastasia, sorry for late feedback.
> > I think being able to link SPIR-V modules is a great feature, but I have a 
> > concerns regarding `spirv-link` tool.
> > The documentation says that the linker tool is still under development and 
> > from our experience this tool had issues blocking us from using it for SYCL 
> > mode. The last time new features were added to this tool is almost 4 year 
> > ago.
> > Do you know if there are any plans for to finish the development and if ? 
> > Are you aware of any "real-world usages" of this tool? Have you tried to 
> > use it for SPIR-V module produced from C++ (e.g. C++ for OpenCL)?
> > I think supporting SPIR-V extensions like [[ 
> > https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/KHR/SPV_KHR_linkonce_odr.asciidoc
> >  | SPV_KHR_linkonce_odr ]] is quite important for code size and JIT 
> > compilation time reduction. As this extension was ratified recently, I 
> > suppose `spirv-link` doesn't support it yet.
> Hi Alexey,
> 
> Sorry for the late reply. Do you have any other suggestions about the tools 
> that can be used for linking SPIR-V binaries? 
> 
> I am not in contact with the maintainers but it is an open-source project so 
> I imagine contributions to enhance or improve functionality should be 
> welcome... unless you have other experiences?
> 
> Do you have any other suggestions about the tools that can be used for 
> linking SPIR-V binaries?

I'm unaware of other tools for SPIR-V binaries linking. To link SPIR-V binaries 
in our toolchain, we translate them to/from LLVM IR to link LLVM IR.

> I am not in contact with the maintainers but it is an open-source project so 
> I imagine contributions to enhance or improve functionality should be 
> welcome... unless you have other experiences?

I talked to the maintainers (but it was quite long time ago) and they told me 
that there are no active contributors to this tool.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116266/new/

https://reviews.llvm.org/D116266

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127579: [clang][WIP] add option to keep types of ptr args for non-kernel functions in metadata

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

> The way I understand a bitcast instruction in SPIR-V (`OpBitcast` in 
> https://www.khronos.org/registry/SPIR-V/specs/unified1/SPIRV.html#_conversion_instructions)
>  is that it can only apply to pointer types which are distinct from function 
> types. Note that I believe that function pointers are illegal, at least we 
> disallow them in OpenCL.

FYI: we are experimenting with function pointers on Intel HW programmed via 
SPIR-V. Extension draft - 
https://github.com/intel/llvm/blob/sycl/sycl/doc/design/spirv-extensions/SPV_INTEL_function_pointers.asciidoc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127579/new/

https://reviews.llvm.org/D127579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127579: [clang][WIP] add option to keep types of ptr args for non-kernel functions in metadata

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D127579#3585516 , @beanz wrote:

> @nikic the most important thing you need to know about SPIR-V is that it is a 
> virtual ISA based on LLVM IR. The ISA itself encodes types for pointers just 
> like LLVM IR would.

And in addition to that ISA defines types, which are not natively supported by 
LLVM IR e.g. image. To represent those types clang in OpenCL language mode 
emits a pointer to an opaque structure with special name like 
opencl. (e.g. opencl.image2d_t). All ISA types, which are 
defined that way look the same with type-less pointers.
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/OpenCLImageTypes.def


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127579/new/

https://reviews.llvm.org/D127579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122587: [clang][NFC] Extract the EmitAssemblyHelper::TargetTriple member

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG87b28f5092f2: [clang][NFC] Extract the 
EmitAssemblyHelper::TargetTriple member (authored by psamolysov-intel, 
committed by bader).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122587/new/

https://reviews.llvm.org/D122587

Files:
  clang/lib/CodeGen/BackendUtil.cpp


Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -118,6 +118,8 @@
 
   std::unique_ptr OS;
 
+  Triple TargetTriple;
+
   TargetIRAnalysis getTargetIRAnalysis() const {
 if (TM)
   return TM->getTargetIRAnalysis();
@@ -170,7 +172,8 @@
  const LangOptions &LOpts, Module *M)
   : Diags(_Diags), HSOpts(HeaderSearchOpts), CodeGenOpts(CGOpts),
 TargetOpts(TOpts), LangOpts(LOpts), TheModule(M),
-CodeGenerationTime("codegen", "Code Generation Time") {}
+CodeGenerationTime("codegen", "Code Generation Time"),
+TargetTriple(TheModule->getTargetTriple()) {}
 
   ~EmitAssemblyHelper() {
 if (CodeGenOpts.DisableFree)
@@ -695,7 +698,6 @@
   // manually (and not via PMBuilder), since some passes (eg. InstrProfiling)
   // are inserted before PMBuilder ones - they'd get the default-constructed
   // TLI with an unknown target otherwise.
-  Triple TargetTriple(TheModule->getTargetTriple());
   std::unique_ptr TLII(
   createTLII(TargetTriple, CodeGenOpts));
 
@@ -965,7 +967,6 @@
raw_pwrite_stream &OS,
raw_pwrite_stream *DwoOS) {
   // Add LibraryInfo.
-  llvm::Triple TargetTriple(TheModule->getTargetTriple());
   std::unique_ptr TLII(
   createTLII(TargetTriple, CodeGenOpts));
   CodeGenPasses.add(new TargetLibraryInfoWrapperPass(*TLII));
@@ -1054,10 +1055,8 @@
   // Emit a module summary by default for Regular LTO except for ld64
   // targets
   bool EmitLTOSummary =
-  (CodeGenOpts.PrepareForLTO &&
-   !CodeGenOpts.DisableLLVMPasses &&
-   llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
-   llvm::Triple::Apple);
+  (CodeGenOpts.PrepareForLTO && !CodeGenOpts.DisableLLVMPasses &&
+   TargetTriple.getVendor() != llvm::Triple::Apple);
   if (EmitLTOSummary) {
 if (!TheModule->getModuleFlag("ThinLTO"))
   TheModule->addModuleFlag(Module::Error, "ThinLTO", uint32_t(0));
@@ -1338,7 +1337,6 @@
 
   // Register the target library analysis directly and give it a customized
   // preset TLI.
-  Triple TargetTriple(TheModule->getTargetTriple());
   std::unique_ptr TLII(
   createTLII(TargetTriple, CodeGenOpts));
   FAM.registerPass([&] { return TargetLibraryAnalysis(*TLII); });
@@ -1474,8 +1472,7 @@
   // targets
   bool EmitLTOSummary =
   (CodeGenOpts.PrepareForLTO && !CodeGenOpts.DisableLLVMPasses &&
-   llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
-   llvm::Triple::Apple);
+   TargetTriple.getVendor() != llvm::Triple::Apple);
   if (EmitLTOSummary) {
 if (!TheModule->getModuleFlag("ThinLTO"))
   TheModule->addModuleFlag(Module::Error, "ThinLTO", uint32_t(0));


Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -118,6 +118,8 @@
 
   std::unique_ptr OS;
 
+  Triple TargetTriple;
+
   TargetIRAnalysis getTargetIRAnalysis() const {
 if (TM)
   return TM->getTargetIRAnalysis();
@@ -170,7 +172,8 @@
  const LangOptions &LOpts, Module *M)
   : Diags(_Diags), HSOpts(HeaderSearchOpts), CodeGenOpts(CGOpts),
 TargetOpts(TOpts), LangOpts(LOpts), TheModule(M),
-CodeGenerationTime("codegen", "Code Generation Time") {}
+CodeGenerationTime("codegen", "Code Generation Time"),
+TargetTriple(TheModule->getTargetTriple()) {}
 
   ~EmitAssemblyHelper() {
 if (CodeGenOpts.DisableFree)
@@ -695,7 +698,6 @@
   // manually (and not via PMBuilder), since some passes (eg. InstrProfiling)
   // are inserted before PMBuilder ones - they'd get the default-constructed
   // TLI with an unknown target otherwise.
-  Triple TargetTriple(TheModule->getTargetTriple());
   std::unique_ptr TLII(
   createTLII(TargetTriple, CodeGenOpts));
 
@@ -965,7 +967,6 @@
raw_pwrite_stream &OS,
raw_pwrite_stream *DwoOS) {
   // Add LibraryInfo.
-  llvm::Triple TargetTriple(TheModule->getTargetTriple());
   std::unique_ptr TLII(
   createTLII(TargetTriple, CodeGenOpts));
   CodeGenPasses.add(new TargetLibraryInfoWrapperPass(*TLII));
@@ -1054,10 +1055,8 @@
   // Emit a module summary by default for Regular LTO except for ld6

[PATCH] D118935: [SYCL] Disallow explicit casts between mismatching address spaces

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118935/new/

https://reviews.llvm.org/D118935

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114483: [SYCL] Add support for sycl_special_class attribute

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM, just one suggestion.
It would be great to get @aaron.ballman approve too.




Comment at: clang/lib/Sema/SemaDecl.cpp:16690
+ diag::err_sycl_special_type_missing_init_method);
+}
   }

I think we might want to check that there is only one member function with 
`__init` name to avoid ambiguity with building kernel parameters.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114483/new/

https://reviews.llvm.org/D114483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116266: [SPIR-V] Add linking of separate translation units using spirv-link

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/docs/UsersManual.rst:3602
 
+Linking is done using ``spirv-link`` from `the SPIRV-Tools project
+`_. Similar to other 
external

@Anastasia, sorry for late feedback.
I think being able to link SPIR-V modules is a great feature, but I have a 
concerns regarding `spirv-link` tool.
The documentation says that the linker tool is still under development and from 
our experience this tool had issues blocking us from using it for SYCL mode. 
The last time new features were added to this tool is almost 4 year ago.
Do you know if there are any plans for to finish the development and if ? Are 
you aware of any "real-world usages" of this tool? Have you tried to use it for 
SPIR-V module produced from C++ (e.g. C++ for OpenCL)?
I think supporting SPIR-V extensions like [[ 
https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/KHR/SPV_KHR_linkonce_odr.asciidoc
 | SPV_KHR_linkonce_odr ]] is quite important for code size and JIT compilation 
time reduction. As this extension was ratified recently, I suppose `spirv-link` 
doesn't support it yet.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116266/new/

https://reviews.llvm.org/D116266

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109818: [HIPSPV] Convert HIP kernels to SPIR-V kernels

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9ae5810b53c2: [HIPSPV] Convert HIP kernels to SPIR-V kernels 
(authored by linjamaki, committed by bader).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109818/new/

https://reviews.llvm.org/D109818

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGenHIP/hipspv-kernel.cpp

Index: clang/test/CodeGenHIP/hipspv-kernel.cpp
===
--- /dev/null
+++ clang/test/CodeGenHIP/hipspv-kernel.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple spirv64 -x hip -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck %s
+
+#define __global__ __attribute__((global))
+
+// CHECK: define {{.*}}spir_kernel void @_Z3fooPff(float addrspace(1)* {{.*}}, float {{.*}})
+__global__ void foo(float *a, float b) {
+  *a = b;
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -10228,12 +10228,23 @@
 private:
   void setCCs();
 };
+
+class SPIRVABIInfo : public CommonSPIRABIInfo {
+public:
+  SPIRVABIInfo(CodeGenTypes &CGT) : CommonSPIRABIInfo(CGT) {}
+  void computeInfo(CGFunctionInfo &FI) const override;
+
+private:
+  ABIArgInfo classifyKernelArgumentType(QualType Ty) const;
+};
 } // end anonymous namespace
 namespace {
 class CommonSPIRTargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   CommonSPIRTargetCodeGenInfo(CodeGen::CodeGenTypes &CGT)
   : TargetCodeGenInfo(std::make_unique(CGT)) {}
+  CommonSPIRTargetCodeGenInfo(std::unique_ptr ABIInfo)
+  : TargetCodeGenInfo(std::move(ABIInfo)) {}
 
   LangAS getASTAllocaAddressSpace() const override {
 return getLangASFromTargetAS(
@@ -10242,18 +10253,60 @@
 
   unsigned getOpenCLKernelCallingConv() const override;
 };
-
+class SPIRVTargetCodeGenInfo : public CommonSPIRTargetCodeGenInfo {
+public:
+  SPIRVTargetCodeGenInfo(CodeGen::CodeGenTypes &CGT)
+  : CommonSPIRTargetCodeGenInfo(std::make_unique(CGT)) {}
+  void setCUDAKernelCallingConvention(const FunctionType *&FT) const override;
+};
 } // End anonymous namespace.
+
 void CommonSPIRABIInfo::setCCs() {
   assert(getRuntimeCC() == llvm::CallingConv::C);
   RuntimeCC = llvm::CallingConv::SPIR_FUNC;
 }
 
+ABIArgInfo SPIRVABIInfo::classifyKernelArgumentType(QualType Ty) const {
+  if (getContext().getLangOpts().HIP) {
+// Coerce pointer arguments with default address space to CrossWorkGroup
+// pointers for HIPSPV. When the language mode is HIP, the SPIRTargetInfo
+// maps cuda_device to SPIR-V's CrossWorkGroup address space.
+llvm::Type *LTy = CGT.ConvertType(Ty);
+auto DefaultAS = getContext().getTargetAddressSpace(LangAS::Default);
+auto GlobalAS = getContext().getTargetAddressSpace(LangAS::cuda_device);
+if (LTy->isPointerTy() && LTy->getPointerAddressSpace() == DefaultAS) {
+  LTy = llvm::PointerType::get(
+  cast(LTy)->getElementType(), GlobalAS);
+  return ABIArgInfo::getDirect(LTy, 0, nullptr, false);
+}
+  }
+  return classifyArgumentType(Ty);
+}
+
+void SPIRVABIInfo::computeInfo(CGFunctionInfo &FI) const {
+  // The logic is same as in DefaultABIInfo with an exception on the kernel
+  // arguments handling.
+  llvm::CallingConv::ID CC = FI.getCallingConvention();
+
+  if (!getCXXABI().classifyReturnType(FI))
+FI.getReturnInfo() = classifyReturnType(FI.getReturnType());
+
+  for (auto &I : FI.arguments()) {
+if (CC == llvm::CallingConv::SPIR_KERNEL) {
+  I.info = classifyKernelArgumentType(I.type);
+} else {
+  I.info = classifyArgumentType(I.type);
+}
+  }
+}
+
 namespace clang {
 namespace CodeGen {
 void computeSPIRKernelABIInfo(CodeGenModule &CGM, CGFunctionInfo &FI) {
-  DefaultABIInfo SPIRABI(CGM.getTypes());
-  SPIRABI.computeInfo(FI);
+  if (CGM.getTarget().getTriple().isSPIRV())
+SPIRVABIInfo(CGM.getTypes()).computeInfo(FI);
+  else
+CommonSPIRABIInfo(CGM.getTypes()).computeInfo(FI);
 }
 }
 }
@@ -10262,6 +10315,16 @@
   return llvm::CallingConv::SPIR_KERNEL;
 }
 
+void SPIRVTargetCodeGenInfo::setCUDAKernelCallingConvention(
+const FunctionType *&FT) const {
+  // Convert HIP kernels to SPIR-V kernels.
+  if (getABIInfo().getContext().getLangOpts().HIP) {
+FT = getABIInfo().getContext().adjustFunctionType(
+FT, FT->getExtInfo().withCallingConv(CC_OpenCLKernel));
+return;
+  }
+}
+
 static bool appendType(SmallStringEnc &Enc, QualType QType,
const CodeGen::CodeGenModule &CGM,
TypeStringCache &TSC);
@@ -11327,9 +11390,10 @@
 return SetCGInfo(new ARCTargetCodeGenInfo(Types));
   case llvm::Triple::spir:
   case llvm::Triple::spir64:
+return SetCGInfo(new CommonSPIRTargetCodeGenInfo(Types));
   case llvm::Triple::spirv32:
   case llvm::Triple::spirv64:
-return SetCGInfo(new CommonSPIRTargetCodeGenInfo(T

[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D110622#3174113 , @tra wrote:

> The patch looks OK for the time being. That said, I do have concerns that we 
> may be organically growing something that will be troublesome to deal with 
> long-term.
>
> TBH, I still can't quite make sense of where/how SPIR-V fits in the 
> offloading nomenclature.
>
> Right now we have multiple levels of offloading-related control points.
>
> - offload targets, specified by --offload-arch. Determines the ISA of the GPU 
> binary we produce.
> - offload mechanism: OpenMP, CUDA runtime, HSA. Determines how we 
> compile/pack/launch the GPU binaries.
> - front-end: CUDA/HIP/ C/C++ w/ OpenMP.
> - Driver: Determines compilation pipeline to glue everything together,
>
> SPIR-V in these patches appears to be wearing multiple hats. 
> It changes compilation pipeline, it changes offload mechanism and it changes 
> offload targets.

From my POV, SPIR-V is "the ISA of GPU binary we produce" and we might need 
some changes at offloading-related control points:

- offload mechanism: none of listed "offload mechanisms" (i.e. OpenMP, CUDA 
runtime, HSA) is able to handle SPIR-V natively. On the other hand, I'm not 
sure if there is a need in additional changes for all "offloading mechanisms". 
E.g. Intel's compiler implements OpenMP-offload to SPIR-V target using OpenMP 
runtime plug-in lowering OpenMP runtime calls to OpenCL/Level Zero. OpenCL and 
Level Zero  runtimes are 
able to compile and launch SPIR-V binaries.
- front-end: if we compare SPIR to other ISAs, they change compilation pipeline 
as well (e.g. add new built-ins to expose ISA, add CodeGen library changes to 
emit ISA specific metadata, etc.). AMDGPU ISA 
 or NVIDIA 
 GPU 
 ISA changed front-end/compilation 
pipeline as well. Do you refer to some other non-ISA specific changes? BTW, 
shameless plug, the patch adding built-in functions and types for SPIR-V ISA is 
under review here - https://reviews.llvm.org/D108034.
- Driver: front-end compiler doesn't support SPIR-V format yet (i.e. SPIR-V 
requires special encoding different from LLVM bitcode) , so Driver hooks up 
LLVM->SPIR-V translator tool to produce SPIR-V binary.

> To further complicate things, it appears to be a derivative of the HIP 
> compilation. I can't tell if it's an implementation detail at the moment, or 
> whether it will become a more generic offload mechanism that would be 
> expected to be used by other front- and back-ends. E.g. can we potentially 
> compile CUDA code to target SPIR-V? Can OpenMP offload to SPIR-V?

Intel's compiler compiles OpenMP offload and SYCL to SPIR-V, so we definitely 
would like to target SPIR-V using other front-ends.

> So, the question is -- what's the right way to specify something like this in 
> a consistent manner? 
> `--offload` option proposed here does not seem to be a good fit. It was 
> intended as a more flexible way to create a single `-cc1` sub-compilation and 
> we're doing quite a bit more here.

Does `--offload-arch=spirv*` fit better here? If I understand the goal of this 
patch correctly, it tries to provide controls for changing offload target for 
HIP application from default (AMDGCN) to SPIR-V.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110622/new/

https://reviews.llvm.org/D110622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109818: [HIPSPV] Convert HIP kernels to SPIR-V kernels

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D109818#3169531 , @linjamaki wrote:

> The patch is ready to land. @Anastasia, @bader, could you commit this patch 
> to the LLVM for us? Thanks.

Could you rebase on the tip of the main branch, please? I see a couple of 
conflicts when I cherry-pick the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109818/new/

https://reviews.llvm.org/D109818

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114080: [SYCL] Diagnose uses of zero length arrays

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

LGTM, with a couple of minor suggestions.




Comment at: clang/lib/Sema/SemaSYCL.cpp:68-75
+bool ErrorFound = false;
+if (isZeroSizedArray(*this, TypeToCheck)) {
+  SYCLDiagIfDeviceCode(UsedAt, diag::err_sycl_zero_array_size);
+  ErrorFound = true;
+}
+// Checks for other types can also be done here.
+if (ErrorFound) {





Comment at: clang/lib/Sema/SemaSYCL.cpp:125
+
+// In case pointer/array/reference type is met get pointeetype, then 
proceed
+// with that type.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114080/new/

https://reviews.llvm.org/D114080

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107054: [Clang][CUDA] Add descriptors, mappings, and features for missing CUDA and PTX versions

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.
Herald added subscribers: carlosgalvezp, asavonic.

@tra, ping.
@steffenlarsen, does it make sense to add support for recently released 11.5 as 
well?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107054/new/

https://reviews.llvm.org/D107054

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112404: [SPIR-V] Add translator tool

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG49682f14bf3f: [SPIR-V] Add translator tool (authored by 
linjamaki, committed by bader).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112404/new/

https://reviews.llvm.org/D112404

Files:
  clang/lib/Driver/CMakeLists.txt
  clang/lib/Driver/ToolChains/SPIRV.cpp
  clang/lib/Driver/ToolChains/SPIRV.h

Index: clang/lib/Driver/ToolChains/SPIRV.h
===
--- /dev/null
+++ clang/lib/Driver/ToolChains/SPIRV.h
@@ -0,0 +1,46 @@
+//===--- SPIRV.h - SPIR-V Tool Implementations --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_SPIRV_H
+#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_SPIRV_H
+
+#include "clang/Driver/Tool.h"
+#include "clang/Driver/ToolChain.h"
+
+namespace clang {
+namespace driver {
+namespace tools {
+namespace SPIRV {
+
+void addTranslatorArgs(const llvm::opt::ArgList &InArgs,
+   llvm::opt::ArgStringList &OutArgs);
+
+void constructTranslateCommand(Compilation &C, const Tool &T,
+   const JobAction &JA, const InputInfo &Output,
+   const InputInfo &Input,
+   const llvm::opt::ArgStringList &Args);
+
+class LLVM_LIBRARY_VISIBILITY Translator : public Tool {
+public:
+  Translator(const ToolChain &TC)
+  : Tool("SPIR-V::Translator", "llvm-spirv", TC) {}
+
+  bool hasIntegratedCPP() const override { return false; }
+  bool hasIntegratedAssembler() const override { return true; }
+
+  void ConstructJob(Compilation &C, const JobAction &JA,
+const InputInfo &Output, const InputInfoList &Inputs,
+const llvm::opt::ArgList &TCArgs,
+const char *LinkingOutput) const override;
+};
+
+} // namespace SPIRV
+} // namespace tools
+} // namespace driver
+} // namespace clang
+#endif
Index: clang/lib/Driver/ToolChains/SPIRV.cpp
===
--- /dev/null
+++ clang/lib/Driver/ToolChains/SPIRV.cpp
@@ -0,0 +1,48 @@
+//===--- SPIRV.cpp - SPIR-V Tool Implementations *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "SPIRV.h"
+#include "CommonArgs.h"
+#include "clang/Driver/Compilation.h"
+#include "clang/Driver/Driver.h"
+#include "clang/Driver/InputInfo.h"
+#include "clang/Driver/Options.h"
+
+using namespace clang::driver::tools;
+using namespace llvm::opt;
+
+void SPIRV::constructTranslateCommand(Compilation &C, const Tool &T,
+  const JobAction &JA,
+  const InputInfo &Output,
+  const InputInfo &Input,
+  const llvm::opt::ArgStringList &Args) {
+  llvm::opt::ArgStringList CmdArgs(Args);
+  CmdArgs.push_back(Input.getFilename());
+
+  if (Input.getType() == types::TY_PP_Asm)
+CmdArgs.push_back("-to-binary");
+  if (Output.getType() == types::TY_PP_Asm)
+CmdArgs.push_back("-spirv-text");
+
+  CmdArgs.append({"-o", Output.getFilename()});
+
+  const char *Exec =
+  C.getArgs().MakeArgString(T.getToolChain().GetProgramPath("llvm-spirv"));
+  C.addCommand(std::make_unique(JA, T, ResponseFileSupport::None(),
+ Exec, CmdArgs, Input, Output));
+}
+
+void SPIRV::Translator::ConstructJob(Compilation &C, const JobAction &JA,
+ const InputInfo &Output,
+ const InputInfoList &Inputs,
+ const ArgList &Args,
+ const char *LinkingOutput) const {
+  claimNoWarnArgs(Args);
+  if (Inputs.size() != 1)
+llvm_unreachable("Invalid number of input files.");
+  constructTranslateCommand(C, *this, JA, Output, Inputs[0], {});
+}
Index: clang/lib/Driver/CMakeLists.txt
===
--- clang/lib/Driver/CMakeLists.txt
+++ clang/lib/Driver/CMakeLists.txt
@@ -69,6 +69,7 @@
   ToolChains/PS4CPU.cpp
   ToolChains/RISCVToolchain.cpp
   ToolChains/Solaris.cpp
+  ToolChains/SPIRV.cpp
   ToolChains/TCE.cpp
   ToolChains/VEToolchain.cpp
   ToolChains/WebAssembly.cpp
___
cfe-commits mailing list
cfe-commits@lists

[PATCH] D112404: [SPIR-V] Add translator tool

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

This part looks good to me. Just a couple of minor style comments.




Comment at: clang/lib/Driver/ToolChains/SPIRV.cpp:18
+
+void SPIRV::constructTranslateCommand(Compilation &C, const Tool &T,
+  const JobAction &JA,

If this function is going to be used only by `SPIRV::Translator::ConstructJob`, 
it's better to make it `static` or manually inline into 4-line 
`SPIRV::Translator::ConstructJob`.



Comment at: clang/lib/Driver/ToolChains/SPIRV.h:31
+  Translator(const ToolChain &TC)
+  : Tool("SPIRV::Translator", "translator", TC) {}
+

I think using just "translator" as a short name might be ambiguous.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112404/new/

https://reviews.llvm.org/D112404

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

@vlastik, your commit fixes function pointers on AVR - 
https://github.com/llvm/llvm-project/commit/57fd86de879cf2b4c7001b6d0a09df60877ce24d.
 I suppose this change is required for fixing lvalue references to function 
pointers on AVR as well. Right?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71016: [SYCL] Implement OpenCL kernel function generation

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D71016#3063762 , @tschuett wrote:

> Would a codegenSYCL directory help you to separate Sema from code generation?

Moving wrapper kernel function generation to CodeGen library make sense to me.

> Doesn't this make AST non-representable of the reality,
> shouldn't the lowering happen in codegen, not in sema?

I'm not sure I understand what does "make AST non-representable of the reality" 
mean, but it seems to be the same suggestion as @tschuett proposed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71016/new/

https://reviews.llvm.org/D71016

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71016: [SYCL] Implement OpenCL kernel function generation

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D71016#3063457 , @tschuett wrote:

> It feels like you are doing codegen(OpenCL kernel) in Sema. Are OpenCL 
> kernels the only approach.

We need to update the description of the patch to clarify that it applies to 
other GPU programming models as well. When the patch was uploaded SYCL targeted 
OpenCL kernels only and today the downstream implementation can target CUDA, 
HIP and Intel oneAPI Level Zero kernels as well.
SYCL kernel is defined as C++ callable type, but typical GPU kernel interface 
is a C-like function. In addition to that there might be additional 
restrictions on passing data from the host system (e.g. image types can be 
passed as a member of C++ class, etc.). The solution here is emit a wrapper 
function, which initializes and invokes SYCL callable object.

Does it answer your question or you would like to see changes in addition to 
the description update?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71016/new/

https://reviews.llvm.org/D71016

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71016: [SYCL] Implement OpenCL kernel function generation

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/Sema/SemaSYCL.cpp:45
+  /// accessor class.
+  static bool isSyclAccessorType(const QualType &Ty);
+

erichkeane wrote:
> Isn't there a big rewrite going on downstream of these with 
> `sycl_special_class`?  Why are we trying to upstream this before that happens?
> Isn't there a big rewrite going on downstream of these with 
> `sycl_special_class`?  

Yes.

> Why are we trying to upstream this before that happens?

The downstream work was initiated by this comment: 
https://reviews.llvm.org/D71016#inline-644645.
This patch was uploaded for review here before refactoring work has started.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71016/new/

https://reviews.llvm.org/D71016

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109144: [SPIR-V] Add SPIR-V triple architecture and clang target info

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D109144#3042247 , @Anastasia wrote:

> 1. Implementing SPIR-V target as SPIR target.  @bader do you suggest that we 
> add `spirv` triple to clang and map it into SPIR taget or do you suggest 
> something different?

What I have in mind is to continue using SPIR target for now (until SPIR-V 
back-end is added).
For instance, SYCL compiler emits code for SPIR target and code format is 
configured via flag.

`-emit-llvm` changes output file format for regular C++ compilation flow:

  clang++ a.cpp -c -o a.o  # object format by default 
  clang++ a.cpp -c -emit-llvm -o a.bc  # LLVM IR format with 
`-emit-llvm`

Similar approach for HIP device compilation flow:

  clang++ -target spir -x hip a.cpp -cuda-device-only -o a.spv 
# SPIR-V format by default
  clang++ -target spir -x hip a.cpp -cuda-device-only -emit-llvm -o a.bc   
# LLVM IR (aka SPIR) format with `-emit-llvm` if needed

I think this was proposed in RFC. @linjamaki, am I right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109144/new/

https://reviews.llvm.org/D109144

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > bader wrote:
> > > > Anastasia wrote:
> > > > > linjamaki wrote:
> > > > > > bader wrote:
> > > > > > > keryell wrote:
> > > > > > > > Anastasia wrote:
> > > > > > > > > bader wrote:
> > > > > > > > > > Anastasia wrote:
> > > > > > > > > > > I am slightly confused as in the LLVM project those 
> > > > > > > > > > > address spaces are for SPIR not SPIR-V though. It is 
> > > > > > > > > > > however used outside of LLVM project by some tools like 
> > > > > > > > > > > SPIRV-LLVM Translator as a path to SPIR-V, but it has 
> > > > > > > > > > > only been done as a workaround since we had no SPIR-V 
> > > > > > > > > > > support in the LLVM project yet. And if we are adding it 
> > > > > > > > > > > let's do it clean to avoid/resolve any confusion.
> > > > > > > > > > > 
> > > > > > > > > > > I think we need to keep both because some vendors do 
> > > > > > > > > > > target/use SPIR but not SPIR-V.
> > > > > > > > > > > 
> > > > > > > > > > > So if you are interested in SPIR-V and not SPIR you 
> > > > > > > > > > > should probably add a new target that will make things 
> > > > > > > > > > > cleaner.
> > > > > > > > > > > I think we need to keep both because some vendors do 
> > > > > > > > > > > target/use SPIR but not SPIR-V.
> > > > > > > > > > 
> > > > > > > > > > @Anastasia, could you elaborate more on the difference 
> > > > > > > > > > between SPIR and SPIR-V?
> > > > > > > > > > I would like to understand what these terms mean in the 
> > > > > > > > > > context of LLVM project.
> > > > > > > > > Their conceptual differences are just that they are two 
> > > > > > > > > different intermediate formats.
> > > > > > > > > 
> > > > > > > > > The important thing to highlight is that it is not impossible 
> > > > > > > > > that some vendors use SPIR (without using SPIR-V) even 
> > > > > > > > > despite the fact it has been discontinued by Khronos. 
> > > > > > > > > 
> > > > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM 
> > > > > > > > > project yet.
> > > > > > > > > Their conceptual differences are just that they are two 
> > > > > > > > > different intermediate formats.
> > > > > > > > > 
> > > > > > > > > The important thing to highlight is that it is not impossible 
> > > > > > > > > that some vendors use SPIR (without using SPIR-V) even 
> > > > > > > > > despite the fact it has been discontinued by Khronos. 
> > > > > > > > > 
> > > > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM 
> > > > > > > > > project yet.
> > > > > > > > 
> > > > > > > > All the official Xilinx OpenCL stack is based on legacy SPIR 
> > > > > > > > (encoded in LLVM 6.x IR but this is another story) and I 
> > > > > > > > suspect this is the case for other companies.
> > > > > > > > So, do not deprecate or discontinue, please. :-)
> > > > > > > > The important thing to highlight is that it is not impossible 
> > > > > > > > that some vendors use SPIR (without using SPIR-V) even despite 
> > > > > > > > the fact it has been discontinued by Khronos.
> > > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM project 
> > > > > > > > yet.
> > > > > > > 
> > > > > > > Strictly speaking `SPIR` is not defined as an intermediate 
> > > > > > > language. Khronos defines `SPIR-1.2` and `SPIR-2.0` formats which 
> > > > > > > are based on LLVM 3.2 and LLVM 3.4 version 
> > > > > > > (https://www.khronos.org/spir/). There is no definition of SPIR 
> > > > > > > format based on current version of LLVM IR. Another note is that 
> > > > > > > metadata and intrinsics emitted for OpenCL with clang-14 doesn't 
> > > > > > > follow neither `SPIR-1.2` nor `SPIR-2.0`.
> > > > > > > 
> > > > > > > I always think of LLVM IR as leaving thing that is subject to 
> > > > > > > change by LLVM community, so tools working with LLVM IR must 
> > > > > > > adjust to the particular version (e.g. release version like LLVM 
> > > > > > > 13 or ToT). We apply this logic to SPIRV-LLVM-Translator tool and 
> > > > > > > update it according to LLVM format changes (e.g. kernel argument 
> > > > > > > information defined in Khronos spec must be named metadata 
> > > > > > > whereas clang emits function metadata).
> > > > > > > 
> > > > > > > > I am slightly confused as in the LLVM project those address 
> > > > > > > > spaces are for SPIR not SPIR-V though.
> > > > > > > [skip]
> > > > > > > > Their conceptual differences are just that they are two 
> > > > > > > > different intermediate formats.
> > > > > > > 
> > > > > > > If this is the only difference, I don't think it a good idea to 
> > > > > > > create another LLVM target to separate SPIR and SPIR-V. From my 
> > > > >

[PATCH] D109144: [SPIR-V] Add SPIR-V triple architecture and clang target info

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D109144#3032865 , @Anastasia wrote:

> It would be good to get closure on this asap.
>
> @bader We had related discussions on the other reviews about the approach in 
> this patch. If you have any concerns/suggestions can you please notify asap...

Sorry for the delay. I was on vacation last week. I've added my concerns to the 
discussion in https://reviews.llvm.org/D108621.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109144/new/

https://reviews.llvm.org/D109144

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > linjamaki wrote:
> > > > bader wrote:
> > > > > keryell wrote:
> > > > > > Anastasia wrote:
> > > > > > > bader wrote:
> > > > > > > > Anastasia wrote:
> > > > > > > > > I am slightly confused as in the LLVM project those address 
> > > > > > > > > spaces are for SPIR not SPIR-V though. It is however used 
> > > > > > > > > outside of LLVM project by some tools like SPIRV-LLVM 
> > > > > > > > > Translator as a path to SPIR-V, but it has only been done as 
> > > > > > > > > a workaround since we had no SPIR-V support in the LLVM 
> > > > > > > > > project yet. And if we are adding it let's do it clean to 
> > > > > > > > > avoid/resolve any confusion.
> > > > > > > > > 
> > > > > > > > > I think we need to keep both because some vendors do 
> > > > > > > > > target/use SPIR but not SPIR-V.
> > > > > > > > > 
> > > > > > > > > So if you are interested in SPIR-V and not SPIR you should 
> > > > > > > > > probably add a new target that will make things cleaner.
> > > > > > > > > I think we need to keep both because some vendors do 
> > > > > > > > > target/use SPIR but not SPIR-V.
> > > > > > > > 
> > > > > > > > @Anastasia, could you elaborate more on the difference between 
> > > > > > > > SPIR and SPIR-V?
> > > > > > > > I would like to understand what these terms mean in the context 
> > > > > > > > of LLVM project.
> > > > > > > Their conceptual differences are just that they are two different 
> > > > > > > intermediate formats.
> > > > > > > 
> > > > > > > The important thing to highlight is that it is not impossible 
> > > > > > > that some vendors use SPIR (without using SPIR-V) even despite 
> > > > > > > the fact it has been discontinued by Khronos. 
> > > > > > > 
> > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM project 
> > > > > > > yet.
> > > > > > > Their conceptual differences are just that they are two different 
> > > > > > > intermediate formats.
> > > > > > > 
> > > > > > > The important thing to highlight is that it is not impossible 
> > > > > > > that some vendors use SPIR (without using SPIR-V) even despite 
> > > > > > > the fact it has been discontinued by Khronos. 
> > > > > > > 
> > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM project 
> > > > > > > yet.
> > > > > > 
> > > > > > All the official Xilinx OpenCL stack is based on legacy SPIR 
> > > > > > (encoded in LLVM 6.x IR but this is another story) and I suspect 
> > > > > > this is the case for other companies.
> > > > > > So, do not deprecate or discontinue, please. :-)
> > > > > > The important thing to highlight is that it is not impossible that 
> > > > > > some vendors use SPIR (without using SPIR-V) even despite the fact 
> > > > > > it has been discontinued by Khronos.
> > > > > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > > > > 
> > > > > Strictly speaking `SPIR` is not defined as an intermediate language. 
> > > > > Khronos defines `SPIR-1.2` and `SPIR-2.0` formats which are based on 
> > > > > LLVM 3.2 and LLVM 3.4 version (https://www.khronos.org/spir/). There 
> > > > > is no definition of SPIR format based on current version of LLVM IR. 
> > > > > Another note is that metadata and intrinsics emitted for OpenCL with 
> > > > > clang-14 doesn't follow neither `SPIR-1.2` nor `SPIR-2.0`.
> > > > > 
> > > > > I always think of LLVM IR as leaving thing that is subject to change 
> > > > > by LLVM community, so tools working with LLVM IR must adjust to the 
> > > > > particular version (e.g. release version like LLVM 13 or ToT). We 
> > > > > apply this logic to SPIRV-LLVM-Translator tool and update it 
> > > > > according to LLVM format changes (e.g. kernel argument information 
> > > > > defined in Khronos spec must be named metadata whereas clang emits 
> > > > > function metadata).
> > > > > 
> > > > > > I am slightly confused as in the LLVM project those address spaces 
> > > > > > are for SPIR not SPIR-V though.
> > > > > [skip]
> > > > > > Their conceptual differences are just that they are two different 
> > > > > > intermediate formats.
> > > > > 
> > > > > If this is the only difference, I don't think it a good idea to 
> > > > > create another LLVM target to separate SPIR and SPIR-V. From my point 
> > > > > of view it creates logic ambiguity and code duplication with no 
> > > > > additional value. @Anastasia, what problems do you see if we continue 
> > > > > treating LLVM IR with spir* target triple as LLVM IR representation 
> > > > > of SPIR-V format?
> > > > The state of SPIR 1.2/2.0 in Clang seems to be that the SPIR target has 
> > > > transformed to mean “SPIR 1.2/2.0 derivative”, but that do

[PATCH] D110281: Change __builtin_sycl_unique_stable_name to just use an Itanium mangling

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

LGTM, just one typo in addition to linter reports and I'd like John to take a 
look.




Comment at: clang/docs/LanguageExtensions.rst:2524
 mangling scheme at runtime. The mangler marks all the lambdas required to name
-the SYCL kernel and emits a stable local ordering of the respective lambdas,
-starting from ``1``. The initial value of ``1`` serves as an obvious
-differentiator from ordinary lambda mangling numbers but does not serve any
-other purpose and may change in the future. The resulting pattern is
-demanglable. When non-lambda types are passed to the builtin, the mangler emits
-their usual pattern without any special treatment.
+the SYCL kernel a nd emits a stable local ordering of the respective lambdas.
+The resulting pattern is demanglable.  When non-lambda types are passed to the

"a nd" -> "and"


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110281/new/

https://reviews.llvm.org/D110281

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109818: [HIPSPV] Convert HIP kernels to SPIR-V kernels

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:10224
+// pointers for HIPSPV. When the language mode is HIP, the SPIRTargetInfo
+// maps cuda_device to SPIR-V's CrossWorkGroup address space.
+llvm::Type *LTy = CGT.ConvertType(Ty);

Anastasia wrote:
> linjamaki wrote:
> > Anastasia wrote:
> > > Can you explain why this mapping is needed? We already have an address 
> > > space map to perform the mapping of address spaces b/w language and 
> > > target. It would be good if we don't replicate similar logic in too many 
> > > places.
> > HIP does not require address space qualifiers on kernel pointer arguments 
> > (e.g. see hipspv-kernel.cpp) nor there are AS qualifiers that can be placed 
> > on them. With the default logic, provided by SPIRVTargetInfo’s address 
> > space map, the kernel pointer arguments get converted to generic pointers 
> > which are not allowed by the OpenCL SPIR-V Environment Specification.
> I feel that it is the same for SYCL... It might be good to check with @bader 
> whether there is already a way to handle this that can be reused for HIP...
We need to do similar transformation for SYCL, but it's not exactly the same. 
For SYCL kernels, which represented as function objects, compiler generates 
SPIR kernel function and fixes up the address space for pointer arguments in 
compiler generated declaration. For more details, see the description of 
https://reviews.llvm.org/D71016  and `handlePointerType` function code in 
clang/lib/Sema/SemaSYCL.cpp of this review request (lines 848-876). As address 
space is fixed in Sema, it works for all targets SYCL currently supports SPIR, 
NVPTX and AMDGPU.

If I understand it correctly, we are trying to do minimal amount of work for 
convert HIP kernel function to SPIR kernel function, i.e. fix calling 
convention and address spaces. 
Are these two fixes enough or we need more fixes to enable more sophisticated 
kernels?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109818/new/

https://reviews.llvm.org/D109818

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

Anastasia wrote:
> linjamaki wrote:
> > bader wrote:
> > > keryell wrote:
> > > > Anastasia wrote:
> > > > > bader wrote:
> > > > > > Anastasia wrote:
> > > > > > > I am slightly confused as in the LLVM project those address 
> > > > > > > spaces are for SPIR not SPIR-V though. It is however used outside 
> > > > > > > of LLVM project by some tools like SPIRV-LLVM Translator as a 
> > > > > > > path to SPIR-V, but it has only been done as a workaround since 
> > > > > > > we had no SPIR-V support in the LLVM project yet. And if we are 
> > > > > > > adding it let's do it clean to avoid/resolve any confusion.
> > > > > > > 
> > > > > > > I think we need to keep both because some vendors do target/use 
> > > > > > > SPIR but not SPIR-V.
> > > > > > > 
> > > > > > > So if you are interested in SPIR-V and not SPIR you should 
> > > > > > > probably add a new target that will make things cleaner.
> > > > > > > I think we need to keep both because some vendors do target/use 
> > > > > > > SPIR but not SPIR-V.
> > > > > > 
> > > > > > @Anastasia, could you elaborate more on the difference between SPIR 
> > > > > > and SPIR-V?
> > > > > > I would like to understand what these terms mean in the context of 
> > > > > > LLVM project.
> > > > > Their conceptual differences are just that they are two different 
> > > > > intermediate formats.
> > > > > 
> > > > > The important thing to highlight is that it is not impossible that 
> > > > > some vendors use SPIR (without using SPIR-V) even despite the fact it 
> > > > > has been discontinued by Khronos. 
> > > > > 
> > > > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > > > > Their conceptual differences are just that they are two different 
> > > > > intermediate formats.
> > > > > 
> > > > > The important thing to highlight is that it is not impossible that 
> > > > > some vendors use SPIR (without using SPIR-V) even despite the fact it 
> > > > > has been discontinued by Khronos. 
> > > > > 
> > > > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > > > 
> > > > All the official Xilinx OpenCL stack is based on legacy SPIR (encoded 
> > > > in LLVM 6.x IR but this is another story) and I suspect this is the 
> > > > case for other companies.
> > > > So, do not deprecate or discontinue, please. :-)
> > > > The important thing to highlight is that it is not impossible that some 
> > > > vendors use SPIR (without using SPIR-V) even despite the fact it has 
> > > > been discontinued by Khronos.
> > > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > > 
> > > Strictly speaking `SPIR` is not defined as an intermediate language. 
> > > Khronos defines `SPIR-1.2` and `SPIR-2.0` formats which are based on LLVM 
> > > 3.2 and LLVM 3.4 version (https://www.khronos.org/spir/). There is no 
> > > definition of SPIR format based on current version of LLVM IR. Another 
> > > note is that metadata and intrinsics emitted for OpenCL with clang-14 
> > > doesn't follow neither `SPIR-1.2` nor `SPIR-2.0`.
> > > 
> > > I always think of LLVM IR as leaving thing that is subject to change by 
> > > LLVM community, so tools working with LLVM IR must adjust to the 
> > > particular version (e.g. release version like LLVM 13 or ToT). We apply 
> > > this logic to SPIRV-LLVM-Translator tool and update it according to LLVM 
> > > format changes (e.g. kernel argument information defined in Khronos spec 
> > > must be named metadata whereas clang emits function metadata).
> > > 
> > > > I am slightly confused as in the LLVM project those address spaces are 
> > > > for SPIR not SPIR-V though.
> > > [skip]
> > > > Their conceptual differences are just that they are two different 
> > > > intermediate formats.
> > > 
> > > If this is the only difference, I don't think it a good idea to create 
> > > another LLVM target to separate SPIR and SPIR-V. From my point of view it 
> > > creates logic ambiguity and code duplication with no additional value. 
> > > @Anastasia, what problems do you see if we continue treating LLVM IR with 
> > > spir* target triple as LLVM IR representation of SPIR-V format?
> > The state of SPIR 1.2/2.0 in Clang seems to be that the SPIR target has 
> > transformed to mean “SPIR 1.2/2.0 derivative”, but that does not still make 
> > it SPIR-V, which is not based on LLVM IR. When one is targeting spir* there 
> > is ambiguity on whether one is aiming to produce the old-SPIR-derivative or 
> > SPIR-V. Considering that there are still SPIR-derivative consumers, in my 
> > opinion we should have separate LLVM targets for SPIR-V to have explicit 
> > disambiguation of intent for producing the SPIR-derivative vs SPIR-V.
> @bade

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

keryell wrote:
> Anastasia wrote:
> > bader wrote:
> > > Anastasia wrote:
> > > > I am slightly confused as in the LLVM project those address spaces are 
> > > > for SPIR not SPIR-V though. It is however used outside of LLVM project 
> > > > by some tools like SPIRV-LLVM Translator as a path to SPIR-V, but it 
> > > > has only been done as a workaround since we had no SPIR-V support in 
> > > > the LLVM project yet. And if we are adding it let's do it clean to 
> > > > avoid/resolve any confusion.
> > > > 
> > > > I think we need to keep both because some vendors do target/use SPIR 
> > > > but not SPIR-V.
> > > > 
> > > > So if you are interested in SPIR-V and not SPIR you should probably add 
> > > > a new target that will make things cleaner.
> > > > I think we need to keep both because some vendors do target/use SPIR 
> > > > but not SPIR-V.
> > > 
> > > @Anastasia, could you elaborate more on the difference between SPIR and 
> > > SPIR-V?
> > > I would like to understand what these terms mean in the context of LLVM 
> > > project.
> > Their conceptual differences are just that they are two different 
> > intermediate formats.
> > 
> > The important thing to highlight is that it is not impossible that some 
> > vendors use SPIR (without using SPIR-V) even despite the fact it has been 
> > discontinued by Khronos. 
> > 
> > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > Their conceptual differences are just that they are two different 
> > intermediate formats.
> > 
> > The important thing to highlight is that it is not impossible that some 
> > vendors use SPIR (without using SPIR-V) even despite the fact it has been 
> > discontinued by Khronos. 
> > 
> > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> 
> All the official Xilinx OpenCL stack is based on legacy SPIR (encoded in LLVM 
> 6.x IR but this is another story) and I suspect this is the case for other 
> companies.
> So, do not deprecate or discontinue, please. :-)
> The important thing to highlight is that it is not impossible that some 
> vendors use SPIR (without using SPIR-V) even despite the fact it has been 
> discontinued by Khronos.
> Nobody has deprecated or discontinued SPIR in the LLVM project yet.

Strictly speaking `SPIR` is not defined as an intermediate language. Khronos 
defines `SPIR-1.2` and `SPIR-2.0` formats which are based on LLVM 3.2 and LLVM 
3.4 version (https://www.khronos.org/spir/). There is no definition of SPIR 
format based on current version of LLVM IR. Another note is that metadata and 
intrinsics emitted for OpenCL with clang-14 doesn't follow neither `SPIR-1.2` 
nor `SPIR-2.0`.

I always think of LLVM IR as leaving thing that is subject to change by LLVM 
community, so tools working with LLVM IR must adjust to the particular version 
(e.g. release version like LLVM 13 or ToT). We apply this logic to 
SPIRV-LLVM-Translator tool and update it according to LLVM format changes (e.g. 
kernel argument information defined in Khronos spec must be named metadata 
whereas clang emits function metadata).

> I am slightly confused as in the LLVM project those address spaces are for 
> SPIR not SPIR-V though.
[skip]
> Their conceptual differences are just that they are two different 
> intermediate formats.

If this is the only difference, I don't think it a good idea to create another 
LLVM target to separate SPIR and SPIR-V. From my point of view it creates logic 
ambiguity and code duplication with no additional value. @Anastasia, what 
problems do you see if we continue treating LLVM IR with spir* target triple as 
LLVM IR representation of SPIR-V format?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

Anastasia wrote:
> I am slightly confused as in the LLVM project those address spaces are for 
> SPIR not SPIR-V though. It is however used outside of LLVM project by some 
> tools like SPIRV-LLVM Translator as a path to SPIR-V, but it has only been 
> done as a workaround since we had no SPIR-V support in the LLVM project yet. 
> And if we are adding it let's do it clean to avoid/resolve any confusion.
> 
> I think we need to keep both because some vendors do target/use SPIR but not 
> SPIR-V.
> 
> So if you are interested in SPIR-V and not SPIR you should probably add a new 
> target that will make things cleaner.
> I think we need to keep both because some vendors do target/use SPIR but not 
> SPIR-V.

@Anastasia, could you elaborate more on the difference between SPIR and SPIR-V?
I would like to understand what these terms mean in the context of LLVM project.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM.




Comment at: clang/lib/Basic/Targets/SPIR.h:146
+// See comment on the SPIRDefIsGenMap table.
+bool IsHIPSPV = Opts.HIP && Opts.CUDAIsDevice;
 // FIXME: SYCL specification considers unannotated pointers and references

Minor: in my opinion, Opts.HIP check is unnecessary. I don't think CUDA can be 
compiled to SPIR target today, but when this flow is enabled I think it should 
set `DefaultIsGeneric` flag the same way as HIP.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108020: [NFC] Drop idle compiler option from the test.

[Alexey Bader via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd754b970eddb: [NFC] Drop idle compiler option from the test. 
(authored by bader).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108020/new/

https://reviews.llvm.org/D108020

Files:
  clang/test/AST/ast-print-sycl-unique-stable-name.cpp


Index: clang/test/AST/ast-print-sycl-unique-stable-name.cpp
===
--- clang/test/AST/ast-print-sycl-unique-stable-name.cpp
+++ clang/test/AST/ast-print-sycl-unique-stable-name.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -ast-print -fsycl-is-device %s -o - -triple 
spir64-sycldevice | FileCheck %s
+// RUN: %clang_cc1 -ast-print -fsycl-is-device %s -o - | FileCheck %s
 
 template 
 void WrappedInTemplate(T t) {


Index: clang/test/AST/ast-print-sycl-unique-stable-name.cpp
===
--- clang/test/AST/ast-print-sycl-unique-stable-name.cpp
+++ clang/test/AST/ast-print-sycl-unique-stable-name.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -ast-print -fsycl-is-device %s -o - -triple spir64-sycldevice | FileCheck %s
+// RUN: %clang_cc1 -ast-print -fsycl-is-device %s -o - | FileCheck %s
 
 template 
 void WrappedInTemplate(T t) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108020: [NFC] Drop idle compiler option from the test.

[Alexey Bader via Phabricator via cfe-commits]

bader created this revision.
bader added a reviewer: erichkeane.
Herald added a subscriber: ebevhan.
bader requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108020

Files:
  clang/test/AST/ast-print-sycl-unique-stable-name.cpp


Index: clang/test/AST/ast-print-sycl-unique-stable-name.cpp
===
--- clang/test/AST/ast-print-sycl-unique-stable-name.cpp
+++ clang/test/AST/ast-print-sycl-unique-stable-name.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -ast-print -fsycl-is-device %s -o - -triple 
spir64-sycldevice | FileCheck %s
+// RUN: %clang_cc1 -ast-print -fsycl-is-device %s -o - | FileCheck %s
 
 template 
 void WrappedInTemplate(T t) {


Index: clang/test/AST/ast-print-sycl-unique-stable-name.cpp
===
--- clang/test/AST/ast-print-sycl-unique-stable-name.cpp
+++ clang/test/AST/ast-print-sycl-unique-stable-name.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -ast-print -fsycl-is-device %s -o - -triple spir64-sycldevice | FileCheck %s
+// RUN: %clang_cc1 -ast-print -fsycl-is-device %s -o - | FileCheck %s
 
 template 
 void WrappedInTemplate(T t) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28080: [Docs][OpenCL] Added OpenCL feature description to user manual.

[Alexey Bader via Phabricator via cfe-commits]

bader closed this revision.
bader added a comment.
Herald added subscribers: ebevhan, arphaman.

Closed by https://reviews.llvm.org/rG18e165f50d8c1ab3afe7098dc00557d5f1a43cfa.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D28080/new/

https://reviews.llvm.org/D28080

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102261: Introduce SYCL 2020 mode

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

Sorry for the delay.
LGTM. Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102261/new/

https://reviews.llvm.org/D102261

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100396: [SYCL] Enable `opencl_global_[host,device]` attributes for SYCL

[Alexey Bader via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2ab513cd3e06: [SYCL] Enable `opencl_global_[host,device]` 
attributes for SYCL (authored by bader).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100396/new/

https://reviews.llvm.org/D100396

Files:
  clang/docs/SYCLSupport.rst
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/AddressSpaces.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/X86.h
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp
  clang/test/SemaTemplate/address_space-dependent.cpp

Index: clang/test/SemaTemplate/address_space-dependent.cpp
===
--- clang/test/SemaTemplate/address_space-dependent.cpp
+++ clang/test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
 
 template 
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388590)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388588)}}
 }
 
 template 
@@ -101,7 +101,7 @@
   car<1, 2, 3>(); // expected-note {{in instantiation of function template specialization 'car<1, 2, 3>' requested here}}
   HasASTemplateFields<1> HASTF;
   neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
-  correct<0x7FFFED>();
+  correct<0x7FFFEB>();
   tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650L>' requested here}}
 
   __attribute__((address_space(1))) char *x;
Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- clang/test/SemaSYCL/address-space-conversions.cpp
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -61,4 +61,17 @@
   void *v = GLOB;
   (void)i;
   (void)v;
+
+  __attribute__((opencl_global_host)) int *GLOB_HOST;
+  bar(*GLOB_HOST);
+  bar2(*GLOB_HOST);
+  GLOB = GLOB_HOST;
+  GLOB_HOST = GLOB; // expected-error {{assigning '__global int *' to '__global_host int *' changes address space of pointer}}
+  GLOB_HOST = static_cast<__attribute__((opencl_global_host)) int *>(GLOB); // expected-error {{static_cast from '__global int *' to '__global_host int *' is not allowed}}
+  __attribute__((opencl_global_device)) int *GLOB_DEVICE;
+  bar(*GLOB_DEVICE);
+  bar2(*GLOB_DEVICE);
+  GLOB = GLOB_DEVICE;
+  GLOB_DEVICE = GLOB; // expected-error {{assigning '__global int *' to '__global_device int *' changes address space of pointer}}
+  GLOB_DEVICE = static_cast<__attribute__((opencl_global_device)) int *>(GLOB); // expected-error {{static_cast from '__global int *' to '__global_device int *' is not allowed}}
 }
Index: clang/test/CodeGenSYCL/address-space-conversions.cpp
===
--- clang/test/CodeGenSYCL/address-space-conversions.cpp
+++ clang/test/CodeGenSYCL/address-space-conversions.cpp
@@ -29,6 +29,10 @@
   // CHECK-DAG: [[PRIV:%[a-zA-Z0-9]+]] = alloca i32*
   // CHECK-DAG: [[PRIV]].ascast = addrspacecast i32** [[PRIV]] to i32* addrspace(4)*
   __attribute__((opencl_private)) int *PRIV;
+  // CHECK-DAG: [[GLOB_DEVICE:%[a-zA-Z0-9]+]] = alloca i32 addrspace(5)*
+  __attribute__((opencl_global_device)) int *GLOBDEVICE;
+  // CHECK-DAG: [[GLOB_HOST:%[a-zA-Z0-9]+]] = alloca i32 addrspace(6)*
+  __attribute__((opencl_global_host)) int *GLOBHOST;
 
   // Explicit conversions
   // From names address spaces to default address space
@@ -57,6 +61,15 @@
   // CHECK-DAG: [[NoAS_CAST:%[a-zA-Z0-9]+]] = addrspacecast i32 addrspace(4)* [[NoAS_LOAD]] to i32*
   // CHECK-DAG: store i32* [[NoAS_CAST]], i32* addrspace(4)* [[PRIV]].ascast
   PRIV = (__attribute__((opencl_private)) int *)NoAS;
+  // From opencl_global_[host/device] address spaces to opencl_global
+  // CHECK-DAG: [[GLOBDEVICE_LOAD:%[a-zA-Z0-9]+]] = load i32 addrspace(5)*, i32 addrspace(5)* addrspace(4)* [[GLOB_DEVICE]].ascast
+  // CHECK-DAG: [[GLOBDEVICE_CAST:%[a-zA-Z0-9]+]] = addrspacecast i32 addrspace(5)* [[GLOBDEVICE_LOAD]] to i32 addrspace(1)*
+  // CHECK-DAG: store i32 addrspace(1)* [[GLOBDEVICE_CAST]], i32 addrspace(1)* addrspace(4)* [[GLOB]].ascast
+  GLOB = (__attribute__((opencl_global)) int *)GLOBDEVICE;
+  // CHECK-DAG: [[GLOBHOST_LOAD:%[a-zA-Z0-9]+]] = load i32 addrspace(6)*, i32 addrspace(6)* addrspace(4)* [[GLOB_HOST]].ascast
+  // CHECK-DAG: [[GLOBHOST_CAST:%[a-zA-Z0-9]+]] = addrspacecast i32 addrspace(6)* [[GLOBHOST_LOAD]] to i32 addrspace(

[PATCH] D100396: [SYCL] Enable `opencl_global_[host,device]` attributes for SYCL

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 345806.
bader marked an inline comment as done.
bader added a comment.

Added documentation to cover conversion rules and rebased on ToT.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100396/new/

https://reviews.llvm.org/D100396

Files:
  clang/docs/SYCLSupport.rst
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/AddressSpaces.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/X86.h
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp
  clang/test/SemaTemplate/address_space-dependent.cpp

Index: clang/test/SemaTemplate/address_space-dependent.cpp
===
--- clang/test/SemaTemplate/address_space-dependent.cpp
+++ clang/test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
 
 template 
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388590)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388588)}}
 }
 
 template 
@@ -101,7 +101,7 @@
   car<1, 2, 3>(); // expected-note {{in instantiation of function template specialization 'car<1, 2, 3>' requested here}}
   HasASTemplateFields<1> HASTF;
   neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
-  correct<0x7FFFED>();
+  correct<0x7FFFEB>();
   tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650L>' requested here}}
 
   __attribute__((address_space(1))) char *x;
Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- clang/test/SemaSYCL/address-space-conversions.cpp
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -61,4 +61,17 @@
   void *v = GLOB;
   (void)i;
   (void)v;
+
+  __attribute__((opencl_global_host)) int *GLOB_HOST;
+  bar(*GLOB_HOST);
+  bar2(*GLOB_HOST);
+  GLOB = GLOB_HOST;
+  GLOB_HOST = GLOB; // expected-error {{assigning '__global int *' to '__global_host int *' changes address space of pointer}}
+  GLOB_HOST = static_cast<__attribute__((opencl_global_host)) int *>(GLOB); // expected-error {{static_cast from '__global int *' to '__global_host int *' is not allowed}}
+  __attribute__((opencl_global_device)) int *GLOB_DEVICE;
+  bar(*GLOB_DEVICE);
+  bar2(*GLOB_DEVICE);
+  GLOB = GLOB_DEVICE;
+  GLOB_DEVICE = GLOB; // expected-error {{assigning '__global int *' to '__global_device int *' changes address space of pointer}}
+  GLOB_DEVICE = static_cast<__attribute__((opencl_global_device)) int *>(GLOB); // expected-error {{static_cast from '__global int *' to '__global_device int *' is not allowed}}
 }
Index: clang/test/CodeGenSYCL/address-space-conversions.cpp
===
--- clang/test/CodeGenSYCL/address-space-conversions.cpp
+++ clang/test/CodeGenSYCL/address-space-conversions.cpp
@@ -29,6 +29,10 @@
   // CHECK-DAG: [[PRIV:%[a-zA-Z0-9]+]] = alloca i32*
   // CHECK-DAG: [[PRIV]].ascast = addrspacecast i32** [[PRIV]] to i32* addrspace(4)*
   __attribute__((opencl_private)) int *PRIV;
+  // CHECK-DAG: [[GLOB_DEVICE:%[a-zA-Z0-9]+]] = alloca i32 addrspace(5)*
+  __attribute__((opencl_global_device)) int *GLOBDEVICE;
+  // CHECK-DAG: [[GLOB_HOST:%[a-zA-Z0-9]+]] = alloca i32 addrspace(6)*
+  __attribute__((opencl_global_host)) int *GLOBHOST;
 
   // Explicit conversions
   // From names address spaces to default address space
@@ -57,6 +61,15 @@
   // CHECK-DAG: [[NoAS_CAST:%[a-zA-Z0-9]+]] = addrspacecast i32 addrspace(4)* [[NoAS_LOAD]] to i32*
   // CHECK-DAG: store i32* [[NoAS_CAST]], i32* addrspace(4)* [[PRIV]].ascast
   PRIV = (__attribute__((opencl_private)) int *)NoAS;
+  // From opencl_global_[host/device] address spaces to opencl_global
+  // CHECK-DAG: [[GLOBDEVICE_LOAD:%[a-zA-Z0-9]+]] = load i32 addrspace(5)*, i32 addrspace(5)* addrspace(4)* [[GLOB_DEVICE]].ascast
+  // CHECK-DAG: [[GLOBDEVICE_CAST:%[a-zA-Z0-9]+]] = addrspacecast i32 addrspace(5)* [[GLOBDEVICE_LOAD]] to i32 addrspace(1)*
+  // CHECK-DAG: store i32 addrspace(1)* [[GLOBDEVICE_CAST]], i32 addrspace(1)* addrspace(4)* [[GLOB]].ascast
+  GLOB = (__attribute__((opencl_global)) int *)GLOBDEVICE;
+  // CHECK-DAG: [[GLOBHOST_LOAD:%[a-zA-Z0-9]+]] = load i32 addrspace(6)*, i32 addrspace(6)* addrspace(4)* [[GLOB_HOST]].ascast
+  // CHECK-DAG: [[GLOBHOST_CAST:%[a-zA-Z0-9]+]] = addrspacecast i32 addrspace(6)* [[GLOBHOST_LOAD]] to i32 addrspace(1)*
+  // CHECK-DAG: store i32 addrspace(1)* [[GLOBHOST_CAST]], i32 addrsp

[PATCH] D100396: [SYCL] Enable `opencl_global_[host,device]` attributes for SYCL

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 344399.
bader marked an inline comment as done.
bader added a comment.

Added explicit cast checks to Sema tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100396/new/

https://reviews.llvm.org/D100396

Files:
  clang/docs/SYCLSupport.rst
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/AddressSpaces.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/X86.h
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp
  clang/test/SemaTemplate/address_space-dependent.cpp

Index: clang/test/SemaTemplate/address_space-dependent.cpp
===
--- clang/test/SemaTemplate/address_space-dependent.cpp
+++ clang/test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
 
 template 
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388590)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388588)}}
 }
 
 template 
@@ -101,7 +101,7 @@
   car<1, 2, 3>(); // expected-note {{in instantiation of function template specialization 'car<1, 2, 3>' requested here}}
   HasASTemplateFields<1> HASTF;
   neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
-  correct<0x7FFFED>();
+  correct<0x7FFFEB>();
   tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650>' requested here}}
 
   __attribute__((address_space(1))) char *x;
Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- clang/test/SemaSYCL/address-space-conversions.cpp
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -61,4 +61,17 @@
   void *v = GLOB;
   (void)i;
   (void)v;
+
+  __attribute__((opencl_global_host)) int *GLOB_HOST;
+  bar(*GLOB_HOST);
+  bar2(*GLOB_HOST);
+  GLOB = GLOB_HOST;
+  GLOB_HOST = GLOB; // expected-error {{assigning '__global int *' to '__global_host int *' changes address space of pointer}}
+  GLOB_HOST = static_cast<__attribute__((opencl_global_host)) int *>(GLOB); // expected-error {{static_cast from '__global int *' to '__global_host int *' is not allowed}}
+  __attribute__((opencl_global_device)) int *GLOB_DEVICE;
+  bar(*GLOB_DEVICE);
+  bar2(*GLOB_DEVICE);
+  GLOB = GLOB_DEVICE;
+  GLOB_DEVICE = GLOB; // expected-error {{assigning '__global int *' to '__global_device int *' changes address space of pointer}}
+  GLOB_DEVICE = static_cast<__attribute__((opencl_global_device)) int *>(GLOB); // expected-error {{static_cast from '__global int *' to '__global_device int *' is not allowed}}
 }
Index: clang/test/CodeGenSYCL/address-space-conversions.cpp
===
--- clang/test/CodeGenSYCL/address-space-conversions.cpp
+++ clang/test/CodeGenSYCL/address-space-conversions.cpp
@@ -29,6 +29,10 @@
   // CHECK-DAG: [[PRIV:%[a-zA-Z0-9]+]] = alloca i32*
   // CHECK-DAG: [[PRIV]].ascast = addrspacecast i32** [[PRIV]] to i32* addrspace(4)*
   __attribute__((opencl_private)) int *PRIV;
+  // CHECK-DAG: [[GLOB_DEVICE:%[a-zA-Z0-9]+]] = alloca i32 addrspace(5)*
+  __attribute__((opencl_global_device)) int *GLOBDEVICE;
+  // CHECK-DAG: [[GLOB_HOST:%[a-zA-Z0-9]+]] = alloca i32 addrspace(6)*
+  __attribute__((opencl_global_host)) int *GLOBHOST;
 
   // Explicit conversions
   // From names address spaces to default address space
@@ -57,6 +61,15 @@
   // CHECK-DAG: [[NoAS_CAST:%[a-zA-Z0-9]+]] = addrspacecast i32 addrspace(4)* [[NoAS_LOAD]] to i32*
   // CHECK-DAG: store i32* [[NoAS_CAST]], i32* addrspace(4)* [[PRIV]].ascast
   PRIV = (__attribute__((opencl_private)) int *)NoAS;
+  // From opencl_global_[host/device] address spaces to opencl_global
+  // CHECK-DAG: [[GLOBDEVICE_LOAD:%[a-zA-Z0-9]+]] = load i32 addrspace(5)*, i32 addrspace(5)* addrspace(4)* [[GLOB_DEVICE]].ascast
+  // CHECK-DAG: [[GLOBDEVICE_CAST:%[a-zA-Z0-9]+]] = addrspacecast i32 addrspace(5)* [[GLOBDEVICE_LOAD]] to i32 addrspace(1)*
+  // CHECK-DAG: store i32 addrspace(1)* [[GLOBDEVICE_CAST]], i32 addrspace(1)* addrspace(4)* [[GLOB]].ascast
+  GLOB = (__attribute__((opencl_global)) int *)GLOBDEVICE;
+  // CHECK-DAG: [[GLOBHOST_LOAD:%[a-zA-Z0-9]+]] = load i32 addrspace(6)*, i32 addrspace(6)* addrspace(4)* [[GLOB_HOST]].ascast
+  // CHECK-DAG: [[GLOBHOST_CAST:%[a-zA-Z0-9]+]] = addrspacecast i32 addrspace(6)* [[GLOBHOST_LOAD]] to i32 addrspace(1)*
+  // CHECK-DAG: store i32 addrspace(1)* [[GLOBHOST_CAST]], i32 addrspace(1)* addrspace(4)* [[G

[PATCH] D100396: [SYCL] Enable `opencl_global_[host,device]` attributes for SYCL

[Alexey Bader via Phabricator via cfe-commits]

bader marked 3 inline comments as done.
bader added inline comments.



Comment at: clang/test/SemaSYCL/address-space-conversions.cpp:74
+  GLOB = GLOB_DEVICE;
+  GLOB_DEVICE = GLOB; // expected-error {{assigning '__global int *' to 
'__global_device int *' changes address space of pointer}}
 }

Anastasia wrote:
> Let's add explicit casts too.
Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100396/new/

https://reviews.llvm.org/D100396

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100396: [SYCL] Enable `opencl_global_[host,device]` attributes for SYCL

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 344393.
bader added a comment.

Apply code review suggestions and rebase on ToT.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100396/new/

https://reviews.llvm.org/D100396

Files:
  clang/docs/SYCLSupport.rst
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/AddressSpaces.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/X86.h
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp
  clang/test/SemaTemplate/address_space-dependent.cpp

Index: clang/test/SemaTemplate/address_space-dependent.cpp
===
--- clang/test/SemaTemplate/address_space-dependent.cpp
+++ clang/test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
 
 template 
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388590)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388588)}}
 }
 
 template 
@@ -101,7 +101,7 @@
   car<1, 2, 3>(); // expected-note {{in instantiation of function template specialization 'car<1, 2, 3>' requested here}}
   HasASTemplateFields<1> HASTF;
   neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
-  correct<0x7FFFED>();
+  correct<0x7FFFEB>();
   tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650>' requested here}}
 
   __attribute__((address_space(1))) char *x;
Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- clang/test/SemaSYCL/address-space-conversions.cpp
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -61,4 +61,15 @@
   void *v = GLOB;
   (void)i;
   (void)v;
+
+  __attribute__((opencl_global_host)) int *GLOB_HOST;
+  bar(*GLOB_HOST);
+  bar2(*GLOB_HOST);
+  GLOB = GLOB_HOST;
+  GLOB_HOST = GLOB; // expected-error {{assigning '__global int *' to '__global_host int *' changes address space of pointer}}
+  __attribute__((opencl_global_device)) int *GLOB_DEVICE;
+  bar(*GLOB_DEVICE);
+  bar2(*GLOB_DEVICE);
+  GLOB = GLOB_DEVICE;
+  GLOB_DEVICE = GLOB; // expected-error {{assigning '__global int *' to '__global_device int *' changes address space of pointer}}
 }
Index: clang/test/CodeGenSYCL/address-space-conversions.cpp
===
--- clang/test/CodeGenSYCL/address-space-conversions.cpp
+++ clang/test/CodeGenSYCL/address-space-conversions.cpp
@@ -29,6 +29,10 @@
   // CHECK-DAG: [[PRIV:%[a-zA-Z0-9]+]] = alloca i32*
   // CHECK-DAG: [[PRIV]].ascast = addrspacecast i32** [[PRIV]] to i32* addrspace(4)*
   __attribute__((opencl_private)) int *PRIV;
+  // CHECK-DAG: [[GLOB_DEVICE:%[a-zA-Z0-9]+]] = alloca i32 addrspace(5)*
+  __attribute__((opencl_global_device)) int *GLOBDEVICE;
+  // CHECK-DAG: [[GLOB_HOST:%[a-zA-Z0-9]+]] = alloca i32 addrspace(6)*
+  __attribute__((opencl_global_host)) int *GLOBHOST;
 
   // Explicit conversions
   // From names address spaces to default address space
@@ -57,6 +61,15 @@
   // CHECK-DAG: [[NoAS_CAST:%[a-zA-Z0-9]+]] = addrspacecast i32 addrspace(4)* [[NoAS_LOAD]] to i32*
   // CHECK-DAG: store i32* [[NoAS_CAST]], i32* addrspace(4)* [[PRIV]].ascast
   PRIV = (__attribute__((opencl_private)) int *)NoAS;
+  // From opencl_global_[host/device] address spaces to opencl_global
+  // CHECK-DAG: [[GLOBDEVICE_LOAD:%[a-zA-Z0-9]+]] = load i32 addrspace(5)*, i32 addrspace(5)* addrspace(4)* [[GLOB_DEVICE]].ascast
+  // CHECK-DAG: [[GLOBDEVICE_CAST:%[a-zA-Z0-9]+]] = addrspacecast i32 addrspace(5)* [[GLOBDEVICE_LOAD]] to i32 addrspace(1)*
+  // CHECK-DAG: store i32 addrspace(1)* [[GLOBDEVICE_CAST]], i32 addrspace(1)* addrspace(4)* [[GLOB]].ascast
+  GLOB = (__attribute__((opencl_global)) int *)GLOBDEVICE;
+  // CHECK-DAG: [[GLOBHOST_LOAD:%[a-zA-Z0-9]+]] = load i32 addrspace(6)*, i32 addrspace(6)* addrspace(4)* [[GLOB_HOST]].ascast
+  // CHECK-DAG: [[GLOBHOST_CAST:%[a-zA-Z0-9]+]] = addrspacecast i32 addrspace(6)* [[GLOBHOST_LOAD]] to i32 addrspace(1)*
+  // CHECK-DAG: store i32 addrspace(1)* [[GLOBHOST_CAST]], i32 addrspace(1)* addrspace(4)* [[GLOB]].ascast
+  GLOB = (__attribute__((opencl_global)) int *)GLOBHOST;
 
   bar(*GLOB);
   // CHECK-DAG: [[GLOB_LOAD:%[a-zA-Z0-9]+]] = load i32 addrspace(1)*, i32 addrspace(1)* addrspace(4)* [[GLOB]].ascast
Index: clang/lib/Basic/Targets/X86.h
===
--- clang/lib/Basic/Targets/X86.h
+++ clang/lib/Basic/Targets/X86.h
@@ -3

[PATCH] D101549: [Doc] Fix sphynx warnings about wrong code-block format

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG76f84e772978: [Doc] Fix sphinx warnings about wrong 
code-block format (authored by bader).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101549/new/

https://reviews.llvm.org/D101549

Files:
  clang/docs/SYCLSupport.rst


Index: clang/docs/SYCLSupport.rst
===
--- clang/docs/SYCLSupport.rst
+++ clang/docs/SYCLSupport.rst
@@ -99,7 +99,7 @@
  - private_space
 
 
-.. code-block::
+.. code-block:: C++
+
+//TODO: add support for __attribute__((opencl_global_host)) and 
__attribute__((opencl_global_device)).
 
-   TODO: add support for `__attribute__((opencl_global_host))` and
-   `__attribute__((opencl_global_device))`.


Index: clang/docs/SYCLSupport.rst
===
--- clang/docs/SYCLSupport.rst
+++ clang/docs/SYCLSupport.rst
@@ -99,7 +99,7 @@
  - private_space
 
 
-.. code-block::
+.. code-block:: C++
+
+//TODO: add support for __attribute__((opencl_global_host)) and __attribute__((opencl_global_device)).
 
-   TODO: add support for `__attribute__((opencl_global_host))` and
-   `__attribute__((opencl_global_device))`.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80932: [SYCL] Make default address space a superset of OpenCL address spaces.

[Alexey Bader via Phabricator via cfe-commits]

bader abandoned this revision.
bader added a comment.
Herald added a subscriber: ldrumm.

Committed alternative version - https://reviews.llvm.org/D89909.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80932/new/

https://reviews.llvm.org/D80932

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99488: [SYCL][Doc] Add design document for SYCL mode

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

Thanks! I've uploaded this version to https://reviews.llvm.org/D101549.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99488/new/

https://reviews.llvm.org/D99488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101549: [Doc] Fix sphynx warnings about wrong code-block format

[Alexey Bader via Phabricator via cfe-commits]

bader created this revision.
bader added a reviewer: Anastasia.
Herald added a subscriber: ebevhan.
bader requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101549

Files:
  clang/docs/SYCLSupport.rst


Index: clang/docs/SYCLSupport.rst
===
--- clang/docs/SYCLSupport.rst
+++ clang/docs/SYCLSupport.rst
@@ -99,7 +99,7 @@
  - private_space
 
 
-.. code-block::
+.. code-block:: C++
+
+//TODO: add support for __attribute__((opencl_global_host)) and 
__attribute__((opencl_global_device)).
 
-   TODO: add support for `__attribute__((opencl_global_host))` and
-   `__attribute__((opencl_global_device))`.


Index: clang/docs/SYCLSupport.rst
===
--- clang/docs/SYCLSupport.rst
+++ clang/docs/SYCLSupport.rst
@@ -99,7 +99,7 @@
  - private_space
 
 
-.. code-block::
+.. code-block:: C++
+
+//TODO: add support for __attribute__((opencl_global_host)) and __attribute__((opencl_global_device)).
 
-   TODO: add support for `__attribute__((opencl_global_host))` and
-   `__attribute__((opencl_global_device))`.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99488: [SYCL][Doc] Add design document for SYCL mode

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D99488#2725435 , @Anastasia wrote:

> If I build docs now I get the following output:
>
>   llvm-project/build-doc/tools/clang/docs/SYCLSupport.rst:102: WARNING: Error 
> in "code-block" directive:
>   1 argument(s) required, 0 supplied.
>   
>   .. code-block::
>   
>  TODO: add support for `__attribute__((opencl_global_host))` and
>  `__attribute__((opencl_global_device))`.
>
> Is this something already being looked at?

It looks like it can be fixed by adding language parameter:

> .. code-block:: c++

Unfortunately, I can't verify this fix locally. I see other types of warnings, 
which are treated as errors.

tools/clang/docs/ClangCommandLineReference.rst:22:Duplicate explicit target 
name: "cmdoption-clang--prefix".

Does anyone know how to avoid this issue?
If no, @Anastasia, could you confirm that adding `c++` parameter fixes the 
warning, please? If it does, I can commit this fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99488/new/

https://reviews.llvm.org/D99488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99488: [SYCL][Doc] Add design document for SYCL mode

[Alexey Bader via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb52e69c42681: [SYCL][Doc] Add design document for SYCL mode 
(authored by bader).

Changed prior to commit:
  https://reviews.llvm.org/D99488?vs=340526&id=340538#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99488/new/

https://reviews.llvm.org/D99488

Files:
  clang/docs/SYCLSupport.rst

Index: clang/docs/SYCLSupport.rst
===
--- /dev/null
+++ clang/docs/SYCLSupport.rst
@@ -0,0 +1,105 @@
+=
+SYCL Compiler and Runtime architecture design
+=
+
+.. contents::
+   :local:
+
+Introduction
+
+
+This document describes the architecture of the SYCL compiler and runtime
+library. More details are provided in
+`external document `_\ ,
+which are going to be added to clang documentation in the future.
+
+Address space handling
+==
+
+The SYCL specification represents pointers to disjoint memory regions using C++
+wrapper classes on an accelerator to enable compilation with a standard C++
+toolchain and a SYCL compiler toolchain. Section 3.8.2 of SYCL 2020
+specification defines
+`memory model `_\ ,
+section 4.7.7 - `address space classes `_
+and section 5.9 covers `address space deduction `_.
+The SYCL specification allows two modes of address space deduction: "generic as
+default address space" (see section 5.9.3) and "inferred address space" (see
+section 5.9.4). Current implementation supports only "generic as default address
+space" mode.
+
+SYCL borrows its memory model from OpenCL however SYCL doesn't perform
+the address space qualifier inference as detailed in
+`OpenCL C v3.0 6.7.8 `_.
+
+The default address space is "generic-memory", which is a virtual address space
+that overlaps the global, local, and private address spaces. SYCL mode enables
+explicit conversions to/from the default address space from/to the address
+space-attributed type and implicit conversions from the address space-attributed
+type to the default address space. All named address spaces are disjoint and
+sub-sets of default address space.
+
+The SPIR target allocates SYCL namespace scope variables in the global address
+space.
+
+Pointers to default address space should get lowered into a pointer to a generic
+address space (or flat to reuse more general terminology). But depending on the
+allocation context, the default address space of a non-pointer type is assigned
+to a specific address space. This is described in
+`common address space deduction rules `_
+section.
+
+This is also in line with the behaviour of CUDA (`small example
+`_).
+
+``multi_ptr`` class implementation example:
+
+.. code-block:: C++
+
+   // check that SYCL mode is ON and we can use non-standard decorations
+   #if defined(__SYCL_DEVICE_ONLY__)
+   // GPU/accelerator implementation
+   template  class multi_ptr {
+ // DecoratedType applies corresponding address space attribute to the type T
+ // DecoratedType::type == "__attribute__((opencl_global)) T"
+ // See sycl/include/CL/sycl/access/access.hpp for more details
+ using pointer_t = typename DecoratedType::type *;
+
+ pointer_t m_Pointer;
+ public:
+ pointer_t get() { return m_Pointer; }
+ T& operator* () { return *reinterpret_cast(m_Pointer); }
+   }
+   #else
+   // CPU/host implementation
+   template  class multi_ptr {
+ T *m_Pointer; // regular undecorated pointer
+ public:
+ T *get() { return m_Pointer; }
+ T& operator* () { return *m_Pointer; }
+   }
+   #endif
+
+Depending on the compiler mode, ``multi_ptr`` will either decorate its internal
+data with the address space attribute or not.
+
+To utilize clang's existing functionality, we reuse the following OpenCL address
+space attributes for pointers:
+
+.. list-table::
+   :header-rows: 1
+
+   * - Address space attribute
+ - SYCL address_space enumeration
+   * - ``__attribute__((opencl_global))``
+ - global_space, constant_space
+   * - ``__attribute__((opencl_local))``
+ - local_space
+   * - ``__attribute__((opencl_private))``
+ - private_space
+
+
+.. code-block::
+
+   TODO: add support for `__attribut

[PATCH] D100396: [SYCL] Enable `opencl_global_[host,device]` attributes for SYCL

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 340528.
bader added a comment.

Rebase on ToT


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100396/new/

https://reviews.llvm.org/D100396

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/AddressSpaces.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/X86.h
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp
  clang/test/SemaTemplate/address_space-dependent.cpp

Index: clang/test/SemaTemplate/address_space-dependent.cpp
===
--- clang/test/SemaTemplate/address_space-dependent.cpp
+++ clang/test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
 
 template 
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388590)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388588)}}
 }
 
 template 
@@ -101,7 +101,7 @@
   car<1, 2, 3>(); // expected-note {{in instantiation of function template specialization 'car<1, 2, 3>' requested here}}
   HasASTemplateFields<1> HASTF;
   neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
-  correct<0x7FFFED>();
+  correct<0x7FFFEB>();
   tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650>' requested here}}
 
   __attribute__((address_space(1))) char *x;
Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- clang/test/SemaSYCL/address-space-conversions.cpp
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -61,4 +61,15 @@
   void *v = GLOB;
   (void)i;
   (void)v;
+
+  __attribute__((opencl_global_host)) int *GLOB_HOST;
+  bar(*GLOB_HOST);
+  bar2(*GLOB_HOST);
+  GLOB = GLOB_HOST;
+  GLOB_HOST = GLOB; // expected-error {{assigning '__global int *' to '__global_host int *' changes address space of pointer}}
+  __attribute__((opencl_global_device)) int *GLOB_DEVICE;
+  bar(*GLOB_DEVICE);
+  bar2(*GLOB_DEVICE);
+  GLOB = GLOB_DEVICE;
+  GLOB_DEVICE = GLOB; // expected-error {{assigning '__global int *' to '__global_device int *' changes address space of pointer}}
 }
Index: clang/test/CodeGenSYCL/address-space-conversions.cpp
===
--- clang/test/CodeGenSYCL/address-space-conversions.cpp
+++ clang/test/CodeGenSYCL/address-space-conversions.cpp
@@ -29,6 +29,10 @@
   // CHECK-DAG: [[PRIV:%[a-zA-Z0-9]+]] = alloca i32*
   // CHECK-DAG: [[PRIV]].ascast = addrspacecast i32** [[PRIV]] to i32* addrspace(4)*
   __attribute__((opencl_private)) int *PRIV;
+  // CHECK-DAG: [[GLOB_DEVICE:%[a-zA-Z0-9]+]] = alloca i32 addrspace(5)*
+  __attribute__((opencl_global_device)) int *GLOBDEVICE;
+  // CHECK-DAG: [[GLOB_HOST:%[a-zA-Z0-9]+]] = alloca i32 addrspace(6)*
+  __attribute__((opencl_global_host)) int *GLOBHOST;
 
   // Explicit conversions
   // From names address spaces to default address space
Index: clang/lib/Basic/Targets/X86.h
===
--- clang/lib/Basic/Targets/X86.h
+++ clang/lib/Basic/Targets/X86.h
@@ -36,6 +36,8 @@
 0,   // cuda_constant
 0,   // cuda_shared
 0,   // sycl_global
+0,   // sycl_global_device
+0,   // sycl_global_host
 0,   // sycl_local
 0,   // sycl_private
 270, // ptr32_sptr
Index: clang/lib/Basic/Targets/TCE.h
===
--- clang/lib/Basic/Targets/TCE.h
+++ clang/lib/Basic/Targets/TCE.h
@@ -42,8 +42,10 @@
 0, // cuda_device
 0, // cuda_constant
 0, // cuda_shared
-3, // sycl_global
-4, // sycl_local
+0, // sycl_global
+0, // sycl_global_device
+0, // sycl_global_host
+0, // sycl_local
 0, // sycl_private
 0, // ptr32_sptr
 0, // ptr32_uptr
Index: clang/lib/Basic/Targets/SPIR.h
===
--- clang/lib/Basic/Targets/SPIR.h
+++ clang/lib/Basic/Targets/SPIR.h
@@ -35,6 +35,8 @@
 0, // cuda_shared
 // SYCL address space values for this map are dummy
 0, // sycl_global
+0, // sycl_global_device
+0, // sycl_global_host
 0, // sycl_local
 0, // sycl_private
 0, // ptr32_sptr
@@ -56,6 +58,8 @@
 0, // cuda_constant
 0, // cuda_shared
 1, // sycl_global
+5, // sycl_global_device
+6, // sycl_global_host
 3, // sycl_local
 0, // sycl_private
 0, // ptr32_sptr
Index: clang/lib/Basic/Ta

[PATCH] D99488: [SYCL][Doc] Add address space handling section to SYCL documentation

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 340526.
bader added a comment.

Rebased patch to unblock commit.

This patch had a dependency on D99190 , which 
adds SYCLSupport.rst document.
To unblock commit of D99488 , I switched the 
order of these two patches.
Now D99488  add SYCLSupport.rst document with 
just two sections: "Introduction" and "Address space handling".
D99190  will be rebased on top of D99488 
 to add more content to the document.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99488/new/

https://reviews.llvm.org/D99488

Files:
  clang/docs/SYCLSupport.rst

Index: clang/docs/SYCLSupport.rst
===
--- /dev/null
+++ clang/docs/SYCLSupport.rst
@@ -0,0 +1,105 @@
+=
+SYCL Compiler and Runtime architecture design
+=
+
+.. contents::
+   :local:
+
+Introduction
+
+
+This document describes the architecture of the SYCL compiler and runtime
+library. More details are provided in 
+`external document `_\ ,
+which are going to be added to clang documentation in the future.
+
+Address space handling
+==
+
+The SYCL specification represents pointers to disjoint memory regions using C++
+wrapper classes on an accelerator to enable compilation with a standard C++
+toolchain and a SYCL compiler toolchain. Section 3.8.2 of SYCL 2020
+specification defines
+`memory model `_\ ,
+section 4.7.7 - `address space classes `_
+and section 5.9 covers `address space deduction `_.
+The SYCL specification allows two modes of address space deduction: "generic as
+default address space" (see section 5.9.3) and "inferred address space" (see
+section 5.9.4). Current implementation supports only "generic as default address
+space" mode.
+
+SYCL borrows its memory model from OpenCL however SYCL doesn't perform
+the address space qualifier inference as detailed in
+`OpenCL C v3.0 6.7.8 `_.
+
+The default address space is "generic-memory", which is a virtual address space
+that overlaps the global, local, and private address spaces. SYCL mode enables
+explicit conversions to/from the default address space from/to the address
+space-attributed type and implicit conversions from the address space-attributed
+type to the default address space. All named address spaces are disjoint and
+sub-sets of default address space.
+
+The SPIR target allocates SYCL namespace scope variables in the global address
+space.
+
+Pointers to default address space should get lowered into a pointer to a generic
+address space (or flat to reuse more general terminology). But depending on the
+allocation context, the default address space of a non-pointer type is assigned
+to a specific address space. This is described in
+`common address space deduction rules `_
+section.
+
+This is also in line with the behaviour of CUDA (`small example
+`_).
+
+``multi_ptr`` class implementation example:
+
+.. code-block:: C++
+
+   // check that SYCL mode is ON and we can use non-standard decorations
+   #if defined(__SYCL_DEVICE_ONLY__)
+   // GPU/accelerator implementation
+   template  class multi_ptr {
+ // DecoratedType applies corresponding address space attribute to the type T
+ // DecoratedType::type == "__attribute__((opencl_global)) T"
+ // See sycl/include/CL/sycl/access/access.hpp for more details
+ using pointer_t = typename DecoratedType::type *;
+
+ pointer_t m_Pointer;
+ public:
+ pointer_t get() { return m_Pointer; }
+ T& operator* () { return *reinterpret_cast(m_Pointer); }
+   }
+   #else
+   // CPU/host implementation
+   template  class multi_ptr {
+ T *m_Pointer; // regular undecorated pointer
+ public:
+ T *get() { return m_Pointer; }
+ T& operator* () { return *m_Pointer; }
+   }
+   #endif
+
+Depending on the compiler mode, ``multi_ptr`` will either decorate its internal
+data with the address space attribute or not.
+
+To utilize clang's existing functionality, we reuse the following OpenCL address
+space attributes for pointers:
+
+.. list-table::
+   :header-rows: 1
+
+   * - Address space attribute
+ - SY

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
bader marked an inline comment as done.
Closed by commit rG7818906ca134: [SYCL] Implement SYCL address space attributes 
handling (authored by bader).

Changed prior to commit:
  https://reviews.llvm.org/D89909?vs=339973&id=340505#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/AddressSpaces.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/CodeGenSYCL/address-space-deduction.cpp
  clang/test/CodeGenSYCL/address-space-mangling.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp
  clang/test/SemaTemplate/address_space-dependent.cpp

Index: clang/test/SemaTemplate/address_space-dependent.cpp
===
--- clang/test/SemaTemplate/address_space-dependent.cpp
+++ clang/test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
 
 template 
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388593)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388590)}}
 }
 
 template 
@@ -101,7 +101,7 @@
   car<1, 2, 3>(); // expected-note {{in instantiation of function template specialization 'car<1, 2, 3>' requested here}}
   HasASTemplateFields<1> HASTF;
   neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
-  correct<0x71>();
+  correct<0x7FFFED>();
   tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650>' requested here}}
 
   __attribute__((address_space(1))) char *x;
Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsycl-is-device -verify -fsyntax-only %s
+
+void bar(int &Data) {}
+void bar2(int &Data) {}
+void bar(__attribute__((opencl_private)) int &Data) {}
+void foo(int *Data) {}
+void foo2(int *Data) {}
+void foo(__attribute__((opencl_private)) int *Data) {}
+void baz(__attribute__((opencl_private)) int *Data) {} // expected-note {{candidate function not viable: cannot pass pointer to generic address space as a pointer to address space '__private' in 1st argument}}
+
+template 
+void tmpl(T *t) {}
+
+void usages() {
+  __attribute__((opencl_global)) int *GLOB;
+  __attribute__((opencl_private)) int *PRIV;
+  __attribute__((opencl_local)) int *LOC;
+  int *NoAS;
+
+  GLOB = PRIV; // expected-error {{assigning '__private int *' to '__global int *' changes address space of pointer}}
+  GLOB = LOC;  // expected-error {{assigning '__local int *' to '__global int *' changes address space of pointer}}
+  PRIV = static_cast<__attribute__((opencl_private)) int *>(GLOB); // expected-error {{static_cast from '__global int *' to '__private int *' is not allowed}}
+  PRIV = static_cast<__attribute__((opencl_private)) int *>(LOC);  // expected-error {{static_cast from '__local int *' to '__private int *' is not allowed}}
+  NoAS = GLOB + PRIV;  // expected-error {{invalid operands to binary expression ('__global int *' and '__private int *')}}
+  NoAS = GLOB + LOC;   // expected-error {{invalid operands to binary expression ('__global int *' and '__local int *')}}
+  NoAS += GLOB;// expected-error {{invalid operands to binary expression ('int *' and '__global int *')}}
+
+  bar(*GLOB);
+  bar2(*GLOB);
+
+  bar(*PRIV);
+  bar2(*PRIV);
+
+  bar(*NoAS);
+  bar2(*NoAS);
+
+  bar(*LOC);
+  bar2(*LOC);
+
+  foo(GLOB);
+  foo2(GLOB);
+  foo(PRIV);
+  foo2(PRIV);
+  foo(NoAS);
+  foo2(NoAS);
+  foo(LOC);
+  foo2(LOC);
+
+  tmpl(GLOB);
+  tmpl(PRIV);
+  tmpl(NoAS);
+  tmpl(LOC);
+
+  // Implicit casts to named address space are disallowed
+  baz(NoAS);   // expected-error {{no matching function for call to 'baz'}}
+  __attribute__((opencl_lo

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader marked 5 inline comments as done.
bader added a comment.

@Anastasia, I've updated https://reviews.llvm.org/D99488 and refactored 
`getStringLiteralAddressSpace` to handle non-string constants as well. Please, 
take a look.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99488: [SYCL][Doc] Add address space handling section to SYCL documentation

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 340011.
bader added a comment.

Incorporate https://reviews.llvm.org/D89909 review feedback.

Allow one way implicit conversion only for now.
>From named address space to default.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99488/new/

https://reviews.llvm.org/D99488

Files:
  clang/docs/SYCLSupport.rst


Index: clang/docs/SYCLSupport.rst
===
--- clang/docs/SYCLSupport.rst
+++ clang/docs/SYCLSupport.rst
@@ -219,3 +219,94 @@
 
 Additional details of kernel parameter passing may be found in the document
 `SYCL Kernel Parameter Handling and Array Support 
`_.
+
+Address space handling
+^^
+
+The SYCL specification represents pointers to disjoint memory regions using C++
+wrapper classes on an accelerator to enable compilation with a standard C++
+toolchain and a SYCL compiler toolchain. Section 3.8.2 of SYCL 2020
+specification defines
+`memory model 
`_\
 ,
+section 4.7.7 - `address space classes 
`_
+and section 5.9 covers `address space deduction 
`_.
+The SYCL specification allows two modes of address space deduction: "generic as
+default address space" (see section 5.9.3) and "inferred address space" (see
+section 5.9.4). Current implementation supports only "generic as default 
address
+space" mode.
+
+SYCL borrows its memory model from OpenCL however SYCL doesn't perform
+the address space qualifier inference as detailed in
+`OpenCL C v3.0 6.7.8 
`_.
+
+The default address space is "generic-memory", which is a virtual address space
+that overlaps the global, local, and private address spaces. SYCL mode enables
+explicit conversions to/from the default address space from/to the address
+space-attributed type and implicit conversions from the address 
space-attributed
+type to the default address space. All named address spaces are disjoint and
+sub-sets of default address space.
+
+The SPIR target allocates SYCL namespace scope variables in the global address
+space.
+
+Pointers to default address space should get lowered into a pointer to a 
generic
+address space (or flat to reuse more general terminology). But depending on the
+allocation context, the default address space of a non-pointer type is assigned
+to a specific address space. This is described in
+`common address space deduction rules 
`_
+section.
+
+This is also in line with the behaviour of CUDA (`small example
+`_).
+
+``multi_ptr`` class implementation example:
+
+.. code-block:: C++
+
+   // check that SYCL mode is ON and we can use non-standard decorations
+   #if defined(__SYCL_DEVICE_ONLY__)
+   // GPU/accelerator implementation
+   template  class multi_ptr {
+ // DecoratedType applies corresponding address space attribute to the 
type T
+ // DecoratedType::type == 
"__attribute__((opencl_global)) T"
+ // See sycl/include/CL/sycl/access/access.hpp for more details
+ using pointer_t = typename DecoratedType::type *;
+
+ pointer_t m_Pointer;
+ public:
+ pointer_t get() { return m_Pointer; }
+ T& operator* () { return *reinterpret_cast(m_Pointer); }
+   }
+   #else
+   // CPU/host implementation
+   template  class multi_ptr {
+ T *m_Pointer; // regular undecorated pointer
+ public:
+ T *get() { return m_Pointer; }
+ T& operator* () { return *m_Pointer; }
+   }
+   #endif
+
+Depending on the compiler mode, ``multi_ptr`` will either decorate its internal
+data with the address space attribute or not.
+
+To utilize clang's existing functionality, we reuse the following OpenCL 
address
+space attributes for pointers:
+
+.. list-table::
+   :header-rows: 1
+
+   * - Address space attribute
+ - SYCL address_space enumeration
+   * - ``__attribute__((opencl_global))``
+ - global_space, constant_space
+   * - ``__attribute__((opencl_local))``
+ - local_space
+   * - ``__attribute__((opencl_private))``
+ - private_space
+
+
+.. code-block::
+
+   TODO: add support for `__attribute__((opencl_global_host))` and
+   `__attribute__((opencl_global_device))`.


Index: clang/docs/SYCLSupport.rst
===
--- clang/docs/SYCLSupport.rst
+++ clang/docs/SYCLSupport.rst
@@ -219,3 +219,94 @@
 
 Additional details of kernel parameter passing may be found in the document

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 339973.
bader marked an inline comment as done.
bader added a comment.

Generalize getStringLiteralAddressSpace to GetGlobalConstantAddressSpace

Rebased on ToT.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/AddressSpaces.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/CodeGenSYCL/address-space-deduction.cpp
  clang/test/CodeGenSYCL/address-space-mangling.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp
  clang/test/SemaTemplate/address_space-dependent.cpp

Index: clang/test/SemaTemplate/address_space-dependent.cpp
===
--- clang/test/SemaTemplate/address_space-dependent.cpp
+++ clang/test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
 
 template 
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388593)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388590)}}
 }
 
 template 
@@ -101,7 +101,7 @@
   car<1, 2, 3>(); // expected-note {{in instantiation of function template specialization 'car<1, 2, 3>' requested here}}
   HasASTemplateFields<1> HASTF;
   neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
-  correct<0x71>();
+  correct<0x7FFFED>();
   tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650>' requested here}}
 
   __attribute__((address_space(1))) char *x;
Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsycl-is-device -verify -fsyntax-only %s
+
+void bar(int &Data) {}
+void bar2(int &Data) {}
+void bar(__attribute__((opencl_private)) int &Data) {}
+void foo(int *Data) {}
+void foo2(int *Data) {}
+void foo(__attribute__((opencl_private)) int *Data) {}
+void baz(__attribute__((opencl_private)) int *Data) {} // expected-note {{candidate function not viable: cannot pass pointer to generic address space as a pointer to address space '__private' in 1st argument}}
+
+template 
+void tmpl(T *t) {}
+
+void usages() {
+  __attribute__((opencl_global)) int *GLOB;
+  __attribute__((opencl_private)) int *PRIV;
+  __attribute__((opencl_local)) int *LOC;
+  int *NoAS;
+
+  GLOB = PRIV; // expected-error {{assigning '__private int *' to '__global int *' changes address space of pointer}}
+  GLOB = LOC; // expected-error {{assigning '__local int *' to '__global int *' changes address space of pointer}}
+  PRIV = static_cast<__attribute__((opencl_private)) int *>(GLOB); // expected-error {{static_cast from '__global int *' to '__private int *' is not allowed}}
+  PRIV = static_cast<__attribute__((opencl_private)) int *>(LOC); // expected-error {{static_cast from '__local int *' to '__private int *' is not allowed}}
+  NoAS = GLOB + PRIV; // expected-error {{invalid operands to binary expression ('__global int *' and '__private int *')}}
+  NoAS = GLOB + LOC; // expected-error {{invalid operands to binary expression ('__global int *' and '__local int *')}}
+  NoAS += GLOB; // expected-error {{invalid operands to binary expression ('int *' and '__global int *')}}
+
+  bar(*GLOB);
+  bar2(*GLOB);
+
+  bar(*PRIV);
+  bar2(*PRIV);
+
+  bar(*NoAS);
+  bar2(*NoAS);
+
+  bar(*LOC);
+  bar2(*LOC);
+
+  foo(GLOB);
+  foo2(GLOB);
+  foo(PRIV);
+  foo2(PRIV);
+  foo(NoAS);
+  foo2(NoAS);
+  foo(LOC);
+  foo2(LOC);
+
+  tmpl(GLOB);
+  tmpl(PRIV);
+  tmpl(NoAS);
+  tmpl(LOC);
+
+  // Implicit casts to named address space are disallowed
+  baz(NoAS); // expected-error {{no matching function for call to 'baz'}}
+  __attribute__((opencl_local)) int *l = NoAS; // expected-error {{cannot initialize a variable of type '__local int *' with an lvalue of type 'int *'}}
+
+  (void)static_cast(GLOB);
+  (void)static_cast(GLOB);
+  int *i = GLOB;
+  void *v = GLOB;
+  (void)i;
+  (void)v;
+}
Index: clang/test/CodeGenSYCL/address-space-mangling.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/address-space-mangling.cpp
@@ -0,

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader marked 2 inline comments as done.
bader added inline comments.



Comment at: clang/include/clang/AST/Type.h:493
+   // Default is a superset of SYCL address spaces.
+   (A == LangAS::Default &&
+(B == LangAS::sycl_private || B == LangAS::sycl_local ||

Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > bader wrote:
> > > > Anastasia wrote:
> > > > > Ok if you allow implicit conversions both ways then this condition 
> > > > > should be extended to also contain all named address spaces in `A` 
> > > > > and `Default` in `B`. But actually, could you simplify by checking 
> > > > > that you have `Default` on either side, so something like 
> > > > > 
> > > > > 
> > > > > ```
> > > > > (A == LangAS::Default || B == LangAS::Default)
> > > > > ```
> > > > > ?
> > > > > Ok if you allow implicit conversions both ways then this condition 
> > > > > should be extended to also contain all named address spaces in `A` 
> > > > > and `Default` in `B`. But actually, could you simplify by checking 
> > > > > that you have `Default` on either side, so something like 
> > > > > 
> > > > > 
> > > > > ```
> > > > > (A == LangAS::Default || B == LangAS::Default)
> > > > > ```
> > > > > ?
> > > > 
> > > > According to the comment above `isAddressSpaceSupersetOf` function 
> > > > definition.
> > > > ```
> > > >   /// Returns true if address space A is equal to or a superset of B.
> > > > ```
> > > > 
> > > > `(A == LangAS::Default || B == LangAS::Default)` <- this change makes 
> > > > `Default` address space a superset of all address spaces including 
> > > > OpenCL, which we were trying to avoid with adding SYCL address spaces. 
> > > > Another problem with this code is that make `Default` a **sub-set** of 
> > > > named address spaces (like `sycl_local`), which is not right.
> > > > If I understand it correctly defining "isSupersSetOf" relation is 
> > > > enough for the rest of framework to enable conversions. Am I right?
> > > > (A == LangAS::Default || B == LangAS::Default) <- this change makes 
> > > > Default address space a superset of all address spaces including OpenCL.
> > > 
> > > I see, yes this will break pretty much everything unless we guard by SYCL 
> > > mode. But I don't think it is good to go this route though.
> > > 
> > > > Another problem with this code is that make Default a sub-set of named 
> > > > address spaces (like sycl_local), which is not right.
> > > 
> > > Well, if you need implicit conversions to work both ways as you have 
> > > written in the documentation then you don't really have a true 
> > > super-/subsets between the named address spaces and the default one. They 
> > > appear to be equivalent.
> > > 
> > > ```
> > > SYCL mode enables both explicit and implicit conversion to/from the 
> > > default address space from/to
> > > the address space-attributed type.
> > > ```
> > > 
> > > So do you actually need something like this to work?
> > > 
> > > ```
> > > int * genptr = ...;
> > > __private int * privptr = genptr:
> > > ```
> > > 
> > > 
> > I looked though the code base and I see that explicit cast is used when raw 
> > pointer is casted to address space annotated type. I think we can always 
> > use explicit cast from `Default` to named address space instead of implicit 
> > cast. It might be even useful to avoid unintended implicit casts causing UB.
> > @keryell, @Naghasan, what do you think if we update 
> > https://reviews.llvm.org/D99488 to disallow implicit casts from `Default` 
> > to named address space? I think it should be okay considering that current 
> > implementation doesn't use this type of casts (and I can't come up with a 
> > use case for it).
> > 
> > Meanwhile I've added checks for that to 
> > clang/test/SemaSYCL/address-space-conversions.cpp.
> Do you still plan to wait for extra input or otherwise we could just update 
> the documentation for now?
> 
> If you discover that you need to allow the reverse conversions later it 
> should not be problematic to add since it won't break anyone's code. It will 
> only allow more code to compile!
Okay. I'll update the document right away.



Comment at: clang/lib/Basic/Targets/SPIR.h:140
+// space must be compatible with the generic address space
+return LangAS::sycl_global;
+  }

Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > This needs a language guard too?
> > I can re-write this method to avoid using language address space:
> > 
> > ```
> >   llvm::Optional getConstantAddressSpace() const override {
> > return getLangASFromTargetAS(1);
> >   }
> > ```
> > 
> > Does it look okay to you?
> > 
> > I don't think we need a language guard here as this hook is already guarded 
> > by users. E.g. 
> > https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenModule.cpp#L4137-L4159.
> > Adding language guards for `TargetInfo::getConstantAddressSpace` method 
> > will require API c

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader marked an inline comment as done.
bader added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:140
+// space must be compatible with the generic address space
+return LangAS::sycl_global;
+  }

Anastasia wrote:
> This needs a language guard too?
I can re-write this method to avoid using language address space:

```
  llvm::Optional getConstantAddressSpace() const override {
return getLangASFromTargetAS(1);
  }
```

Does it look okay to you?

I don't think we need a language guard here as this hook is already guarded by 
users. E.g. 
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenModule.cpp#L4137-L4159.
Adding language guards for `TargetInfo::getConstantAddressSpace` method will 
require API change similar to `adjust` method i.e. explicit `LangOptions` type 
parameter.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 339515.
bader marked an inline comment as done.
bader added a comment.

Added SYCL address spaces mangling for targets without address space map


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/AddressSpaces.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/CodeGenSYCL/address-space-deduction.cpp
  clang/test/CodeGenSYCL/address-space-mangling.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp
  clang/test/SemaTemplate/address_space-dependent.cpp

Index: clang/test/SemaTemplate/address_space-dependent.cpp
===
--- clang/test/SemaTemplate/address_space-dependent.cpp
+++ clang/test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
 
 template 
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388593)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388590)}}
 }
 
 template 
@@ -101,7 +101,7 @@
   car<1, 2, 3>(); // expected-note {{in instantiation of function template specialization 'car<1, 2, 3>' requested here}}
   HasASTemplateFields<1> HASTF;
   neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
-  correct<0x71>();
+  correct<0x7FFFED>();
   tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650>' requested here}}
 
   __attribute__((address_space(1))) char *x;
Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsycl-is-device -verify -fsyntax-only %s
+
+void bar(int &Data) {}
+void bar2(int &Data) {}
+void bar(__attribute__((opencl_private)) int &Data) {}
+void foo(int *Data) {}
+void foo2(int *Data) {}
+void foo(__attribute__((opencl_private)) int *Data) {}
+void baz(__attribute__((opencl_private)) int *Data) {} // expected-note {{candidate function not viable: cannot pass pointer to generic address space as a pointer to address space '__private' in 1st argument}}
+
+template 
+void tmpl(T *t) {}
+
+void usages() {
+  __attribute__((opencl_global)) int *GLOB;
+  __attribute__((opencl_private)) int *PRIV;
+  __attribute__((opencl_local)) int *LOC;
+  int *NoAS;
+
+  GLOB = PRIV; // expected-error {{assigning '__private int *' to '__global int *' changes address space of pointer}}
+  GLOB = LOC; // expected-error {{assigning '__local int *' to '__global int *' changes address space of pointer}}
+  PRIV = static_cast<__attribute__((opencl_private)) int *>(GLOB); // expected-error {{static_cast from '__global int *' to '__private int *' is not allowed}}
+  PRIV = static_cast<__attribute__((opencl_private)) int *>(LOC); // expected-error {{static_cast from '__local int *' to '__private int *' is not allowed}}
+  NoAS = GLOB + PRIV; // expected-error {{invalid operands to binary expression ('__global int *' and '__private int *')}}
+  NoAS = GLOB + LOC; // expected-error {{invalid operands to binary expression ('__global int *' and '__local int *')}}
+  NoAS += GLOB; // expected-error {{invalid operands to binary expression ('int *' and '__global int *')}}
+
+  bar(*GLOB);
+  bar2(*GLOB);
+
+  bar(*PRIV);
+  bar2(*PRIV);
+
+  bar(*NoAS);
+  bar2(*NoAS);
+
+  bar(*LOC);
+  bar2(*LOC);
+
+  foo(GLOB);
+  foo2(GLOB);
+  foo(PRIV);
+  foo2(PRIV);
+  foo(NoAS);
+  foo2(NoAS);
+  foo(LOC);
+  foo2(LOC);
+
+  tmpl(GLOB);
+  tmpl(PRIV);
+  tmpl(NoAS);
+  tmpl(LOC);
+
+  // Implicit casts to named address space are disallowed
+  baz(NoAS); // expected-error {{no matching function for call to 'baz'}}
+  __attribute__((opencl_local)) int *l = NoAS; // expected-error {{cannot initialize a variable of type '__local int *' with an lvalue of type 'int *'}}
+
+  (void)static_cast(GLOB);
+  (void)static_cast(GLOB);
+  int *i = GLOB;
+  void *v = GLOB;
+  (void)i;
+  (void)v;
+}
Index: clang/test/CodeGenSYCL/address-space-mangling.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/address-space-mangling.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -triple spir64 -fsycl-is-device -disable-llvm-passes -emit-llvm %s -o - | FileCheck 

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 339484.
bader marked 7 inline comments as done.
bader added a comment.

Applied more review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/AddressSpaces.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/CodeGenSYCL/address-space-deduction.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp
  clang/test/SemaTemplate/address_space-dependent.cpp

Index: clang/test/SemaTemplate/address_space-dependent.cpp
===
--- clang/test/SemaTemplate/address_space-dependent.cpp
+++ clang/test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
 
 template 
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388593)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388590)}}
 }
 
 template 
@@ -101,7 +101,7 @@
   car<1, 2, 3>(); // expected-note {{in instantiation of function template specialization 'car<1, 2, 3>' requested here}}
   HasASTemplateFields<1> HASTF;
   neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
-  correct<0x71>();
+  correct<0x7FFFED>();
   tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650>' requested here}}
 
   __attribute__((address_space(1))) char *x;
Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsycl-is-device -verify -fsyntax-only %s
+
+void bar(int &Data) {}
+void bar2(int &Data) {}
+void bar(__attribute__((opencl_private)) int &Data) {}
+void foo(int *Data) {}
+void foo2(int *Data) {}
+void foo(__attribute__((opencl_private)) int *Data) {}
+void baz(__attribute__((opencl_private)) int *Data) {} // expected-note {{candidate function not viable: cannot pass pointer to generic address space as a pointer to address space '__private' in 1st argument}}
+
+template 
+void tmpl(T *t) {}
+
+void usages() {
+  __attribute__((opencl_global)) int *GLOB;
+  __attribute__((opencl_private)) int *PRIV;
+  __attribute__((opencl_local)) int *LOC;
+  int *NoAS;
+
+  GLOB = PRIV; // expected-error {{assigning '__private int *' to '__global int *' changes address space of pointer}}
+  GLOB = LOC; // expected-error {{assigning '__local int *' to '__global int *' changes address space of pointer}}
+  PRIV = static_cast<__attribute__((opencl_private)) int *>(GLOB); // expected-error {{static_cast from '__global int *' to '__private int *' is not allowed}}
+  PRIV = static_cast<__attribute__((opencl_private)) int *>(LOC); // expected-error {{static_cast from '__local int *' to '__private int *' is not allowed}}
+  NoAS = GLOB + PRIV; // expected-error {{invalid operands to binary expression ('__global int *' and '__private int *')}}
+  NoAS = GLOB + LOC; // expected-error {{invalid operands to binary expression ('__global int *' and '__local int *')}}
+  NoAS += GLOB; // expected-error {{invalid operands to binary expression ('int *' and '__global int *')}}
+
+  bar(*GLOB);
+  bar2(*GLOB);
+
+  bar(*PRIV);
+  bar2(*PRIV);
+
+  bar(*NoAS);
+  bar2(*NoAS);
+
+  bar(*LOC);
+  bar2(*LOC);
+
+  foo(GLOB);
+  foo2(GLOB);
+  foo(PRIV);
+  foo2(PRIV);
+  foo(NoAS);
+  foo2(NoAS);
+  foo(LOC);
+  foo2(LOC);
+
+  tmpl(GLOB);
+  tmpl(PRIV);
+  tmpl(NoAS);
+  tmpl(LOC);
+
+  // Implicit casts to named address space are disallowed
+  baz(NoAS); // expected-error {{no matching function for call to 'baz'}}
+  __attribute__((opencl_local)) int *l = NoAS; // expected-error {{cannot initialize a variable of type '__local int *' with an lvalue of type 'int *'}}
+
+  (void)static_cast(GLOB);
+  (void)static_cast(GLOB);
+  int *i = GLOB;
+  void *v = GLOB;
+  (void)i;
+  (void)v;
+}
Index: clang/test/CodeGenSYCL/address-space-deduction.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/address-space-deduction.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -triple spir64 -fsycl-is-device -disable-llvm-passes -emit-llvm %s -o - | FileCheck %s
+
+// CHECK:@_ZZ4testvE3foo = internal addrspace(1) constant i32 66, align 4
+// CHECK

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:9985
+ "Address space agnostic languages only");
+  LangAS DefaultGlobalAS = getLangASFromTargetAS(
+  CGM.getContext().getTargetAddressSpace(LangAS::sycl_global));

Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > bader wrote:
> > > > Anastasia wrote:
> > > > > bader wrote:
> > > > > > Anastasia wrote:
> > > > > > > bader wrote:
> > > > > > > > Anastasia wrote:
> > > > > > > > > bader wrote:
> > > > > > > > > > Anastasia wrote:
> > > > > > > > > > > Since you are using SYCL address space you should 
> > > > > > > > > > > probably guard this line by SYCL mode...  Btw the same 
> > > > > > > > > > > seems to apply to the code below as it implements SYCL 
> > > > > > > > > > > sematics?
> > > > > > > > > > > 
> > > > > > > > > > > Can you add spec references here too.
> > > > > > > > > > > 
> > > > > > > > > > > Also there seems to be nothing target specific in the 
> > > > > > > > > > > code here as you are implementing what is specified by 
> > > > > > > > > > > the language semantics. Should this not be moved to 
> > > > > > > > > > > `GetGlobalVarAddressSpace` along with the other language 
> > > > > > > > > > > handling?
> > > > > > > > > > > 
> > > > > > > > > > > I am not very familiar with this part of address space 
> > > > > > > > > > > handling though. I would be more comfortable if @rjmccall 
> > > > > > > > > > > could take a look too.
> > > > > > > > > > This code assigns target address space "global variables 
> > > > > > > > > > w/o address space attribute". 
> > > > > > > > > > SYCL says it's "implementation defined" (from 
> > > > > > > > > > https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace):
> > > > > > > > > > 
> > > > > > > > > > > Namespace scope
> > > > > > > > > > > If the type is const, the address space the declaration 
> > > > > > > > > > > is assigned to is implementation-defined. If the target 
> > > > > > > > > > > of the SYCL backend can represent the generic address 
> > > > > > > > > > > space, then the assigned address space must be compatible 
> > > > > > > > > > > with the generic address space.
> > > > > > > > > > > Namespace scope non-const declarations cannot be used 
> > > > > > > > > > > within a kernel, as restricted in Section 5.4. This means 
> > > > > > > > > > > that non-const global variables cannot be accessed by any 
> > > > > > > > > > > device kernel or code called by the device kernel.
> > > > > > > > > > 
> > > > > > > > > > I added clarification that SPIR target allocates global 
> > > > > > > > > > variables in global address space to 
> > > > > > > > > > https://reviews.llvm.org/D99488 (see line #248).
> > > > > > > > > > 
> > > > > > > > > > @rjmccall, mentioned in the mailing list discussion that 
> > > > > > > > > > this callbacks were developed for compiling C++ to AMDGPU 
> > > > > > > > > > target, so this not necessary designed only for SYCL, but 
> > > > > > > > > > it works for SYCL as well.
> > > > > > > > > After all what objects are allowed to bind to non-default 
> > > > > > > > > address space here is defined in SYCL spec even if the exact 
> > > > > > > > > address spaces are not defined so it is not completely a 
> > > > > > > > > target-specific behavior.
> > > > > > > > > 
> > > > > > > > > My understanding of the API you are extending (judging from 
> > > > > > > > > its use) is that it allows you to extend the language 
> > > > > > > > > sematics with some target-specific setup. I.e. you could add 
> > > > > > > > > extra address spaces to C++ or OpenCL or any other language. 
> > > > > > > > > But here you are setting the language address spaces instead 
> > > > > > > > > that are mapped to the target at some point implicitly.
> > > > > > > > > 
> > > > > > > > > It seems like this change better fits to 
> > > > > > > > > `CodeGenModule::GetGlobalVarAddressSpace` that already 
> > > > > > > > > contains very similar logic?
> > > > > > > > > 
> > > > > > > > > Otherwise, it makes more sense to use target address spaces 
> > > > > > > > > directly instead of SYCL language address spaces. But either 
> > > > > > > > > way, we should guard it by SYCL mode somehow as we have not 
> > > > > > > > > established this as a universal logic for SPIR. 
> > > > > > > > > It seems like this change better fits to 
> > > > > > > > > `CodeGenModule::GetGlobalVarAddressSpace` that already 
> > > > > > > > > contains very similar logic?
> > > > > > > > 
> > > > > > > > This was the original implementation (see 
> > > > > > > > https://reviews.llvm.org/D89909?id=299795), but @rjmccall 
> > > > > > > > suggested to use this callback instead.
> > > > > > > > Both ways work for me, but the implementation proposed by John 
> > > > > > > > is easier to maintain.
> > > > > > > > 
> > > > > > > > > Otherwise, it makes more sense to use target address spaces 
> > > > > > > > > dire

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:9985
+ "Address space agnostic languages only");
+  LangAS DefaultGlobalAS = getLangASFromTargetAS(
+  CGM.getContext().getTargetAddressSpace(LangAS::sycl_global));

Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > bader wrote:
> > > > Anastasia wrote:
> > > > > bader wrote:
> > > > > > Anastasia wrote:
> > > > > > > bader wrote:
> > > > > > > > Anastasia wrote:
> > > > > > > > > Since you are using SYCL address space you should probably 
> > > > > > > > > guard this line by SYCL mode...  Btw the same seems to apply 
> > > > > > > > > to the code below as it implements SYCL sematics?
> > > > > > > > > 
> > > > > > > > > Can you add spec references here too.
> > > > > > > > > 
> > > > > > > > > Also there seems to be nothing target specific in the code 
> > > > > > > > > here as you are implementing what is specified by the 
> > > > > > > > > language semantics. Should this not be moved to 
> > > > > > > > > `GetGlobalVarAddressSpace` along with the other language 
> > > > > > > > > handling?
> > > > > > > > > 
> > > > > > > > > I am not very familiar with this part of address space 
> > > > > > > > > handling though. I would be more comfortable if @rjmccall 
> > > > > > > > > could take a look too.
> > > > > > > > This code assigns target address space "global variables w/o 
> > > > > > > > address space attribute". 
> > > > > > > > SYCL says it's "implementation defined" (from 
> > > > > > > > https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace):
> > > > > > > > 
> > > > > > > > > Namespace scope
> > > > > > > > > If the type is const, the address space the declaration is 
> > > > > > > > > assigned to is implementation-defined. If the target of the 
> > > > > > > > > SYCL backend can represent the generic address space, then 
> > > > > > > > > the assigned address space must be compatible with the 
> > > > > > > > > generic address space.
> > > > > > > > > Namespace scope non-const declarations cannot be used within 
> > > > > > > > > a kernel, as restricted in Section 5.4. This means that 
> > > > > > > > > non-const global variables cannot be accessed by any device 
> > > > > > > > > kernel or code called by the device kernel.
> > > > > > > > 
> > > > > > > > I added clarification that SPIR target allocates global 
> > > > > > > > variables in global address space to 
> > > > > > > > https://reviews.llvm.org/D99488 (see line #248).
> > > > > > > > 
> > > > > > > > @rjmccall, mentioned in the mailing list discussion that this 
> > > > > > > > callbacks were developed for compiling C++ to AMDGPU target, so 
> > > > > > > > this not necessary designed only for SYCL, but it works for 
> > > > > > > > SYCL as well.
> > > > > > > After all what objects are allowed to bind to non-default address 
> > > > > > > space here is defined in SYCL spec even if the exact address 
> > > > > > > spaces are not defined so it is not completely a target-specific 
> > > > > > > behavior.
> > > > > > > 
> > > > > > > My understanding of the API you are extending (judging from its 
> > > > > > > use) is that it allows you to extend the language sematics with 
> > > > > > > some target-specific setup. I.e. you could add extra address 
> > > > > > > spaces to C++ or OpenCL or any other language. But here you are 
> > > > > > > setting the language address spaces instead that are mapped to 
> > > > > > > the target at some point implicitly.
> > > > > > > 
> > > > > > > It seems like this change better fits to 
> > > > > > > `CodeGenModule::GetGlobalVarAddressSpace` that already contains 
> > > > > > > very similar logic?
> > > > > > > 
> > > > > > > Otherwise, it makes more sense to use target address spaces 
> > > > > > > directly instead of SYCL language address spaces. But either way, 
> > > > > > > we should guard it by SYCL mode somehow as we have not 
> > > > > > > established this as a universal logic for SPIR. 
> > > > > > > It seems like this change better fits to 
> > > > > > > `CodeGenModule::GetGlobalVarAddressSpace` that already contains 
> > > > > > > very similar logic?
> > > > > > 
> > > > > > This was the original implementation (see 
> > > > > > https://reviews.llvm.org/D89909?id=299795), but @rjmccall suggested 
> > > > > > to use this callback instead.
> > > > > > Both ways work for me, but the implementation proposed by John is 
> > > > > > easier to maintain.
> > > > > > 
> > > > > > > Otherwise, it makes more sense to use target address spaces 
> > > > > > > directly instead of SYCL language address spaces. But either way, 
> > > > > > > we should guard it by SYCL mode somehow as we have not 
> > > > > > > established this as a universal logic for SPIR.
> > > > > > 
> > > > > > I've updated the code to use target address space. I also added an 
> > > > > > assertion for SYCL language mode, although I think SPIR doesn't 
> > > > > 

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:9985
+ "Address space agnostic languages only");
+  LangAS DefaultGlobalAS = getLangASFromTargetAS(
+  CGM.getContext().getTargetAddressSpace(LangAS::sycl_global));

Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > bader wrote:
> > > > Anastasia wrote:
> > > > > bader wrote:
> > > > > > Anastasia wrote:
> > > > > > > Since you are using SYCL address space you should probably guard 
> > > > > > > this line by SYCL mode...  Btw the same seems to apply to the 
> > > > > > > code below as it implements SYCL sematics?
> > > > > > > 
> > > > > > > Can you add spec references here too.
> > > > > > > 
> > > > > > > Also there seems to be nothing target specific in the code here 
> > > > > > > as you are implementing what is specified by the language 
> > > > > > > semantics. Should this not be moved to `GetGlobalVarAddressSpace` 
> > > > > > > along with the other language handling?
> > > > > > > 
> > > > > > > I am not very familiar with this part of address space handling 
> > > > > > > though. I would be more comfortable if @rjmccall could take a 
> > > > > > > look too.
> > > > > > This code assigns target address space "global variables w/o 
> > > > > > address space attribute". 
> > > > > > SYCL says it's "implementation defined" (from 
> > > > > > https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace):
> > > > > > 
> > > > > > > Namespace scope
> > > > > > > If the type is const, the address space the declaration is 
> > > > > > > assigned to is implementation-defined. If the target of the SYCL 
> > > > > > > backend can represent the generic address space, then the 
> > > > > > > assigned address space must be compatible with the generic 
> > > > > > > address space.
> > > > > > > Namespace scope non-const declarations cannot be used within a 
> > > > > > > kernel, as restricted in Section 5.4. This means that non-const 
> > > > > > > global variables cannot be accessed by any device kernel or code 
> > > > > > > called by the device kernel.
> > > > > > 
> > > > > > I added clarification that SPIR target allocates global variables 
> > > > > > in global address space to https://reviews.llvm.org/D99488 (see 
> > > > > > line #248).
> > > > > > 
> > > > > > @rjmccall, mentioned in the mailing list discussion that this 
> > > > > > callbacks were developed for compiling C++ to AMDGPU target, so 
> > > > > > this not necessary designed only for SYCL, but it works for SYCL as 
> > > > > > well.
> > > > > After all what objects are allowed to bind to non-default address 
> > > > > space here is defined in SYCL spec even if the exact address spaces 
> > > > > are not defined so it is not completely a target-specific behavior.
> > > > > 
> > > > > My understanding of the API you are extending (judging from its use) 
> > > > > is that it allows you to extend the language sematics with some 
> > > > > target-specific setup. I.e. you could add extra address spaces to C++ 
> > > > > or OpenCL or any other language. But here you are setting the 
> > > > > language address spaces instead that are mapped to the target at some 
> > > > > point implicitly.
> > > > > 
> > > > > It seems like this change better fits to 
> > > > > `CodeGenModule::GetGlobalVarAddressSpace` that already contains very 
> > > > > similar logic?
> > > > > 
> > > > > Otherwise, it makes more sense to use target address spaces directly 
> > > > > instead of SYCL language address spaces. But either way, we should 
> > > > > guard it by SYCL mode somehow as we have not established this as a 
> > > > > universal logic for SPIR. 
> > > > > It seems like this change better fits to 
> > > > > `CodeGenModule::GetGlobalVarAddressSpace` that already contains very 
> > > > > similar logic?
> > > > 
> > > > This was the original implementation (see 
> > > > https://reviews.llvm.org/D89909?id=299795), but @rjmccall suggested to 
> > > > use this callback instead.
> > > > Both ways work for me, but the implementation proposed by John is 
> > > > easier to maintain.
> > > > 
> > > > > Otherwise, it makes more sense to use target address spaces directly 
> > > > > instead of SYCL language address spaces. But either way, we should 
> > > > > guard it by SYCL mode somehow as we have not established this as a 
> > > > > universal logic for SPIR.
> > > > 
> > > > I've updated the code to use target address space. I also added an 
> > > > assertion for SYCL language mode, although I think SPIR doesn't support 
> > > > global variables in address spaces other than global or constant 
> > > > regardless of the language mode, so I think the logic is universal.
> > > > This was the original implementation (see 
> > > > https://reviews.llvm.org/D89909?id=299795), but @rjmccall suggested to 
> > > > use this callback instead.
> > > 
> > > Did you mean to link some particular conversation? Currently, i

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/AST/ItaniumMangle.cpp:2379
   unsigned TargetAS = Context.getASTContext().getTargetAddressSpace(AS);
-  if (TargetAS != 0)
+  if (TargetAS != 0 || Context.getASTContext().getLangOpts().SYCLIsDevice)
 ASString = "AS" + llvm::utostr(TargetAS);

Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > bader wrote:
> > > > Anastasia wrote:
> > > > > Any reason not to use OpenCL mangling? If you do then you might be 
> > > > > able to link against libraries compiled for OpenCL. Also you will get 
> > > > > more stable naming i.e. it would not differ from target to target. 
> > > > > Any reason not to use OpenCL mangling? If you do then you might be 
> > > > > able to link against libraries compiled for OpenCL. Also you will get 
> > > > > more stable naming i.e. it would not differ from target to target. 
> > > > 
> > > > I'm not sure I understand your suggestion. Could you elaborate on 
> > > > "OpenCL mangling", please?
> > > > 
> > > > Let me clarify the problem this change addresses. The test case 
> > > > covering it is located in 
> > > > `clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp` lines 
> > > > 86-91.
> > > > 
> > > > ```
> > > > template 
> > > > void tmpl(T t) {}
> > > > 
> > > > int *NoAS;
> > > > __attribute__((opencl_private)) int *PRIV;
> > > > 
> > > > tmpl(PRIV);
> > > > // CHECK-DAG: [[PRIV_LOAD5:%[a-zA-Z0-9]+]] = load i32*, i32* 
> > > > addrspace(4)* [[PRIV]].ascast
> > > > // CHECK-DAG: call spir_func void [[PRIV_TMPL:@[a-zA-Z0-9_]+]](i32* 
> > > > [[PRIV_LOAD5]])
> > > > tmpl(NoAS);
> > > > // CHECK-DAG: [[NoAS_LOAD5:%[a-zA-Z0-9]+]] = load i32 addrspace(4)*, 
> > > > i32 addrspace(4)* addrspace(4)* [[NoAS]].ascast
> > > > // CHECK-DAG: call spir_func void [[GEN_TMPL:@[a-zA-Z0-9_]+]](i32 
> > > > addrspace(4)* [[NoAS_LOAD5]])
> > > > ```
> > > > Clang has separate code paths for mangling types w/ and w/o address 
> > > > space attributes (i.e. using `Default` address space).
> > > > 
> > > > Address space is not mangled if there is no AS attribute (`Default`) or 
> > > > if address space attribute is maps to `0` target address space. SPIR 
> > > > target maps `*_private` address space to `0`, which causes name 
> > > > conflict for the example above.
> > > > 
> > > > This change for SYCL compiler enables mangling for non-default address 
> > > > space attributes regardless of their mapping to target address space.
> > > It's just that all language address spaces are mangled with the source 
> > > spelling in Italium ABI right now, if you check the `else` statement. I 
> > > don't think it is part of the official spec yet but it might be better to 
> > > stick to the same pattern if possible.
> > > It's just that all language address spaces are mangled with the source 
> > > spelling in Italium ABI right now, if you check the `else` statement. I 
> > > don't think it is part of the official spec yet but it might be better to 
> > > stick to the same pattern if possible.
> > 
> > I would really love to avoid changes to the mangler (e.g. to be able to 
> > link binaries produced by different front-end like SYCL/OpenCL/CUDA), but I 
> > don't know the better way to address the issue 
> > Sorry, I don't get what do you suggest here. Could you clarify what exactly 
> > I should change, please?
> For now I am just trying to understand why you are not adopting similar 
> mangling scheme as for other language address spaces since it gives more 
> stable mangling irrespective from the target compiled for.
> 
> If you plan to link libraries from other frontends i.e. OpenCL or CUDA the 
> mangling you use is different from what they produce. Just have a look at the 
>  line 2470 that explains OpenCL mangling or line 2494 explaining CUDA 
> mangling. FYI similar scheme applies to other language address spaces, so the 
> `AS` was only really used for the address spaces that have no source 
> spelling i.e. no language semantics.
> For now I am just trying to understand why you are not adopting similar 
> mangling scheme as for other language address spaces since it gives more 
> stable mangling irrespective from the target compiled for.

According to my understanding this code is used for other language spaces. For 
instance, per comments at lines 2455-2457 it's used for OpenCL and CUDA address 
spaces.
Do you mean some other mangling scheme?

> If you plan to link libraries from other frontends i.e. OpenCL or CUDA the 
> mangling you use is different from what they produce. 

SYCL standard doesn't have such functionality. OpenCL C functions are not 
mangled (only built-ins), so there should be no problem to link with OpenCL C 
libraries. 
I know that mangling difference causes some problems for SYCL built-ins 
implementations on CUDA, but I don't know all the details. @Naghasan knows 
about these. @Naghasan, do you have suggestions to fix the problems caused by 
mangling?

> Jus

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 337441.
bader marked 5 inline comments as done.
bader added a comment.

Applied more comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/AddressSpaces.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/CodeGenSYCL/address-space-deduction.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp
  clang/test/SemaTemplate/address_space-dependent.cpp

Index: clang/test/SemaTemplate/address_space-dependent.cpp
===
--- clang/test/SemaTemplate/address_space-dependent.cpp
+++ clang/test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
 
 template 
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388593)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388590)}}
 }
 
 template 
@@ -101,7 +101,7 @@
   car<1, 2, 3>(); // expected-note {{in instantiation of function template specialization 'car<1, 2, 3>' requested here}}
   HasASTemplateFields<1> HASTF;
   neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
-  correct<0x71>();
+  correct<0x7FFFED>();
   tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650>' requested here}}
 
   __attribute__((address_space(1))) char *x;
Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsycl-is-device -verify -fsyntax-only %s
+
+void bar(int &Data) {}
+void bar2(int &Data) {}
+void bar(__attribute__((opencl_private)) int &Data) {}
+void foo(int *Data) {}
+void foo2(int *Data) {}
+void foo(__attribute__((opencl_private)) int *Data) {}
+void baz(__attribute__((opencl_private)) int *Data) {} // expected-note {{candidate function not viable: cannot pass pointer to generic address space as a pointer to address space '__private' in 1st argument}}
+
+template 
+void tmpl(T *t) {}
+
+void usages() {
+  __attribute__((opencl_global)) int *GLOB;
+  __attribute__((opencl_private)) int *PRIV;
+  __attribute__((opencl_local)) int *LOC;
+  int *NoAS;
+
+  GLOB = PRIV; // expected-error {{assigning '__private int *' to '__global int *' changes address space of pointer}}
+  GLOB = LOC; // expected-error {{assigning '__local int *' to '__global int *' changes address space of pointer}}
+  PRIV = static_cast<__attribute__((opencl_private)) int *>(GLOB); // expected-error {{static_cast from '__global int *' to '__private int *' is not allowed}}
+  PRIV = static_cast<__attribute__((opencl_private)) int *>(LOC); // expected-error {{static_cast from '__local int *' to '__private int *' is not allowed}}
+  NoAS = GLOB + PRIV; // expected-error {{invalid operands to binary expression ('__global int *' and '__private int *')}}
+  NoAS = GLOB + LOC; // expected-error {{invalid operands to binary expression ('__global int *' and '__local int *')}}
+  NoAS += GLOB; // expected-error {{invalid operands to binary expression ('int *' and '__global int *')}}
+
+  bar(*GLOB);
+  bar2(*GLOB);
+
+  bar(*PRIV);
+  bar2(*PRIV);
+
+  bar(*NoAS);
+  bar2(*NoAS);
+
+  bar(*LOC);
+  bar2(*LOC);
+
+  foo(GLOB);
+  foo2(GLOB);
+  foo(PRIV);
+  foo2(PRIV);
+  foo(NoAS);
+  foo2(NoAS);
+  foo(LOC);
+  foo2(LOC);
+
+  tmpl(GLOB);
+  tmpl(PRIV);
+  tmpl(NoAS);
+  tmpl(LOC);
+
+  // Implicit casts to named address space are disallowed
+  baz(NoAS); // expected-error {{no matching function for call to 'baz'}}
+  __attribute__((opencl_local)) int *l = NoAS; // expected-error {{cannot initialize a variable of type '__local int *' with an lvalue of type 'int *'}}
+
+  (void)static_cast(GLOB);
+  (void)static_cast(GLOB);
+  int *i = GLOB;
+  void *v = GLOB;
+  (void)i;
+  (void)v;
+}
Index: clang/test/CodeGenSYCL/address-space-deduction.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/address-space-deduction.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -triple spir64 -fsycl-is-device -disable-llvm-passes -emit-llvm %s -o - | FileCheck %s
+
+// CHECK:@_ZZ4testvE3foo = internal addrspace(1) constant i32 66, align 4
+// CHECK: @[[STR:[.a-zA-Z0-9_]+]] = private unnamed_a

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader marked 2 inline comments as done.
bader added inline comments.



Comment at: clang/include/clang/AST/Type.h:488
(A == LangAS::opencl_global && (B == LangAS::opencl_global_device ||
B == LangAS::opencl_global_host)) ||
// Consider pointer size address spaces to be equivalent to default.

Anastasia wrote:
> bader wrote:
> > BTW, we need enable `global_device` and `global_host` attributes from 
> > https://reviews.llvm.org/D82174 for SYCL USM feature. I have following 
> > question regarding this: should I create a follow-up patch or we can enable 
> > all attributes for SYCL at once?
> It seems like they would just be extending the existing functionality and not 
> redesigning what we do in this patch?
> 
> If that's the case let's keep it in a separate patch, but feel free to upload 
> it even now.
> It seems like they would just be extending the existing functionality and not 
> redesigning what we do in this patch?
> 
> If that's the case let's keep it in a separate patch, but feel free to upload 
> it even now.

Added in https://reviews.llvm.org/D100396.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100396: [SYCL] Enable `opencl_global_[host,device]` attributes for SYCL

[Alexey Bader via Phabricator via cfe-commits]

bader created this revision.
Herald added subscribers: Naghasan, ldrumm, dexonsmith, kerbowa, Anastasia, 
ebevhan, yaxunl, nhaehnle, jvesely, jholewinski.
bader requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D100396

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/AddressSpaces.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/X86.h
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp
  clang/test/SemaTemplate/address_space-dependent.cpp

Index: clang/test/SemaTemplate/address_space-dependent.cpp
===
--- clang/test/SemaTemplate/address_space-dependent.cpp
+++ clang/test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
 
 template 
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388590)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388588)}}
 }
 
 template 
@@ -101,7 +101,7 @@
   car<1, 2, 3>(); // expected-note {{in instantiation of function template specialization 'car<1, 2, 3>' requested here}}
   HasASTemplateFields<1> HASTF;
   neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
-  correct<0x7FFFED>();
+  correct<0x7FFFEB>();
   tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650>' requested here}}
 
   __attribute__((address_space(1))) char *x;
Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- clang/test/SemaSYCL/address-space-conversions.cpp
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -61,4 +61,15 @@
   void *v = GLOB;
   (void)i;
   (void)v;
+
+  __attribute__((opencl_global_host)) int *GLOB_HOST;
+  bar(*GLOB_HOST);
+  bar2(*GLOB_HOST);
+  GLOB = GLOB_HOST;
+  GLOB_HOST = GLOB; // expected-error {{assigning '__global int *' to '__global_host int *' changes address space of pointer}}
+  __attribute__((opencl_global_device)) int *GLOB_DEVICE;
+  bar(*GLOB_DEVICE);
+  bar2(*GLOB_DEVICE);
+  GLOB = GLOB_DEVICE;
+  GLOB_DEVICE = GLOB; // expected-error {{assigning '__global int *' to '__global_device int *' changes address space of pointer}}
 }
Index: clang/test/CodeGenSYCL/address-space-conversions.cpp
===
--- clang/test/CodeGenSYCL/address-space-conversions.cpp
+++ clang/test/CodeGenSYCL/address-space-conversions.cpp
@@ -29,6 +29,10 @@
   // CHECK-DAG: [[PRIV:%[a-zA-Z0-9]+]] = alloca i32*
   // CHECK-DAG: [[PRIV]].ascast = addrspacecast i32** [[PRIV]] to i32* addrspace(4)*
   __attribute__((opencl_private)) int *PRIV;
+  // CHECK-DAG: [[GLOB_DEVICE:%[a-zA-Z0-9]+]] = alloca i32 addrspace(5)*
+  __attribute__((opencl_global_device)) int *GLOBDEVICE;
+  // CHECK-DAG: [[GLOB_HOST:%[a-zA-Z0-9]+]] = alloca i32 addrspace(6)*
+  __attribute__((opencl_global_host)) int *GLOBHOST;
 
   // Explicit conversions
   // From names address spaces to default address space
Index: clang/lib/Basic/Targets/X86.h
===
--- clang/lib/Basic/Targets/X86.h
+++ clang/lib/Basic/Targets/X86.h
@@ -36,6 +36,8 @@
 0,   // cuda_constant
 0,   // cuda_shared
 0,   // sycl_global
+0,   // sycl_global_device
+0,   // sycl_global_host
 0,   // sycl_local
 0,   // sycl_private
 270, // ptr32_sptr
Index: clang/lib/Basic/Targets/TCE.h
===
--- clang/lib/Basic/Targets/TCE.h
+++ clang/lib/Basic/Targets/TCE.h
@@ -42,8 +42,10 @@
 0, // cuda_device
 0, // cuda_constant
 0, // cuda_shared
-3, // sycl_global
-4, // sycl_local
+0, // sycl_global
+0, // sycl_global_device
+0, // sycl_global_host
+0, // sycl_local
 0, // sycl_private
 0, // ptr32_sptr
 0, // ptr32_uptr
Index: clang/lib/Basic/Targets/SPIR.h
===
--- clang/lib/Basic/Targets/SPIR.h
+++ clang/lib/Basic/Targets/SPIR.h
@@ -35,6 +35,8 @@
 0, // cuda_shared
 // SYCL address space values for this map are dummy
 0, // sycl_global
+0, // sycl_global_device
+0, // sycl_global_host
 0, // sycl_local
 0, // sycl_private
 0, // ptr32_sptr
@@ -56,6 +58,8 @@
 0, // cuda_constant
 0, // cuda_shared
 1, // sycl_global
+5, // sycl_global_device
+6, // sycl_global_host
 3, //

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/include/clang/AST/Type.h:493
+   // Default is a superset of SYCL address spaces.
+   (A == LangAS::Default &&
+(B == LangAS::sycl_private || B == LangAS::sycl_local ||

Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > Ok if you allow implicit conversions both ways then this condition should 
> > > be extended to also contain all named address spaces in `A` and `Default` 
> > > in `B`. But actually, could you simplify by checking that you have 
> > > `Default` on either side, so something like 
> > > 
> > > 
> > > ```
> > > (A == LangAS::Default || B == LangAS::Default)
> > > ```
> > > ?
> > > Ok if you allow implicit conversions both ways then this condition should 
> > > be extended to also contain all named address spaces in `A` and `Default` 
> > > in `B`. But actually, could you simplify by checking that you have 
> > > `Default` on either side, so something like 
> > > 
> > > 
> > > ```
> > > (A == LangAS::Default || B == LangAS::Default)
> > > ```
> > > ?
> > 
> > According to the comment above `isAddressSpaceSupersetOf` function 
> > definition.
> > ```
> >   /// Returns true if address space A is equal to or a superset of B.
> > ```
> > 
> > `(A == LangAS::Default || B == LangAS::Default)` <- this change makes 
> > `Default` address space a superset of all address spaces including OpenCL, 
> > which we were trying to avoid with adding SYCL address spaces. Another 
> > problem with this code is that make `Default` a **sub-set** of named 
> > address spaces (like `sycl_local`), which is not right.
> > If I understand it correctly defining "isSupersSetOf" relation is enough 
> > for the rest of framework to enable conversions. Am I right?
> > (A == LangAS::Default || B == LangAS::Default) <- this change makes Default 
> > address space a superset of all address spaces including OpenCL.
> 
> I see, yes this will break pretty much everything unless we guard by SYCL 
> mode. But I don't think it is good to go this route though.
> 
> > Another problem with this code is that make Default a sub-set of named 
> > address spaces (like sycl_local), which is not right.
> 
> Well, if you need implicit conversions to work both ways as you have written 
> in the documentation then you don't really have a true super-/subsets between 
> the named address spaces and the default one. They appear to be equivalent.
> 
> ```
> SYCL mode enables both explicit and implicit conversion to/from the default 
> address space from/to
> the address space-attributed type.
> ```
> 
> So do you actually need something like this to work?
> 
> ```
> int * genptr = ...;
> __private int * privptr = genptr:
> ```
> 
> 
I looked though the code base and I see that explicit cast is used when raw 
pointer is casted to address space annotated type. I think we can always use 
explicit cast from `Default` to named address space instead of implicit cast. 
It might be even useful to avoid unintended implicit casts causing UB.
@keryell, @Naghasan, what do you think if we update 
https://reviews.llvm.org/D99488 to disallow implicit casts from `Default` to 
named address space? I think it should be okay considering that current 
implementation doesn't use this type of casts (and I can't come up with a use 
case for it).

Meanwhile I've added checks for that to 
clang/test/SemaSYCL/address-space-conversions.cpp.



Comment at: clang/lib/AST/ItaniumMangle.cpp:2379
   unsigned TargetAS = Context.getASTContext().getTargetAddressSpace(AS);
-  if (TargetAS != 0)
+  if (TargetAS != 0 || Context.getASTContext().getLangOpts().SYCLIsDevice)
 ASString = "AS" + llvm::utostr(TargetAS);

Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > Any reason not to use OpenCL mangling? If you do then you might be able 
> > > to link against libraries compiled for OpenCL. Also you will get more 
> > > stable naming i.e. it would not differ from target to target. 
> > > Any reason not to use OpenCL mangling? If you do then you might be able 
> > > to link against libraries compiled for OpenCL. Also you will get more 
> > > stable naming i.e. it would not differ from target to target. 
> > 
> > I'm not sure I understand your suggestion. Could you elaborate on "OpenCL 
> > mangling", please?
> > 
> > Let me clarify the problem this change addresses. The test case covering it 
> > is located in 
> > `clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp` lines 
> > 86-91.
> > 
> > ```
> > template 
> > void tmpl(T t) {}
> > 
> > int *NoAS;
> > __attribute__((opencl_private)) int *PRIV;
> > 
> > tmpl(PRIV);
> > // CHECK-DAG: [[PRIV_LOAD5:%[a-zA-Z0-9]+]] = load i32*, i32* addrspace(4)* 
> > [[PRIV]].ascast
> > // CHECK-DAG: call spir_func void [[PRIV_TMPL:@[a-zA-Z0-9_]+]](i32* 
> > [[PRIV_LOAD5]])
> > tmpl(NoAS);
> > // CHECK-DAG: [[NoAS_LOAD5:%[a-zA-Z0-9]+]] = load i32 addrspace(4)*, i32 
> 

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 337180.
bader marked 16 inline comments as done.
bader added a comment.

Applied more code review suggestions.

Rebased on ToT.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/AddressSpaces.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/CodeGenSYCL/address-space-deduction.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp
  clang/test/SemaTemplate/address_space-dependent.cpp

Index: clang/test/SemaTemplate/address_space-dependent.cpp
===
--- clang/test/SemaTemplate/address_space-dependent.cpp
+++ clang/test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
 
 template 
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388593)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388590)}}
 }
 
 template 
@@ -101,7 +101,7 @@
   car<1, 2, 3>(); // expected-note {{in instantiation of function template specialization 'car<1, 2, 3>' requested here}}
   HasASTemplateFields<1> HASTF;
   neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
-  correct<0x71>();
+  correct<0x7FFFED>();
   tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650>' requested here}}
 
   __attribute__((address_space(1))) char *x;
Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsycl-is-device -verify -fsyntax-only %s
+
+void bar(int &Data) {}
+void bar2(int &Data) {}
+void bar(__attribute__((opencl_private)) int &Data) {}
+void foo(int *Data) {}
+void foo2(int *Data) {}
+void foo(__attribute__((opencl_private)) int *Data) {}
+void baz(__attribute__((opencl_private)) int *Data) {} // expected-note {{candidate function not viable: cannot pass pointer to generic address space as a pointer to address space '__private' in 1st argument}}
+
+template 
+void tmpl(T *t) {}
+
+void usages() {
+  __attribute__((opencl_global)) int *GLOB;
+  __attribute__((opencl_private)) int *PRIV;
+  __attribute__((opencl_local)) int *LOC;
+  int *NoAS;
+
+  GLOB = PRIV; // expected-error {{assigning '__private int *' to '__global int *' changes address space of pointer}}
+  GLOB = LOC; // expected-error {{assigning '__local int *' to '__global int *' changes address space of pointer}}
+  PRIV = static_cast<__attribute__((opencl_private)) int *>(GLOB); // expected-error {{static_cast from '__global int *' to '__private int *' is not allowed}}
+  PRIV = static_cast<__attribute__((opencl_private)) int *>(LOC); // expected-error {{static_cast from '__local int *' to '__private int *' is not allowed}}
+  NoAS = GLOB + PRIV; // expected-error {{invalid operands to binary expression ('__global int *' and '__private int *')}}
+  NoAS = GLOB + LOC; // expected-error {{invalid operands to binary expression ('__global int *' and '__local int *')}}
+  NoAS += GLOB; // expected-error {{invalid operands to binary expression ('int *' and '__global int *')}}
+
+  bar(*GLOB);
+  bar2(*GLOB);
+
+  bar(*PRIV);
+  bar2(*PRIV);
+
+  bar(*NoAS);
+  bar2(*NoAS);
+
+  bar(*LOC);
+  bar2(*LOC);
+
+  foo(GLOB);
+  foo2(GLOB);
+  foo(PRIV);
+  foo2(PRIV);
+  foo(NoAS);
+  foo2(NoAS);
+  foo(LOC);
+  foo2(LOC);
+
+  tmpl(GLOB);
+  tmpl(PRIV);
+  tmpl(NoAS);
+  tmpl(LOC);
+
+  // Implicit casts to named address space are disallowed
+  baz(NoAS); // expected-error {{no matching function for call to 'baz'}}
+  __attribute__((opencl_local)) int *l = NoAS; // expected-error {{cannot initialize a variable of type '__local int *' with an lvalue of type 'int *'}}
+
+  (void)static_cast(GLOB);
+  (void)static_cast(GLOB);
+  int *i = GLOB;
+  void *v = GLOB;
+  (void)i;
+  (void)v;
+}
Index: clang/test/CodeGenSYCL/address-space-deduction.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/address-space-deduction.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -triple spir64 -fsycl-is-device -disable-llvm-passes -emit-llvm %s -o - | FileCheck %s
+
+// CHECK:@_ZZ4testvE3foo = internal addrspace(1) constant i32 66, align 4
+// CHECK: @[[STR:[.a

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader marked an inline comment as done.
bader added inline comments.



Comment at: clang/include/clang/AST/Type.h:488
(A == LangAS::opencl_global && (B == LangAS::opencl_global_device ||
B == LangAS::opencl_global_host)) ||
// Consider pointer size address spaces to be equivalent to default.

BTW, we need enable `global_device` and `global_host` attributes from 
https://reviews.llvm.org/D82174 for SYCL USM feature. I have following question 
regarding this: should I create a follow-up patch or we can enable all 
attributes for SYCL at once?



Comment at: clang/test/CodeGenSYCL/convergent.cpp:2
 // RUN: %clang_cc1 -fsycl-is-device -emit-llvm -disable-llvm-passes \
-// RUN:  -triple spir64-unknown-unknown-sycldevice -emit-llvm %s -o - | \
 // RUN:   FileCheck %s

Anastasia wrote:
> Is this change related? I thought we are not adding the environment component 
> after all...
> 
>  
> Is this change related? I thought we are not adding the environment component 
> after all...

While I was removing `-sycldevice` environment component from the patch, I 
noticed that one of the committed tests already uses it.
https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGenSYCL/convergent.cpp#L2

Do you want to me to create a separate review request for this change?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/include/clang/AST/Type.h:493
+   // Default is a superset of SYCL address spaces.
+   (A == LangAS::Default &&
+(B == LangAS::sycl_private || B == LangAS::sycl_local ||

Anastasia wrote:
> Ok if you allow implicit conversions both ways then this condition should be 
> extended to also contain all named address spaces in `A` and `Default` in 
> `B`. But actually, could you simplify by checking that you have `Default` on 
> either side, so something like 
> 
> 
> ```
> (A == LangAS::Default || B == LangAS::Default)
> ```
> ?
> Ok if you allow implicit conversions both ways then this condition should be 
> extended to also contain all named address spaces in `A` and `Default` in 
> `B`. But actually, could you simplify by checking that you have `Default` on 
> either side, so something like 
> 
> 
> ```
> (A == LangAS::Default || B == LangAS::Default)
> ```
> ?

According to the comment above `isAddressSpaceSupersetOf` function definition.
```
  /// Returns true if address space A is equal to or a superset of B.
```

`(A == LangAS::Default || B == LangAS::Default)` <- this change makes `Default` 
address space a superset of all address spaces including OpenCL, which we were 
trying to avoid with adding SYCL address spaces. Another problem with this code 
is that make `Default` a **sub-set** of named address spaces (like 
`sycl_local`), which is not right.
If I understand it correctly defining "isSupersSetOf" relation is enough for 
the rest of framework to enable conversions. Am I right?



Comment at: clang/lib/AST/ItaniumMangle.cpp:2379
   unsigned TargetAS = Context.getASTContext().getTargetAddressSpace(AS);
-  if (TargetAS != 0)
+  if (TargetAS != 0 || Context.getASTContext().getLangOpts().SYCLIsDevice)
 ASString = "AS" + llvm::utostr(TargetAS);

Anastasia wrote:
> Any reason not to use OpenCL mangling? If you do then you might be able to 
> link against libraries compiled for OpenCL. Also you will get more stable 
> naming i.e. it would not differ from target to target. 
> Any reason not to use OpenCL mangling? If you do then you might be able to 
> link against libraries compiled for OpenCL. Also you will get more stable 
> naming i.e. it would not differ from target to target. 

I'm not sure I understand your suggestion. Could you elaborate on "OpenCL 
mangling", please?

Let me clarify the problem this change addresses. The test case covering it is 
located in `clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp` 
lines 86-91.

```
template 
void tmpl(T t) {}

int *NoAS;
__attribute__((opencl_private)) int *PRIV;

tmpl(PRIV);
// CHECK-DAG: [[PRIV_LOAD5:%[a-zA-Z0-9]+]] = load i32*, i32* addrspace(4)* 
[[PRIV]].ascast
// CHECK-DAG: call spir_func void [[PRIV_TMPL:@[a-zA-Z0-9_]+]](i32* 
[[PRIV_LOAD5]])
tmpl(NoAS);
// CHECK-DAG: [[NoAS_LOAD5:%[a-zA-Z0-9]+]] = load i32 addrspace(4)*, i32 
addrspace(4)* addrspace(4)* [[NoAS]].ascast
// CHECK-DAG: call spir_func void [[GEN_TMPL:@[a-zA-Z0-9_]+]](i32 addrspace(4)* 
[[NoAS_LOAD5]])
```
Clang has separate code paths for mangling types w/ and w/o address space 
attributes (i.e. using `Default` address space).

Address space is not mangled if there is no AS attribute (`Default`) or if 
address space attribute is maps to `0` target address space. SPIR target maps 
`*_private` address space to `0`, which causes name conflict for the example 
above.

This change for SYCL compiler enables mangling for non-default address space 
attributes regardless of their mapping to target address space.



Comment at: clang/lib/Basic/Targets/AMDGPU.cpp:74
 Local,// cuda_shared
+Global,   // sycl_global
+Local,// sycl_local

Anastasia wrote:
> Would this map ever be used for SYCL? If not it would be better to add a 
> comment about it and/or perhaps even just use dummy values.
I can't find an example of how to do this.
CUDA address spaces use valid values and I wasn't able to find similar comments.

Where do you think we can put a comment?



Comment at: clang/lib/Basic/Targets/SPIR.h:36
 0, // cuda_shared
+1, // sycl_global
+3, // sycl_local

Anastasia wrote:
> The same here. This map will never work for SYCL so let's just use dummy 
> values like for CUDA and add a comment explaining this.
I've set 0 for SYCL values.



Comment at: clang/lib/Basic/Targets/SPIR.h:71
 LongWidth = LongAlign = 64;
-AddrSpaceMap = &SPIRAddrSpaceMap;
+AddrSpaceMap = Triple.getEnvironment() == llvm::Triple::SYCLDevice
+   ? &SPIRDefIsGenMap

Anastasia wrote:
> Ok so what I understand is that the only reason you need a separate map is 
> that the semantics of `Default` is different for SYCL than for C/C++.
> 
> //i.e. in SYCL (i.e. inherited from CUDA) it is a virtual/placehold

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 336543.
bader marked 32 inline comments as done.
bader added a comment.

Applied code review suggestions.

Rebased on ToT and updated commit message.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/AddressSpaces.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGenSYCL/address-space-cond-op.cpp
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/CodeGenSYCL/address-space-deduction.cpp
  clang/test/CodeGenSYCL/address-space-of-returns.cpp
  clang/test/CodeGenSYCL/convergent.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp
  clang/test/SemaTemplate/address_space-dependent.cpp

Index: clang/test/SemaTemplate/address_space-dependent.cpp
===
--- clang/test/SemaTemplate/address_space-dependent.cpp
+++ clang/test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
 
 template 
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388593)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388590)}}
 }
 
 template 
@@ -101,7 +101,7 @@
   car<1, 2, 3>(); // expected-note {{in instantiation of function template specialization 'car<1, 2, 3>' requested here}}
   HasASTemplateFields<1> HASTF;
   neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
-  correct<0x71>();
+  correct<0x7FFFED>();
   tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650>' requested here}}
 
   __attribute__((address_space(1))) char *x;
Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -fsycl-is-device -verify -fsyntax-only %s
+
+void bar(int &Data) {}
+void bar2(int &Data) {}
+void bar(__attribute__((opencl_private)) int &Data) {}
+void foo(int *Data) {}
+void foo2(int *Data) {}
+void foo(__attribute__((opencl_private)) int *Data) {}
+
+template 
+void tmpl(T *t) {}
+
+void usages() {
+  __attribute__((opencl_global)) int *GLOB;
+  __attribute__((opencl_private)) int *PRIV;
+  __attribute__((opencl_local)) int *LOC;
+  int *NoAS;
+
+  GLOB = PRIV; // expected-error {{assigning '__private int *' to '__global int *' changes address space of pointer}}
+  GLOB = LOC; // expected-error {{assigning '__local int *' to '__global int *' changes address space of pointer}}
+  PRIV = static_cast<__attribute__((opencl_private)) int *>(GLOB); // expected-error {{static_cast from '__global int *' to '__private int *' is not allowed}}
+  PRIV = static_cast<__attribute__((opencl_private)) int *>(LOC); // expected-error {{static_cast from '__local int *' to '__private int *' is not allowed}}
+  NoAS = GLOB + PRIV; // expected-error {{invalid operands to binary expression ('__global int *' and '__private int *')}}
+  NoAS = GLOB + LOC; // expected-error {{invalid operands to binary expression ('__global int *' and '__local int *')}}
+  NoAS += GLOB; // expected-error {{invalid operands to binary expression ('int *' and '__global int *')}}
+
+  bar(*GLOB);
+  bar2(*GLOB);
+
+  bar(*PRIV);
+  bar2(*PRIV);
+
+  bar(*NoAS);
+  bar2(*NoAS);
+
+  bar(*LOC);
+  bar2(*LOC);
+
+  foo(GLOB);
+  foo2(GLOB);
+  foo(PRIV);
+  foo2(PRIV);
+  foo(NoAS);
+  foo2(NoAS);
+  foo(LOC);
+  foo2(LOC);
+
+  tmpl(GLOB);
+  tmpl(PRIV);
+  tmpl(NoAS);
+  tmpl(LOC);
+
+  (void)static_cast(GLOB);
+  (void)static_cast(GLOB);
+  int *i = GLOB;
+  void *v = GLOB;
+  (void)i;
+  (void)v;
+}
Index: clang/test/CodeGenSYCL/convergent.cpp
===
--- clang/test/CodeGenSYCL/convergent.cpp
+++ clang/test/CodeGenSYCL/convergent.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -fsycl-is-device -emit-llvm -disable-llvm-passes \
-// RUN:  -triple spir64-unknown-unknown-sycldevice -emit-llvm %s -o - | \
+// RUN:  -triple spir64 -emit-llvm %s -o - | \
 // RUN:   FileCheck %s
 
 // CHECK-DAG: Function Attrs:
Index: clang/test/CodeGenSYCL/address-space-of-returns.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/address-space-of-returns.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -triple spir64 -fsycl-is-device -disable-llvm-passes -emi

[PATCH] D99488: [SYCL][Doc] Add address space handling section to SYCL documentation

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D99488#2671906 , @Anastasia wrote:

> LGTM! Thanks for working on this. The expected sematic seems fairly clear now.

Thanks for review! I also fixed external hyperlinks formatting.

> We might add a few more details while refining the implementation but it 
> should not block the development progress at this point.

Great! Please, let me know if there any comment for the implementation - 
https://reviews.llvm.org/D89909.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99488/new/

https://reviews.llvm.org/D99488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99488: [SYCL][Doc] Add address space handling section to SYCL documentation

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 335596.
bader marked 7 inline comments as done.
bader added a comment.

Add ReST marks to hyperlinks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99488/new/

https://reviews.llvm.org/D99488

Files:
  clang/docs/SYCLSupport.rst


Index: clang/docs/SYCLSupport.rst
===
--- clang/docs/SYCLSupport.rst
+++ clang/docs/SYCLSupport.rst
@@ -219,3 +219,93 @@
 
 Additional details of kernel parameter passing may be found in the document
 `SYCL Kernel Parameter Handling and Array Support 
`_.
+
+Address space handling
+^^
+
+The SYCL specification represents pointers to disjoint memory regions using C++
+wrapper classes on an accelerator to enable compilation with a standard C++
+toolchain and a SYCL compiler toolchain. Section 3.8.2 of SYCL 2020
+specification defines
+`memory model 
`_\
 ,
+section 4.7.7 - `address space classes 
`_
+and section 5.9 covers `address space deduction 
`_.
+The SYCL specification allows two modes of address space deduction: "generic as
+default address space" (see section 5.9.3) and "inferred address space" (see
+section 5.9.4). Current implementation supports only "generic as default 
address
+space" mode.
+
+SYCL borrows its memory model from OpenCL however SYCL doesn't perform
+the address space qualifier inference as detailed in
+`OpenCL C v3.0 6.7.8 
`_.
+
+The default address space is "generic-memory", which is a virtual address space
+that overlaps the global, local, and private address spaces. SYCL mode enables
+both explicit and implicit conversion to/from the default address space from/to
+the address space-attributed type. All named address spaces are disjoint and
+sub-sets of default address space.
+
+The SPIR target allocates SYCL namespace scope variables in the global address
+space.
+
+Pointers to default address space should get lowered into a pointer to a 
generic
+address space (or flat to reuse more general terminology). But depending on the
+allocation context, the default address space of a non-pointer type is assigned
+to a specific address space. This is described in
+`common address space deduction rules 
`_
+section.
+
+This is also in line with the behaviour of CUDA (`small example
+`_).
+
+``multi_ptr`` class implementation example:
+
+.. code-block:: C++
+
+   // check that SYCL mode is ON and we can use non-standard decorations
+   #if defined(__SYCL_DEVICE_ONLY__)
+   // GPU/accelerator implementation
+   template  class multi_ptr {
+ // DecoratedType applies corresponding address space attribute to the 
type T
+ // DecoratedType::type == 
"__attribute__((opencl_global)) T"
+ // See sycl/include/CL/sycl/access/access.hpp for more details
+ using pointer_t = typename DecoratedType::type *;
+
+ pointer_t m_Pointer;
+ public:
+ pointer_t get() { return m_Pointer; }
+ T& operator* () { return *reinterpret_cast(m_Pointer); }
+   }
+   #else
+   // CPU/host implementation
+   template  class multi_ptr {
+ T *m_Pointer; // regular undecorated pointer
+ public:
+ T *get() { return m_Pointer; }
+ T& operator* () { return *m_Pointer; }
+   }
+   #endif
+
+Depending on the compiler mode, ``multi_ptr`` will either decorate its internal
+data with the address space attribute or not.
+
+To utilize clang's existing functionality, we reuse the following OpenCL 
address
+space attributes for pointers:
+
+.. list-table::
+   :header-rows: 1
+
+   * - Address space attribute
+ - SYCL address_space enumeration
+   * - ``__attribute__((opencl_global))``
+ - global_space, constant_space
+   * - ``__attribute__((opencl_local))``
+ - local_space
+   * - ``__attribute__((opencl_private))``
+ - private_space
+
+
+.. code-block::
+
+   TODO: add support for `__attribute__((opencl_global_host))` and
+   `__attribute__((opencl_global_device))`.


Index: clang/docs/SYCLSupport.rst
===
--- clang/docs/SYCLSupport.rst
+++ clang/docs/SYCLSupport.rst
@@ -219,3 +219,93 @@
 
 Additional details of kernel parameter passing may be found in the document
 `SYCL Kernel Parameter Handling and Array Support `_.
+
+Address space handlin

[PATCH] D99488: [SYCL][Doc] Add address space handling section to SYCL documentation

[Alexey Bader via Phabricator via cfe-commits]

bader marked 4 inline comments as done.
bader added inline comments.



Comment at: clang/docs/SYCLSupport.rst:243
+Similar to other single-source C++-based GPU programming modes like
+OpenMP/CUDA/HIP, SYCL uses clang's "default" address space for types with no
+explicit address space attribute. This design has two important features: it

Anastasia wrote:
> This is ambiguous now because every language will use `clang's "default" 
> address space` because at least one address space is always needed by every 
> language but it has different semantics in languages. We should either 
> attempt to describe it somehow or perhaps just point out that it is inherited 
> from CUDA and currently undocumented.
Removed this paragraph as it's already covered by SYCL specification.



Comment at: clang/docs/SYCLSupport.rst:341
+that overlaps the global, local, and private address spaces. SYCL mode enables
+conversion to/from the default address space from/to the address
+space-attributed type.

Anastasia wrote:
> Do you mean both implicit and explicit conversions? Does it mean that in your 
> AS model named ASes are subset of generic AS and generic AS is a subset of 
> named ASes so they are equivalent sets? It is probably good to mention here 
> that all named address spaces are disjoint.
> Do you mean both implicit and explicit conversions? Does it mean that in your 
> AS model named ASes are subset of generic AS and generic AS is a subset of 
> named ASes so they are equivalent sets? It is probably good to mention here 
> that all named address spaces are disjoint.

Updated paragraph:

The default address space is "generic-memory", which is a virtual address space
that overlaps the global, local, and private address spaces. SYCL mode enables
both explicit and implicit conversion to/from the default address space from/to
the address space-attributed type. All named address spaces are disjoint and
sub-sets of default address space.



Comment at: clang/docs/SYCLSupport.rst:344
+
+The SPIR target allocates SYCL namespace scope variables in the global address
+space.

Anastasia wrote:
> Interesting, will this deduction always be target specific or can it be 
> generalized since it is governed by the language semantic already?
> Interesting, will this deduction always be target specific or can it be 
> generalized since it is governed by the language semantic already?

It's target specific deduction. CPU targets doesn't require such deduction.



Comment at: clang/docs/SYCLSupport.rst:347
+
+Pointers to Default address space should get lowered into a pointer to a 
generic
+address space (or flat to reuse more general terminology). But depending on the

Anastasia wrote:
> I think it is also relevant to highlight that you don't perform inference of 
> the address space qualifiers and the memory segment binding is performed as a 
> final phase of parsing. This is quite relevant since embedded C or C++ have 
> no address space inference at all and OpenCL explicitly requires inference in 
> the type qualifiers.
> I think it is also relevant to highlight that you don't perform inference of 
> the address space qualifiers and the memory segment binding is performed as a 
> final phase of parsing. This is quite relevant since embedded C or C++ have 
> no address space inference at all and OpenCL explicitly requires inference in 
> the type qualifiers.

I move this paragraph before the code example right after this section:

SYCL borrows its memory model from OpenCL however SYCL doesn't perform
the address space qualifier inference as detailed in
`OpenCL C v3.0 6.7.8 
`_.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99488/new/

https://reviews.llvm.org/D99488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99488: [SYCL][Doc] Add address space handling section to SYCL documentation

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 334927.
bader marked 4 inline comments as done.
bader added a comment.

Address comments from @Anastasia.

- Removed controversial clarifications.
- Reshuffled text to keep language semantics clarifications closer to each 
other.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99488/new/

https://reviews.llvm.org/D99488

Files:
  clang/docs/SYCLSupport.rst


Index: clang/docs/SYCLSupport.rst
===
--- clang/docs/SYCLSupport.rst
+++ clang/docs/SYCLSupport.rst
@@ -219,3 +219,92 @@
 
 Additional details of kernel parameter passing may be found in the document
 `SYCL Kernel Parameter Handling and Array Support 
`_.
+
+Address space handling
+^^
+
+The SYCL specification represents pointers to disjoint memory regions using C++
+wrapper classes on an accelerator to enable compilation with a standard C++
+toolchain and a SYCL compiler toolchain. Section 3.8.2 of SYCL 2020
+specification defines
+`memory model 
`_\
 ,
+section 4.7.7 - `address space classes 
`_
+and section 5.9 covers `address space deduction 
`_.
+The SYCL specification allows two modes of address space deduction: "generic as
+default address space" (see section 5.9.3) and "inferred address space" (see
+section 5.9.4). Current implementation supports only "generic as default 
address
+space" mode.
+
+SYCL borrows its memory model from OpenCL however SYCL doesn't perform
+the address space qualifier inference as detailed in
+`OpenCL C v3.0 6.7.8 
`_.
+
+The default address space is "generic-memory", which is a virtual address space
+that overlaps the global, local, and private address spaces. SYCL mode enables
+both explicit and implicit conversion to/from the default address space from/to
+the address space-attributed type. All named address spaces are disjoint and
+sub-sets of default address space.
+
+The SPIR target allocates SYCL namespace scope variables in the global address
+space.
+
+Pointers to default address space should get lowered into a pointer to a 
generic
+address space (or flat to reuse more general terminology). But depending on the
+allocation context, the default address space of a non-pointer type is assigned
+to a specific address space. This is described in
+https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace.
+
+This is also in line with the behaviour of CUDA (small example
+https://godbolt.org/z/veqTfo9PK).
+
+``multi_ptr`` class implementation example:
+
+.. code-block:: C++
+
+   // check that SYCL mode is ON and we can use non-standard decorations
+   #if defined(__SYCL_DEVICE_ONLY__)
+   // GPU/accelerator implementation
+   template  class multi_ptr {
+ // DecoratedType applies corresponding address space attribute to the 
type T
+ // DecoratedType::type == 
"__attribute__((opencl_global)) T"
+ // See sycl/include/CL/sycl/access/access.hpp for more details
+ using pointer_t = typename DecoratedType::type *;
+
+ pointer_t m_Pointer;
+ public:
+ pointer_t get() { return m_Pointer; }
+ T& operator* () { return *reinterpret_cast(m_Pointer); }
+   }
+   #else
+   // CPU/host implementation
+   template  class multi_ptr {
+ T *m_Pointer; // regular undecorated pointer
+ public:
+ T *get() { return m_Pointer; }
+ T& operator* () { return *m_Pointer; }
+   }
+   #endif
+
+Depending on the compiler mode, ``multi_ptr`` will either decorate its internal
+data with the address space attribute or not.
+
+To utilize clang's existing functionality, we reuse the following OpenCL 
address
+space attributes for pointers:
+
+.. list-table::
+   :header-rows: 1
+
+   * - Address space attribute
+ - SYCL address_space enumeration
+   * - ``__attribute__((opencl_global))``
+ - global_space, constant_space
+   * - ``__attribute__((opencl_local))``
+ - local_space
+   * - ``__attribute__((opencl_private))``
+ - private_space
+
+
+.. code-block::
+
+   TODO: add support for `__attribute__((opencl_global_host))` and
+   `__attribute__((opencl_global_device))`.


Index: clang/docs/SYCLSupport.rst
===
--- clang/docs/SYCLSupport.rst
+++ clang/docs/SYCLSupport.rst
@@ -219,3 +219,92 @@
 
 Additional details of kernel parameter passing may be found in the document
 `SYCL Kernel Parameter Handling and Array Support 

[PATCH] D99190: [SYCL] Add design document for SYCL mode

[Alexey Bader via Phabricator via cfe-commits]

bader marked 2 inline comments as done.
bader added inline comments.



Comment at: clang/docs/SYCLSupport.md:51
+virtual calls), generates LLVM IR for the device code only and an "integration
+header" which provides information like kernel name, parameters order and data
+type for the runtime library.

Naghasan wrote:
> Do you plan on upstreaming your integration header approach ? Even if it is 
> useful in some situations and speeds up the creation of a prototype, it comes 
> with its complications.
> 
> An integration header creates a by-product then used as input for another 
> compilation phase. I haven't at the upstream driver capabilities for awhile, 
> but I don't think you can model 2 outputs yet. Are  you planing on adding 
> this capability ?
> If not, wouldn't that forces you to trigger a compilation step just for the 
> generation of that files ? If so that puts a strong burden on the compilation 
> speed as you now have to process 3 times your input C++ file.
It looks like Mike addressed this issue with 
https://github.com/intel/llvm/pull/3471. :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99190/new/

https://reviews.llvm.org/D99190

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99190: [SYCL] Add design document for SYCL mode

[Alexey Bader via Phabricator via cfe-commits]

bader marked an inline comment as done.
bader added inline comments.



Comment at: clang/docs/SYCLSupport.md:123
+traverse all symbols accessible from kernel functions and add them to the
+"device part" of the code marking them with the new SYCL device attribute.
+

Naghasan wrote:
> bader wrote:
> > Naghasan wrote:
> > > ABataev wrote:
> > > > bader wrote:
> > > > > Naghasan wrote:
> > > > > > OpenMP offload uses a similar approach isn't it? Might be worth to 
> > > > > > describe how the 2 relates to each other and where they diverge. 
> > > > > Do you mean the approach OpenMP compiler uses to outline 
> > > > > single-source code parts to offload?
> > > > > To be honest, I'm not sure... @ABataev, is there any description how 
> > > > > OpenMP compiler outlines device code?
> > > > > https://clang.llvm.org/docs/OpenMPSupport.html doesn't provide much 
> > > > > details unfortunately.
> > > > I don't think we have anything like this. Moreover, currently, there 
> > > > are 2 different models, one with outlining by the frontend and another 
> > > > one with the outlining by the LLVM.
> > > I mentioned that as I know there is some support for CUDA and the clang 
> > > driver openmp offload works with multiple frontend passes.
> > > If the model(s) is too different then there is no point going further 
> > > here. 
> > > 
> > > > Moreover, currently, there are 2 different models, one with outlining 
> > > > by the frontend and another one with the outlining by the LLVM.
> > > 
> > > I do recall some brief conversations about that. Are they meant to work 
> > > in pair or one aims to replace the other ?
> > What do say if add a TODO here (or in a separate TODO document) to study 
> > more about differences between SYCL/OpenMP-offload/CUDA implementation 
> > designs?
> > It seems to be useful to understanding if we want to re-use other 
> > programming model experience with implementing common tasks like device 
> > code outlining.
> fine by me :)
I've added a TODO section to the document.



Comment at: clang/docs/SYCLSupport.rst:198
+   class MyObj {
+ accessor _ptr; // accessor contains a pointer to the global 
address space.
+   public:

Naghasan wrote:
> Oups sorry, I just noticed  I made a mistake in my suggestion, that's more in 
> line with the statement `KernelFuncObj.A.__init(a);` in the pseudo  opencl
Thanks. Fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99190/new/

https://reviews.llvm.org/D99190

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99488: [SYCL][Doc] Add address space handling section to SYCL documentation

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 334689.
bader marked 3 inline comments as done.
bader added a comment.

Applied code review suggestions from @Naghasan.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99488/new/

https://reviews.llvm.org/D99488

Files:
  clang/docs/SYCLSupport.rst

Index: clang/docs/SYCLSupport.rst
===
--- clang/docs/SYCLSupport.rst
+++ clang/docs/SYCLSupport.rst
@@ -219,3 +219,136 @@
 
 Additional details of kernel parameter passing may be found in the document
 `SYCL Kernel Parameter Handling and Array Support `_.
+
+Address space handling
+^^
+
+The SYCL specification represents pointers to disjoint memory regions using C++
+wrapper classes on an accelerator to enable compilation with a standard C++
+toolchain and a SYCL compiler toolchain. Section 3.8.2 of SYCL 2020
+specification defines
+`memory model `_\ ,
+section 4.7.7 - `address space classes `_
+and section 5.9 covers `address space deduction `_.
+The SYCL specification allows two modes of address space deduction: "generic as
+default address space" (see section 5.9.3) and "inferred address space" (see
+section 5.9.4). Current implementation supports only "generic as default address
+space" mode.
+
+SYCL borrows its memory model from OpenCL however SYCL doesn't perform
+the address space qualifier inference as detailed in
+`OpenCL C v3.0 6.7.8 `_.
+
+Similar to other single-source C++-based GPU programming modes like
+OpenMP/CUDA/HIP, SYCL uses clang's "default" address space for types with no
+explicit address space attribute. This design has two important features: it
+keeps the type system consistent with C++ and enable tools for emitting device
+code aligned with SPIR memory model (and other GPU targets).
+
+So inside a function, this variable declaration:
+
+.. code-block:: C++
+
+   int var;
+
+The SYCL device compiler turns into:
+
+.. code-block:: C++
+
+   VarDecl  var 'int'
+
+while the OpenCL compiler turns it into:
+
+.. code-block:: C++
+
+   VarDecl  var '__private int'
+
+Changing the type of a variable can have observable effects in C++. For example,
+this does not compile in C++ for OpenCL mode:
+
+.. code-block:: C++
+
+   template
+   struct is_same {
+ static constexpr int value = 0;
+   };
+
+   template
+   struct is_same {
+ static constexpr int value = 1;
+   };
+
+   void foo(int p) {
+ static_assert(is_same::value, "int is not an int?"); // Fails: p is '__private int' != 'int'
+ static_assert(is_same::value, "int* is not an int*?");  // Fails: p is '__private int*' != '__generic int*'
+   }
+
+``multi_ptr`` class implementation example:
+
+.. code-block:: C++
+
+   // check that SYCL mode is ON and we can use non-standard decorations
+   #if defined(__SYCL_DEVICE_ONLY__)
+   // GPU/accelerator implementation
+   template  class multi_ptr {
+ // DecoratedType applies corresponding address space attribute to the type T
+ // DecoratedType::type == "__attribute__((opencl_global)) T"
+ // See sycl/include/CL/sycl/access/access.hpp for more details
+ using pointer_t = typename DecoratedType::type *;
+
+ pointer_t m_Pointer;
+ public:
+ pointer_t get() { return m_Pointer; }
+ T& operator* () { return *reinterpret_cast(m_Pointer); }
+   }
+   #else
+   // CPU/host implementation
+   template  class multi_ptr {
+ T *m_Pointer; // regular undecorated pointer
+ public:
+ T *get() { return m_Pointer; }
+ T& operator* () { return *m_Pointer; }
+   }
+   #endif
+
+Depending on the compiler mode, ``multi_ptr`` will either decorate its internal
+data with the address space attribute or not.
+
+To utilize clang's existing functionality, we reuse the following OpenCL address
+space attributes for pointers:
+
+.. list-table::
+   :header-rows: 1
+
+   * - Address space attribute
+ - SYCL address_space enumeration
+   * - ``__attribute__((opencl_global))``
+ - global_space, constant_space
+   * - ``__attribute__((opencl_local))``
+ - local_space
+   * - ``__attribute__((opencl_private))``
+ - private_space
+
+
+.. code-block::
+
+   TODO: add support for `__attribute__((opencl_global_host))` and
+   `__attribute__((opencl_global_device))`.
+
+
+The default address space is "generic-memory", which is a virtual address space
+that overlaps the global, local, and private address spaces. SYCL mode enables
+conversion to/from the default address space 

[PATCH] D99190: WIP: [SYCL] Add design document for SYCL mode

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/docs/SYCLSupport.md:123
+traverse all symbols accessible from kernel functions and add them to the
+"device part" of the code marking them with the new SYCL device attribute.
+

Naghasan wrote:
> ABataev wrote:
> > bader wrote:
> > > Naghasan wrote:
> > > > OpenMP offload uses a similar approach isn't it? Might be worth to 
> > > > describe how the 2 relates to each other and where they diverge. 
> > > Do you mean the approach OpenMP compiler uses to outline single-source 
> > > code parts to offload?
> > > To be honest, I'm not sure... @ABataev, is there any description how 
> > > OpenMP compiler outlines device code?
> > > https://clang.llvm.org/docs/OpenMPSupport.html doesn't provide much 
> > > details unfortunately.
> > I don't think we have anything like this. Moreover, currently, there are 2 
> > different models, one with outlining by the frontend and another one with 
> > the outlining by the LLVM.
> I mentioned that as I know there is some support for CUDA and the clang 
> driver openmp offload works with multiple frontend passes.
> If the model(s) is too different then there is no point going further here. 
> 
> > Moreover, currently, there are 2 different models, one with outlining by 
> > the frontend and another one with the outlining by the LLVM.
> 
> I do recall some brief conversations about that. Are they meant to work in 
> pair or one aims to replace the other ?
What do say if add a TODO here (or in a separate TODO document) to study more 
about differences between SYCL/OpenMP-offload/CUDA implementation designs?
It seems to be useful to understanding if we want to re-use other programming 
model experience with implementing common tasks like device code outlining.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99190/new/

https://reviews.llvm.org/D99190

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99488: [SYCL][Doc] Add address space handling section to SYCL documentation

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 334433.
bader added a comment.

Convert document to ReST format.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99488/new/

https://reviews.llvm.org/D99488

Files:
  clang/docs/SYCLSupport.rst

Index: clang/docs/SYCLSupport.rst
===
--- clang/docs/SYCLSupport.rst
+++ clang/docs/SYCLSupport.rst
@@ -207,3 +207,132 @@
 
 Additional details of kernel parameter passing may be found in the document
 `SYCL Kernel Parameter Handling and Array Support `_.
+
+Address space handling
+^^
+
+The SYCL specification represents pointers to disjoint memory regions using C++
+wrapper classes on an accelerator to enable compilation with a standard C++
+toolchain and a SYCL compiler toolchain. Section 3.8.2 of SYCL 2020
+specification defines
+`memory model `_\ ,
+section 4.7.7 - `address space classes `_
+and section 5.9 covers `address space deduction `_.
+
+The main address space semantic difference between SYCL and OpenCL is that SYCL
+doesn't perform the address space qualifier inference detailed in
+`OpenCL C v3.0 6.7.8 `_.
+
+Similar to other single-source C++-based GPU programming modes like
+OpenMP/CUDA/HIP, SYCL uses clang's "default" address space for types with no
+explicit address space attribute. This design has two important features: it
+keeps the type system consistent with C++ and enable tools for emitting device
+code aligned with SPIR memory model (and other GPU targets).
+
+So inside a function, this variable declaration:
+
+.. code-block:: C++
+
+   int var;
+
+The SYCL device compiler turns into:
+
+.. code-block:: C++
+
+   VarDecl  var 'int'
+
+while the OpenCL compiler turns it into:
+
+.. code-block:: C++
+
+   VarDecl  var '__private int'
+
+Changing the type of a variable can have observable effects in C++. For example,
+this does not compile in C++ for OpenCL mode:
+
+.. code-block:: C++
+
+   template
+   struct is_same {
+ static constexpr int value = 0;
+   };
+
+   template
+   struct is_same {
+ static constexpr int value = 1;
+   };
+
+   void foo(int p) {
+ static_assert(is_same::value, "int is not an int?"); // Fails: p is '__private int' != 'int'
+ static_assert(is_same::value, "int* is not an int*?");  // Fails: p is '__private int*' != '__generic int*'
+   }
+
+``multi_ptr`` class implementation example:
+
+.. code-block:: C++
+
+   // check that SYCL mode is ON and we can use non-standard decorations
+   #if defined(__SYCL_DEVICE_ONLY__)
+   // GPU/accelerator implementation
+   template  class multi_ptr {
+ // DecoratedType applies corresponding address space attribute to the type T
+ // DecoratedType::type == "__attribute__((opencl_global)) T"
+ // See sycl/include/CL/sycl/access/access.hpp for more details
+ using pointer_t = typename DecoratedType::type *;
+
+ pointer_t m_Pointer;
+ public:
+ pointer_t get() { return m_Pointer; }
+ T& operator* () { return *reinterpret_cast(m_Pointer); }
+   }
+   #else
+   // CPU/host implementation
+   template  class multi_ptr {
+ T *m_Pointer; // regular undecorated pointer
+ public:
+ T *get() { return m_Pointer; }
+ T& operator* () { return *m_Pointer; }
+   }
+   #endif
+
+Depending on the compiler mode, ``multi_ptr`` will either decorate its internal
+data with the address space attribute or not.
+
+To utilize clang's existing functionality, we reuse the following OpenCL address
+space attributes for pointers:
+
+.. list-table::
+   :header-rows: 1
+
+   * - Address space attribute
+ - SYCL address_space enumeration
+   * - ``__attribute__((opencl_global))``
+ - global_space, constant_space
+   * - ``__attribute__((opencl_local))``
+ - local_space
+   * - ``__attribute__((opencl_private))``
+ - private_space
+
+
+.. code-block::
+
+   TODO: add support for `__attribute__((opencl_global_host))` and
+   `__attribute__((opencl_global_device))`.
+
+
+The default address space is "generic-memory", which is a virtual address space
+that overlaps the global, local, and private address spaces. SYCL mode enables
+conversion to/from the default address space from/to the address
+space-attributed type.
+
+The SPIR target allocates SYCL namespace scope variables in the global address
+space.
+
+Pointers to Default address space should get lowered into a pointer to a generic
+address space (or flat to reuse more general terminology). But depend

[PATCH] D99488: [SYCL][Doc] Add address space handling section to SYCL documentation

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/docs/SYCLSupport.md:821
+SYCL compiler toolchain. Section 3.8.2 of SYCL 2020 specification defines
+[memory 
model](https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_sycl_device_memory_model),
+section 4.7.7 - [address space 
classes](https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_address_space_classes)

Anastasia wrote:
> > The memory model for SYCL devices is based on the OpenCL 1.2 memory model.
> 
> Is this possibly a spec bug? OpenCL didn't have generic address space in 
> v1.2, it has only been added in v2.0.
> 
> https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#the-generic-address-space
> 
> 
I filed https://github.com/KhronosGroup/SYCL-Docs/issues/131 to clarify.



Comment at: clang/docs/SYCLSupport.md:830
+Similar to other single-source C++-based GPU programming modes like
+OpenMP/CUDA/HIP, SYCL uses clang's "default" address space for types with no
+address space attributes. This design has two important features: keeps the 
type system consistent with C++ on one hand and enable tools for emitting 
device code aligned with SPIR memory model (and other GPU targets).

Anastasia wrote:
> Is this explained somewhere would you be able to add any reference?
I wasn't able to find documentation for this implementation detail, but we 
should be able to confirm that by printing AST for example.

Here is the documentation I found for CUDA in llvm project:
 - https://llvm.org/docs/CompileCudaWithLLVM.html
 - https://llvm.org/docs/NVPTXUsage.html - defines LLVM IR representation for 
NVPTX.

NVIDIA documentation - 
https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#variable-memory-space-specifiers.
It says that memory space is assigned using "variable specifiers" rather than 
type qualifiers.



Comment at: clang/docs/SYCLSupport.md:851
+
+Changing variable type has massive and destructive effect in C++. For instance
+this does not compile in C++ for OpenCL mode:

Anastasia wrote:
> aaron.ballman wrote:
> > 
> > This example demonstrates the problem with compiling C++ code when address 
> > space type qualifiers are inferred.
> > 
> > The example compiles in accordance with OpenCL language semantic...
> > 
> > https://godbolt.org/z/9jzxK5xc4 - ToT clang doesn't compile this example.
> 
> I am still not clear what message you are trying to convey here? In OpenCL 
> kernel languages any object is always in some address space so if you write 
> the following `decltype(p)`, it will always have address space attribute in a 
> type. OpenCL spec is very explicit about this:
> 
> https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#addr-spaces-inference
> 
> So if you compare a type not attributed by an address space with an 
> attributed one they will never compare as equal because according to C++ 
> rules if the qualifiers differ the types will differ. You need to use a 
> special type trait to remove an address space if you need to compare types 
> not qualified by an address space. What is important to highlight however is 
> that address space inference is where OpenCL differs to C or C++. But of 
> course, neither C nor C++ have address spaces so it is hard to compare.
> 
> In relation to your documentation, it is not clear what you are trying to 
> achieve with this paragraph?
>  
> In relation to your documentation, it is not clear what you are trying to 
> achieve with this paragraph?

This paragraph provides clarification to the question why we can't apply OpenCL 
address space inference rules for SYCL mode.
I think it might be unnecessary because the SYCL specification defines address 
space deduction rules now.
Do you suggest removing this paragraph?



Comment at: clang/docs/SYCLSupport.md:909
+| `__attribute__((opencl_local))` | local_space |
+| `__attribute__((opencl_private))` | private_space |
+

Anastasia wrote:
> Since SYCL spec has constant AS you should explain whether it is going to be 
> supported or not and if so then how.
The first raw of this table covers mapping between SYCL constant_space and 
address space attribute.
Could you clarify what else do we need?



Comment at: clang/docs/SYCLSupport.md:914-919
+Default address space represents "Generic-memory", which is a virtual address
+space which overlaps the global, local and private address spaces. SYCL mode
+enables conversion to/from default address space from/to address space
+attributed type.
+
+SPIR target allocates SYCL namespace scope variables in global address space.

Anastasia wrote:
> Naghasan wrote:
> > aaron.ballman wrote:
> > > 
> > I think this section should be extended.
> > 
> > Pointers to `Default` address space should get lowered into a pointer to a 
> > generic address space (or flat to re

[PATCH] D99488: [SYCL][Doc] Add address space handling section to SYCL documentation

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 334420.
bader marked 24 inline comments as done.
bader added a comment.

Applied code review suggestions

Resolved merge conflicts with D99190 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99488/new/

https://reviews.llvm.org/D99488

Files:
  clang/docs/SYCLSupport.md

Index: clang/docs/SYCLSupport.md
===
--- clang/docs/SYCLSupport.md
+++ clang/docs/SYCLSupport.md
@@ -192,6 +192,124 @@
 document
 [SYCL Kernel Parameter Handling and Array Support](https://github.com/intel/llvm/blob/sycl/sycl/doc/KernelParameterPassing.md).
 
+### Address space handling
+
+The SYCL specification represents pointers to disjoint memory regions using C++
+wrapper classes on an accelerator to enable compilation with a standard C++
+toolchain and a SYCL compiler toolchain. Section 3.8.2 of SYCL 2020
+specification defines
+[memory model](https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_sycl_device_memory_model),
+section 4.7.7 - [address space classes](https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_address_space_classes)
+and section 5.9 covers [address space deduction](https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_address_space_deduction).
+
+The main address space semantic difference between SYCL and OpenCL is that SYCL
+doesn't perform the address space qualifier inference detailed in
+[OpenCL C v3.0 6.7.8](https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#addr-spaces-inference).
+
+Similar to other single-source C++-based GPU programming modes like
+OpenMP/CUDA/HIP, SYCL uses clang's "default" address space for types with no
+explicit address space attribute. This design has two important features: it
+keeps the type system consistent with C++ and enable tools for emitting device
+code aligned with SPIR memory model (and other GPU targets).
+
+So inside a function, this variable declaration:
+
+```C++
+int var;
+```
+
+The SYCL device compiler turns into:
+
+```C++
+VarDecl  var 'int'
+```
+
+while the OpenCL compiler turns it into:
+
+```C++
+VarDecl  var '__private int'
+```
+
+Changing the type of a variable can have observable effects in C++. For example,
+this does not compile in C++ for OpenCL mode:
+
+```C++
+template
+struct is_same {
+  static constexpr int value = 0;
+};
+
+template
+struct is_same {
+  static constexpr int value = 1;
+};
+
+void foo(int p) {
+  static_assert(is_same::value, "int is not an int?"); // Fails: p is '__private int' != 'int'
+  static_assert(is_same::value, "int* is not an int*?");  // Fails: p is '__private int*' != '__generic int*'
+}
+```
+
+`multi_ptr` class implementation example:
+
+``` C++
+// check that SYCL mode is ON and we can use non-standard decorations
+#if defined(__SYCL_DEVICE_ONLY__)
+// GPU/accelerator implementation
+template  class multi_ptr {
+  // DecoratedType applies corresponding address space attribute to the type T
+  // DecoratedType::type == "__attribute__((opencl_global)) T"
+  // See sycl/include/CL/sycl/access/access.hpp for more details
+  using pointer_t = typename DecoratedType::type *;
+
+  pointer_t m_Pointer;
+  public:
+  pointer_t get() { return m_Pointer; }
+  T& operator* () { return *reinterpret_cast(m_Pointer); }
+}
+#else
+// CPU/host implementation
+template  class multi_ptr {
+  T *m_Pointer; // regular undecorated pointer
+  public:
+  T *get() { return m_Pointer; }
+  T& operator* () { return *m_Pointer; }
+}
+#endif
+```
+
+Depending on the compiler mode, `multi_ptr` will either decorate its internal
+data with the address space attribute or not.
+
+To utilize clang's existing functionality, we reuse the following OpenCL address
+space attributes for pointers:
+
+| Address space attribute | SYCL address_space enumeration |
+|-||
+| `__attribute__((opencl_global))` | global_space, constant_space |
+| `__attribute__((opencl_local))` | local_space |
+| `__attribute__((opencl_private))` | private_space |
+
+TODO: add support for `__attribute__((opencl_global_host))` and
+`__attribute__((opencl_global_device))`.
+
+The default address space is "generic-memory", which is a virtual address space
+that overlaps the global, local, and private address spaces. SYCL mode enables
+conversion to/from the default address space from/to the address
+space-attributed type.
+
+The SPIR target allocates SYCL namespace scope variables in the global address
+space.
+
+Pointers to Default address space should get lowered into a pointer to a generic
+address space (or flat to reuse more general terminology). But depending on the
+allocation context, the default address space of a non-pointer type is assigned
+to a specific address space. This is described in
+https://www.khronos.org/registry/SYCL/specs/sycl-202

[PATCH] D99190: WIP: [SYCL] Add design document for SYCL mode

[Alexey Bader via Phabricator via cfe-commits]

bader added a subscriber: ABataev.
bader added inline comments.



Comment at: clang/docs/SYCLSupport.md:73
+the integration header is used (included) by the SYCL runtime implementation, 
so
+the header must be available before the host compilation starts.*
+

Naghasan wrote:
> > First, it must be possible to use any host compiler
> 
> I don't understand the link with the integration header. SYCL being 
> implementable as a library is a design principle of the specs but it doesn't 
> means the clang host compiler has to remain a vanilla C++ compiler.
> 
> > information provided in the integration header is used (included) by the 
> > SYCL runtime implementation, so the header must be available before the 
> > host compilation starts
> 
> Another approach to the integration header would be for clang as the host 
> compiler to generate the used type traits.
> > First, it must be possible to use any host compiler
> 
> I don't understand the link with the integration header. SYCL being 
> implementable as a library is a design principle of the specs but it doesn't 
> means the clang host compiler has to remain a vanilla C++ compiler.
> 
> > information provided in the integration header is used (included) by the 
> > SYCL runtime implementation, so the header must be available before the 
> > host compilation starts
> 
> Another approach to the integration header would be for clang as the host 
> compiler to generate the used type traits.

If there are no objections from @keryell, I'd like to prototype this approach 
for SYCL first to make sure there are no blocking issues.
This option seems to be worth to explore considering integration header 
approach disadvantages.



Comment at: clang/docs/SYCLSupport.md:123
+traverse all symbols accessible from kernel functions and add them to the
+"device part" of the code marking them with the new SYCL device attribute.
+

Naghasan wrote:
> OpenMP offload uses a similar approach isn't it? Might be worth to describe 
> how the 2 relates to each other and where they diverge. 
Do you mean the approach OpenMP compiler uses to outline single-source code 
parts to offload?
To be honest, I'm not sure... @ABataev, is there any description how OpenMP 
compiler outlines device code?
https://clang.llvm.org/docs/OpenMPSupport.html doesn't provide much details 
unfortunately.



Comment at: clang/docs/SYCLSupport.md:130
+`accessor` classes. Raw pointers map to kernel parameters one-to-one without
+additional transformations. `accessor` classes require additional processing as
+The "device" implementation of this class contains pointers to the device 
memory

Naghasan wrote:
> > Raw pointers map to kernel parameters one-to-one without additional 
> > transformations
> 
> Is this true ? I thought they should be passed as "pointer to global memory" 
> (under the OpenCL or CUDA model).
Good catch. I'll adjust the wording.



Comment at: clang/docs/SYCLSupport.md:132
+The "device" implementation of this class contains pointers to the device 
memory
+as a class member. OpenCL doesn't allow passing structures with pointer type
+members as kernel parameters. All memory objects shared between host and device

Naghasan wrote:
> Won't be better to refer to SPIR or SPIR-V kernels rather than OpenCL ?
> 
> Or even SPIR-like or SPIR-defacto to be even less normative.
Is it okay if I refer to OpenCL SPIR-V environment specification:
https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_Env.html#_kernels?



Comment at: clang/docs/SYCLSupport.md:134
+members as kernel parameters. All memory objects shared between host and device
+must be passed to the kernel as raw pointers.
+

Naghasan wrote:
> This is a bit ambiguous and leaves the impression you can't objects pass by 
> value.
Agree. Removed.



Comment at: clang/docs/SYCLSupport.md:161
+// Generated kernel function (expressed in OpenCL-like pseudo-code)
+__kernel KernelName(global int* a) {
+  KernelType KernelFuncObj; // Actually kernel function object declaration

Naghasan wrote:
> This is missing  the template instantiation  that will eventually lead to 
> that lowering.
> 
> I would also suggest to split code block in 2 as to mark what is in header 
> and source file (SYCL code) and what is compiler generated (that pseudo 
> OpenCL).
> 
> Might be good to also mention the glue generated in the integration header as 
> this is what allows arguments to be set by the runtime (bridge between the 
> structure in C++ and the SPIR-like kernel arguments).
> This is missing  the template instantiation  that will eventually lead to 
> that lowering.
> I would also suggest to split code block in 2 as to mark what is in header 
> and source file (SYCL code) and what is compiler generated (that pseudo 
> OpenCL).

Do you suggest to sketch an SYCL k

[PATCH] D99190: WIP: [SYCL] Add design document for SYCL mode

[Alexey Bader via Phabricator via cfe-commits]

bader marked 8 inline comments as done.
bader added a comment.

@Anastasia, I've addressed the comments for the address space section in 
https://reviews.llvm.org/D99488. Let's move discussion there.




Comment at: clang/docs/SYCLSupport.md:861
+space for types with no address space attributes. During the lowering to LLVM
+IR, the default address space is mapped to the SPIR generic address space.
+Declarations are assigned to the relevant memory region depending on their

Anastasia wrote:
> Ok this is an implementation details but from the language sematic it would 
> be good to describe what logic you are expecting.
> 
> So `Default` address space is primarily used for C/C++ flat memory which 
> means everything in standard C or C++ will be  in `Default` and this is where 
> the local memory is allocated too.
> 
> ```
> When not specified otherwise, objects are allocated by default in a generic 
> address space, which corresponds to the single address space of ISO/IEC 
> 9899:1999.
> 
> ```
> I am guessing this doesn't entirely apply to SYCL? I think this would be 
> important to clarify so it is clear what your semantic of `Default` is. It 
> would make sense to reference OpenCL generic address space or any other 
> documentation if you want to be concise.
I hope that the language semantics defined by the SYCL specification is clear 
enough. Please, see 
https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_address_space_deduction
 for details. The only thing left for implementation to define is address space 
assignment for namespace scope variables. I've added a clarification for that 
at the end of the paragraph.



Comment at: clang/docs/SYCLSupport.md:863
+Declarations are assigned to the relevant memory region depending on their
+declaration context and pointers to them are cast to generic. This design has
+two important features: keeps the type system consistent with C++ on one hand

Anastasia wrote:
> Ok, I suggested to lift this to where you describe the inference. It would be 
> good to elaborate on what objects are bound to what memory segments. You 
> might also refer to OpenCL spec since I believe the memory segments are 
> fairly similar. 
> 
> Can you explain this point a bit more `and pointers to them are cast to 
> generic`? Having an example might help too.
This is a description of how CodeGen library implements Target hooks like 
`getGlobalVarAddressSpace`/`getASTAllocaAddressSpace`. As it's not related to 
SYCL, I just removed this confusing sentence.



Comment at: clang/docs/SYCLSupport.md:864
+declaration context and pointers to them are cast to generic. This design has
+two important features: keeps the type system consistent with C++ on one hand
+and enable tools for emitting device code aligned with SPIR memory model (and

Anastasia wrote:
> Ok, I would put the design goals to the top. 
> 
> Btw I am not sure this is the case "keeps the type system consistent with 
> C++" since your semantic of default address spaces is different to C++. 
> Perhaps you can elaborate more what it means...
Moved right after the links to SYCL specification.

> Btw I am not sure this is the case "keeps the type system consistent with 
> C++" your semantic of default address spaces is different to C++

The point here is that SYCL compiler doesn't change standard C++ types by 
assigning non-default address space attribute implicitly. That way C++ types 
not using extensions are left intact.



Comment at: clang/docs/SYCLSupport.md:886
+
+Changing variable type has massive and destructive effect in C++. For instance
+this does not compile in C++ for OpenCL mode:

Anastasia wrote:
> I don't understand what is the message of this paragraph. The example 
> compiles in accordance with OpenCL language semantic... Perhaps you can 
> elaborate more.
This example demonstrates the problem with compiling C++ code when address 
space type qualifiers are inferred.

> The example compiles in accordance with OpenCL language semantic...

https://godbolt.org/z/9jzxK5xc4 - ToT clang doesn't compile this example.



Comment at: clang/docs/SYCLSupport.md:919
+> **NOTE**: although SYCL device compiler supports
+`__attribute__((opencl_constant))`, the use of this attribute is limited within
+SYCL implementation. An OpenCL constant pointer can not be casted to a pointer

Anastasia wrote:
> I am not sure what limited means? I would suggest being more specific and 
> state something like that there is no binding to the constant memory region. 
> Unless this is something that you intend to support later?
I removed this confusing section. It describes the functionality, which is not 
being considered for upstream yet.



Comment at: clang/docs/SYCLSupport.md:922
+with any other address space (including default).
+
+### Compiler/Runti

[PATCH] D99488: [SYCL][Doc] Add address space handling section to SYCL documentation

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 333853.
bader added a comment.

Applied code review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99488/new/

https://reviews.llvm.org/D99488

Files:
  clang/docs/SYCLSupport.md

Index: clang/docs/SYCLSupport.md
===
--- clang/docs/SYCLSupport.md
+++ clang/docs/SYCLSupport.md
@@ -813,6 +813,111 @@
 The SPIR-V specific functions are implemented in for the SYCL host device here:
 `sycl/source/spirv_ops.cpp`.
 
+### Address spaces handling
+
+SYCL specification uses C++ classes to represent pointers to disjoint memory
+regions on an accelerator to enable compilation with standard C++ toolchain and
+SYCL compiler toolchain. Section 3.8.2 of SYCL 2020 specification defines
+[memory model](https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_sycl_device_memory_model),
+section 4.7.7 - [address space classes](https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_address_space_classes)
+and section 5.9 covers [address space deduction](https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_address_space_deduction).
+
+The main address space semantic difference of SYCL mode from OpenCL is that
+SYCL doesn't perform address space qualifier inference detailed in
+[OpenCL C v3.0 s6.7.8](https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#addr-spaces-inference).
+
+Similar to other single-source C++-based GPU programming modes like
+OpenMP/CUDA/HIP, SYCL uses clang's "default" address space for types with no
+address space attributes. This design has two important features: keeps the type system consistent with C++ on one hand and enable tools for emitting device code aligned with SPIR memory model (and other GPU targets).
+
+So inside a function, this variable declaration:
+
+```C++
+int var;
+```
+
+SYCL device compiler turns into
+
+```C++
+VarDecl  var 'int'
+```
+
+OpenCL compiler turn into
+
+```C++
+VarDecl  var '__private int'
+```
+
+Changing variable type has massive and destructive effect in C++. For instance
+this does not compile in C++ for OpenCL mode:
+
+```C++
+template
+struct is_same {
+  static constexpr int value = 0;
+};
+
+template
+struct is_same {
+  static constexpr int value = 1;
+};
+
+void foo(int p) {
+  static_assert(is_same::value, "int is not an int?"); // Fails: p is '__private int' != 'int'
+  static_assert(is_same::value, "int* is not an int*?");  // Fails: p is '__private int*' != '__generic int*'
+}
+```
+
+`multi_ptr` class implementation example:
+
+``` C++
+// check that SYCL mode is ON and we can use non-standard decorations
+#if defined(__SYCL_DEVICE_ONLY__)
+// GPU/accelerator implementation
+template  class multi_ptr {
+  // DecoratedType applies corresponding address space attribute to the type T
+  // DecoratedType::type == "__attribute__((opencl_global)) T"
+  // See sycl/include/CL/sycl/access/access.hpp for more details
+  using pointer_t = typename DecoratedType::type *;
+
+  pointer_t m_Pointer;
+  public:
+  pointer_t get() { return m_Pointer; }
+  T& operator* () { return *reinterpret_cast(m_Pointer); }
+}
+#else
+// CPU/host implementation
+template  class multi_ptr {
+  T *m_Pointer; // regular undecorated pointer
+  public:
+  T *get() { return m_Pointer; }
+  T& operator* () { return *m_Pointer; }
+}
+#endif
+```
+
+Depending on the compiler mode `multi_ptr` will either decorate internal data
+with address space attribute or not.
+
+To utilize existing clang's functionality, we re-use following OpenCL address
+space attributes for decoration pointers implementation:
+
+| Address space attribute | SYCL address_space enumeration |
+|-||
+| `__attribute__((opencl_global))` | global_space, constant_space |
+| `__attribute__((opencl_local))` | local_space |
+| `__attribute__((opencl_private))` | private_space |
+
+TODO: add support for `__attribute__((opencl_global_host))` and
+`__attribute__((opencl_global_device))`.
+
+Default address space represents "Generic-memory", which is a virtual address
+space which overlaps the global, local and private address spaces. SYCL mode
+enables conversion to/from default address space from/to address space
+attributed type.
+
+SPIR target allocates SYCL namespace scope variables in global address space.
+
 ### Compiler/Runtime interface
 
 ## SYCL Runtime architecture
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99488: [SYCL][Doc] Add address space handling section to SYCL documentation

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D99488#2655326 , @Anastasia wrote:

> Thanks. I am guessing you will address relevant review comments from D99190 
>  in here?

Yes. I'm working on it. I wanted to make it in a separate update, so it should 
be easy to see what has changed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99488/new/

https://reviews.llvm.org/D99488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99190: WIP: [SYCL] Add design document for SYCL mode

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 333780.
bader added a comment.

Move address space handling section to https://reviews.llvm.org/D99488 to 
address https://reviews.llvm.org/D89909#2653452.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99190/new/

https://reviews.llvm.org/D99190

Files:
  clang/docs/SYCLSupport.md
  clang/docs/images/Compiler-HLD.svg
  clang/docs/images/DeviceCodeSplit.svg
  clang/docs/images/DeviceLinkAndWrap.svg
  clang/docs/images/DevicePTXProcessing.svg
  clang/docs/images/SplitCompileAndLink.svg

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits