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

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D89909#2606180 , @Anastasia wrote:

> In D89909#2600859 , @aaron.ballman 
> wrote:
>
>> Just a few minor nits from me, but I'm mostly wondering: where are we at 
>> with this and are there still substantive changes required? (I looked 
>> through the comments, but there's a lot of back-and-forth since Oct and I'm 
>> not certain what's holding the patch back currently.)
>
> To make it short, from my side I am not very clear about the overall design. 
> From the SYCL spec side, there is no indication of what compiler extensions 
> are needed and if at all. As a result, some of the design choices are unclear 
> to me - in particular why SPIR target would need a separate address space map 
> for SYCL. This is not how it was intended originally and I am worried that 
> this will create issues for the consumers of IR to handle two different 
> formats. But in general, if the community is now to maintain this code we 
> should at least have some deeper understanding of it.
>
> I would suggest starting from some high-level documentation that provides the 
> details of the compiler extension being implemented. Perhaps the 
> documentation that @bader has linked earlier could be used as a starting 
> point with some more details that would allow assessing and reviewing the 
> changes.

@Anastasia, do you suggest we copy 
https://github.com/intel/llvm/blob/sycl/sycl/doc/CompilerAndRuntimeDesign.md 
document to clang/docs within this patch?


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] D95624: [OpenCL][PR48896] Fix default address space in template argument deduction

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/test/SemaOpenCLCXX/address-space-templates.cl:60
+  // Preserve the address space of the type in forwarding reference.
+  // CHECK: CXXMethodDecl {{.*}} operator() 'void (__private int &__private) 
const __generic'
+  foo4(i, [](auto&& x){;});

This check fails on 32-bit Windows platform where compiler adds  
`__attribute__((thiscall))` both constructor and call operator.

Something like this should fix the problem:

```
  // CHECK: |-CXXConstructorDecl {{.*}} rep 'void (const __generic rep 
&__private){{.*}} __generic'
  // CHECK: CXXMethodDecl {{.*}} operator() 'void (__private int 
&__private){{.*}} const __generic'
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95624

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

[PATCH] D97573: [OpenMP] Handle non-function context before checking for diagnostic emission

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97573

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

[PATCH] D97717: [SYCL] Rework the SYCL driver options

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

@aaron.ballman, it looks like unittests should be updated as well. Please, take 
a look at failures in pre-merge checks.




Comment at: clang/include/clang/Basic/LangOptions.def:252
 LANGOPT(SYCLIsDevice  , 1, 0, "Generate code for SYCL device")
+LANGOPT(SYCLIsHost, 1, 0, "SYCL host compilation")
 ENUM_LANGOPT(SYCLVersion  , SYCLMajorVersion, 1, SYCL_None, "Version of the 
SYCL standard used")

I'm okay with that change, but IIRC, @ABataev suggested to have both -fsycl and 
-fsycl-is-device options to align with OpenMP mode (full discussion is here - 
https://reviews.llvm.org/D72857#inline-674377).


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

https://reviews.llvm.org/D97717

___
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 328100.
bader marked 4 inline comments as done.
bader added a comment.

Apply suggestions from Aaron.


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.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-of-returns.cpp
  clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp
  clang/test/CodeGenSYCL/address-spaces-struct.cpp
  clang/test/CodeGenSYCL/address-spaces.cpp
  clang/test/SemaSYCL/address-space-parameter-conversions.cpp
  clang/test/SemaTemplate/address_space-dependent.cpp
  llvm/include/llvm/ADT/Triple.h
  llvm/lib/Support/Triple.cpp

Index: llvm/lib/Support/Triple.cpp
===
--- llvm/lib/Support/Triple.cpp
+++ llvm/lib/Support/Triple.cpp
@@ -247,6 +247,8 @@
   case Musl: return "musl";
   case MuslEABI: return "musleabi";
   case MuslEABIHF: return "musleabihf";
+  case SYCLDevice:
+return "sycldevice";
   case Simulator: return "simulator";
   }
 
@@ -554,6 +556,7 @@
   .StartsWith("itanium", Triple::Itanium)
   .StartsWith("cygnus", Triple::Cygnus)
   .StartsWith("coreclr", Triple::CoreCLR)
+  .StartsWith("sycldevice", Triple::SYCLDevice)
   .StartsWith("simulator", Triple::Simulator)
   .StartsWith("macabi", Triple::MacABI)
   .Default(Triple::UnknownEnvironment);
Index: llvm/include/llvm/ADT/Triple.h
===
--- llvm/include/llvm/ADT/Triple.h
+++ llvm/include/llvm/ADT/Triple.h
@@ -222,8 +222,9 @@
 Itanium,
 Cygnus,
 CoreCLR,
+SYCLDevice,
 Simulator, // Simulator variants of other systems, e.g., Apple's iOS
-MacABI, // Mac Catalyst variant of Apple's iOS deployment target.
+MacABI,// Mac Catalyst variant of Apple's iOS deployment target.
 LastEnvironmentType = MacABI
   };
   enum ObjectFormatType {
@@ -497,6 +498,10 @@
isMacCatalystEnvironment()));
   }
 
+  bool isSYCLDeviceEnvironment() const {
+return getEnvironment() == Triple::SYCLDevice;
+  }
+
   bool isOSNetBSD() const {
 return getOS() == Triple::NetBSD;
   }
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-parameter-conversions.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/address-space-parameter-conversions.cpp
@@ -0,0 +1,57 @@
+// RUN: %clang_cc1 -fsycl -fsycl-is-device -verify -fsyntax-only -x c++ %s
+
+void bar(int ) {}
+void bar2(int ) {}
+void bar(__attribute__((opencl_private)) int ) {}
+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;
+
+  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);
+  // FIXME: determine if we can warn on the below conversions.
+  int *i = GLOB;
+  

[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] 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/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, 

[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 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 ) {}
+void bar2(int ) {}
+void bar(__attribute__((opencl_private)) int ) {}
+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_]+]] 

[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 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 ) {}
+void bar2(int ) {}
+void bar(__attribute__((opencl_private)) int ) {}
+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_addr 

[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?

> 

[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 
> > > > > > > > > 

[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 ) {}
+void bar2(int ) {}
+void bar(__attribute__((opencl_private)) int ) {}
+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 -emit-llvm -x c++ 

[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 = 
+AddrSpaceMap = Triple.getEnvironment() == llvm::Triple::SYCLDevice
+   ? 

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/placeholder address 
> space that can 

[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 ) {}
+void bar2(int ) {}
+void bar(__attribute__((opencl_private)) int ) {}
+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 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 ) {}
+void bar2(int ) {}
+void bar(__attribute__((opencl_private)) int ) {}
+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 @@
+// 

[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] 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] 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] 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 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] 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] 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 );
+

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

[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 

[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:
> > > 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] 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] 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] 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] 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 ,
+   llvm::opt::ArgStringList );
+
+void constructTranslateCommand(Compilation , const Tool ,
+   const JobAction , const InputInfo ,
+   const InputInfo ,
+   const llvm::opt::ArgStringList );
+
+class LLVM_LIBRARY_VISIBILITY Translator : public Tool {
+public:
+  Translator(const ToolChain )
+  : Tool("SPIR-V::Translator", "llvm-spirv", TC) {}
+
+  bool hasIntegratedCPP() const override { return false; }
+  bool hasIntegratedAssembler() const override { return true; }
+
+  void ConstructJob(Compilation , const JobAction ,
+const InputInfo , const InputInfoList ,
+const llvm::opt::ArgList ,
+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 , const Tool ,
+  const JobAction ,
+  const InputInfo ,
+  const InputInfo ,
+  const llvm::opt::ArgStringList ) {
+  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 , const JobAction ,
+ const InputInfo ,
+ const InputInfoList ,
+ const ArgList ,
+ 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.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]

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 , const Tool ,
+  const JobAction ,

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 )
+  : 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] 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 ) : CommonSPIRABIInfo(CGT) {}
+  void computeInfo(CGFunctionInfo ) const override;
+
+private:
+  ABIArgInfo classifyKernelArgumentType(QualType Ty) const;
+};
 } // end anonymous namespace
 namespace {
 class CommonSPIRTargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   CommonSPIRTargetCodeGenInfo(CodeGen::CodeGenTypes )
   : 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 )
+  : CommonSPIRTargetCodeGenInfo(std::make_unique(CGT)) {}
+  void setCUDAKernelCallingConvention(const FunctionType *) 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 ) 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  : 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 , CGFunctionInfo ) {
-  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 *) 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 , QualType QType,
const CodeGen::CodeGenModule ,
TypeStringCache );
@@ -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(Types));
+return SetCGInfo(new 

[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] 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 , 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 ,
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 , 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 ,
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
   // 

[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] 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] 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] 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] 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] 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] 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]

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] 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] 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 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 , 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] 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 , 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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