[PATCH] D156743: clang/OpenCL: Add inline implementations of sqrt in builtin header

[Anastasia Stulova via Phabricator via cfe-commits]

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

If we think there are no better alternatives and implementation is generic 
enough for every vendor, LGTM!


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

https://reviews.llvm.org/D156743

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

[PATCH] D155978: [SPIRV] Add SPIR-V logical triple.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: llvm/unittests/TargetParser/TripleTest.cpp:1307
+  EXPECT_TRUE(T.isSPIRV());
+
   T.setArch(Triple::spirv32);

pmatos wrote:
> Keenuts wrote:
> > pmatos wrote:
> > > I am not well-versed in SPIRV for gfx but if we are using 32bits in SPIRV 
> > > logical, can't we reuse spirv32? Is there some sort of strong reason not 
> > > to or adding spirv for logical spirv as an alternative to spirv32 and 
> > > spirv64 is just easier?
> > This is a very valid question! And I'm not sure of the best option.
> > My understanding is: in logical SPIR-V, there are no notion of pointer 
> > size, or architecture size. We cannot offset pointers by a sizeof(), or do 
> > such things.
> > 
> > But the options I had didn't seem to allow this kind of "undefined 
> > architecture".
> > I chose 32bits here because the IDs are 32bit. But pointer addresses are 
> > not, so returning 32bit is not quite right either.
> > 
> > 
> This is a tricky one tbh - I would have to do a bit more research to have a 
> more insighful reply here. Maybe @mpaszkowski or @Anastasia can add more.
I think to do this properly in LLVM would require an IR extension or something, 
it is maybe worth starting RFC to discuss this.

Out of curiosity do you have any code that can be compiled from any GFX source 
language that would need a pointer size to be emitted in IR? If there is no 
code that can be written in such a way then using one fixed pointer width could 
be ok. Then the SPIR-V backend should just recognise it and lower as required. 
Otherwise target configurable types might be useful: 
https://llvm.org/docs/LangRef.html#target-extension-type

In general we tried to avoid adding bitwidth neutral SPIR-V triple originally 
just because LLVM always requires a pointer width. We were thinking of adding 
vulkan as a component to the existing SPIR-V 34|64 targets 
https://discourse.llvm.org/t/setting-version-environment-and-extensions-for-spir-v-target.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155978

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

[PATCH] D156743: clang/OpenCL: Add inline implementations of sqrt in builtin header

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Headers/opencl-c-base.h:832
+
+inline float __ovld __cnfn sqrt(float __x) {
+  return __builtin_elementwise_sqrt(__x);

arsenm wrote:
> svenvh wrote:
> > Anastasia wrote:
> > > Is this a generic implementation enough? Would some targets not need to 
> > > do something different for this built-in?
> > > 
> > > Ideally this header is to be kept light so I am a bit worried about 
> > > adding definitions of the functions here. Otherwise we will end up in the 
> > > same situation as we one day were with opencl-c.h. So could these be left 
> > > there instead? It might be good to check with @svenvh if TableGen header 
> > > has already a way to do this function forwarding or can be extended to do 
> > > such a thing. Then it would be implementable in the both header 
> > > mechanisms. I don't know if Sven has some other ideas or opinions...
> > We did already discuss this a bit on the GitHub issue: 
> > https://github.com/llvm/llvm-project/issues/64264
> As I mentioned on the ticket, it's only this one case so I'm not worried 
> about adding a lot more to the base header. I think we can start by assuming 
> llvm.sqrt always works correctly, I don't want to add more complexity to 
> handle this case without a specific reason
> As I mentioned on the ticket, it's only this one case so I'm not worried 
> about adding a lot more to the base header.

This is how things normally start. Someone else might want to continue this 
approach because it is already there.

>I think we can start by assuming llvm.sqrt always works correctly, I don't 
>want to add more complexity to handle this case without a specific reason

Do you mean it would apply to all implementations? What I am missing here is 
why it is required to be in the headers? Is this because it needs to be inlined 
or is it because the compiler must see `__builtin_elementwise_sqrt` with the 
surrounding code where it is called from?


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

https://reviews.llvm.org/D156743

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

[PATCH] D156743: clang/OpenCL: Add inline implementations of sqrt in builtin header

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Headers/opencl-c-base.h:832
+
+inline float __ovld __cnfn sqrt(float __x) {
+  return __builtin_elementwise_sqrt(__x);

Is this a generic implementation enough? Would some targets not need to do 
something different for this built-in?

Ideally this header is to be kept light so I am a bit worried about adding 
definitions of the functions here. Otherwise we will end up in the same 
situation as we one day were with opencl-c.h. So could these be left there 
instead? It might be good to check with @svenvh if TableGen header has already 
a way to do this function forwarding or can be extended to do such a thing. 
Then it would be implementable in the both header mechanisms. I don't know if 
Sven has some other ideas or opinions...


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

https://reviews.llvm.org/D156743

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

[PATCH] D156816: [Clang] Make generic aliases to OpenCL address spaces

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

> We also are tied to OpenCL semantic of future versions.

This is what I would like to avoid aliasing to OpenCL and then starting to 
change it in a way that is not conforming or documented. Maybe there is a need 
for something like an address space for the group of related languages or 
something like that and also a way to derive and specialise from the class 
instead of forking and copying/pasting.

There were a number of proposals for similar things in the past including 
improvement to the target specific address spaces 
https://reviews.llvm.org/D62574.

If what we currently have is not good enough for all purposes perhaps it would 
make sense to start an RFC describing the problems and the requirements and 
then working out the solution that works for all the use cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156816

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

[PATCH] D156816: [Clang] Make generic aliases to OpenCL address spaces

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D156816#4551357 , @jhuber6 wrote:

> In D156816#4551338 , @yaxunl wrote:
>
>>> FFI isn't the reason you'd use these, it's for generic access to the actual 
>>> backend. E.g. an `addrspace(3)` global is local memory, if it's external 
>>> it's dynamic. Having these named is better than doing it via the numerical 
>>> address space. I'd like to use these in the C++ / OpenMP codes instead of 
>>> the numeric ones but I don't like needing to use `opencl` in the name. 
>>> Similarly to how we have the OpenCL atomics that should be usable outside 
>>> of OpenCL.
>>
>> I agree these attributes are useful in other languages, but "global" and 
>> "local" may need a more generic name suitable for all offloading languages. 
>> To me, "device" can be a good alternative to "global". even "shared" seems 
>> clearer than "local".
>
> Global is common in https://llvm.org/docs/AMDGPUUsage.html#address-spaces and 
> https://llvm.org/docs/NVPTXUsage.html#address-spaces. The main problem is 
> `local` vs `shared` and `private` vs `local`. Unsure which one we should 
> prefer in this case. Generally I feel a lot of this OpenCL stuff should've 
> been named commonly at the start considering you can use most of them outside 
> of the actual OpenCL language just fine.

Why not to just use target address space and define it to some macro with 
desirable spelling?

I don't think renaming OpenCL address space to something else makes sense. It 
might make more sense to just introduced different model of address spaces 
completely. But if you use OpenCL ones then it makes sense to have adequate 
naming so its documentation and etc can be located.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156816

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

[PATCH] D151339: [OpenCL] Add cl_ext_image_raw10_raw12 extension

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia 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/D151339/new/

https://reviews.llvm.org/D151339

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

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

I don't feel `reinterpret_cast` is the right fit for this as it's expected to 
do just reinterpretation but `addrspacecast` LLVM instruction which Clang 
generates might result in some more operations in particular some special 
instructions for address calculation. Would it work if you enable 
`addrspace_cast` with double underscore prefix directly in C++ or something 
like this? Otherwise C-cast would be just as good as changing reinterpret cast 
in this way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151087

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

[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Anastasia Stulova via Phabricator via cfe-commits]

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

LGTM! Thanks


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

https://reviews.llvm.org/D143849

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

[Anastasia Stulova via Phabricator via cfe-commits]

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

LGTM! Thanks




Comment at: clang/lib/CodeGen/TargetInfo.h:383
+
+  /// Return an LLVM target extension type that corresponds to an OpenCL type,
+  /// if such a type is necessary.

The default behaviour doesn't return a target extension type so maybe it's 
better to change the comment to:


```
Return an LLVM type that corresponds to an OpenCL type
```


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] D143348: [Clang][Doc][OpenCL] Release 16 notes

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia closed this revision.
Anastasia added a comment.

Committed in 
https://github.com/llvm/llvm-project/commit/639db8a90d2adf7789b9d3cec3a7c26502bf819b


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

https://reviews.llvm.org/D143348

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

[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:9494
+  // of SVM.
+  if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&
+  (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam))

Ayal wrote:
> Anastasia wrote:
> > Anastasia wrote:
> > > Ayal wrote:
> > > > Anastasia wrote:
> > > > > I think it should be possible to merge this with `if` below to avoid 
> > > > > condition duplication.
> > > > > 
> > > > > 
> > > > Sure, but that trades one duplication for another, rather than clearly 
> > > > separating the early-continue case early?
> > > > 
> > > > ```
> > > >   if (ParamType == PtrKernelParam || ParamType == 
> > > > PtrPtrKernelParam) {
> > > > if (S.getLangOpts().getOpenCLCompatibleVersion() > 120)
> > > >   continue;
> > > > S.Diag(Param->getLocation(),
> > > >diag::err_record_with_pointers_kernel_param)
> > > >   << PT->isUnionType()
> > > >   << PT;
> > > >   } else if (ParamType == InvalidAddrSpacePtrKernelParam) {
> > > > S.Diag(Param->getLocation(),
> > > >diag::err_record_with_pointers_kernel_param)
> > > >   << PT->isUnionType()
> > > >   << PT;
> > > >   } else {
> > > > S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) 
> > > > << PT;
> > > > 
> > > > ```
> > > I am mainly thinking in terms of maintenance if for example someone fixes 
> > > one if and forgets another. Or if ifs will get separated by some other 
> > > code and then it is not easy to see that the same thing is handled 
> > > differently in OpenCL versions. 
> > > 
> > > Unfortunately we have a lot of those cases, I know this function has 
> > > early exists but it is not a common style.
> > > 
> > > 
> > > I was talking about something like:
> > > 
> > > 
> > > ```
> > > if (((S.getLangOpts().getOpenCLCompatibleVersion() <= 120) &&
> > > (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam)) ||
> > >   ParamType == InvalidAddrSpacePtrKernelParam)
> > > ```
> > > 
> > > It would also be ok to separate `InvalidAddrSpacePtrKernelParam` since 
> > > it's handling different feature.
> > Sorry I have forgotten that this part of the code is expected to handle the 
> > diagnostics only. The decision that the kernel parameter is wrong is done 
> > in `getOpenCLKernelParameterType`. I think you should alter the conditions 
> > there to check for OpenCL version and avoid classifying cases you care 
> > about as `PtrKernelParam` or `PtrPtrKernelParam`. Then here you wouldn't 
> > need this extra if/continue block. 
> Hmm, would that be better? This part of the code, namely 
> `checkIsValidOpenCLKernelParameter()`, does check the validity of arguments 
> classified by `getOpenCLKernelParameterType()` in addition to handling 
> diagnostics. E.g., the first case above decides that arguments of 
> pointer-to-pointer type are wrong along with proper diagnostics for OpenCL 
> 1.x while allowing them for other OpenCL versions.
> 
> Struct arguments are simply classified as records by 
> getOpenCLKernelParameterType(), whereas this part of the code traverses each 
> struct and calls getOpenCLKernelParameterType() on each field - the latter 
> seems  unaware if it was invoked on a struct field or not? If it is (made) 
> aware, it could indeed return a (new kind of?) invalid type instead of 
> pointer type for OpenCL 1.x - how would the right 
> err_record_with_pointers_kernel_param diagnostics then be handled? If 
> desired, such refactoring should probably be done independent of this fix?
That's right there is a mix of everything as I think this code has deviated 
from its original idea, but I still think it's cleaner to avoid doing the same 
kind of checks in multiple places.


Overall I find this code a bit over-engineered, this is maybe why it went off 
track. So some refactoring would indeed be helpful. However I am not sure 
whether it's better to continue the same route or try to simplify the design by 
just adding separate functions for each error case that would detect that the 
type is of a certain kind e.g a pointer or a pointer with wrong address space 
and then also provide the diagnostic straight away . I feel this would match 
better the rest of the diagnostic handling in clang but might result in 
slightly more helper functions or duplication of code.


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

https://reviews.llvm.org/D143849

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

[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:9494
+  // of SVM.
+  if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&
+  (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam))

Anastasia wrote:
> Ayal wrote:
> > Anastasia wrote:
> > > I think it should be possible to merge this with `if` below to avoid 
> > > condition duplication.
> > > 
> > > 
> > Sure, but that trades one duplication for another, rather than clearly 
> > separating the early-continue case early?
> > 
> > ```
> >   if (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam) {
> > if (S.getLangOpts().getOpenCLCompatibleVersion() > 120)
> >   continue;
> > S.Diag(Param->getLocation(),
> >diag::err_record_with_pointers_kernel_param)
> >   << PT->isUnionType()
> >   << PT;
> >   } else if (ParamType == InvalidAddrSpacePtrKernelParam) {
> > S.Diag(Param->getLocation(),
> >diag::err_record_with_pointers_kernel_param)
> >   << PT->isUnionType()
> >   << PT;
> >   } else {
> > S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT;
> > 
> > ```
> I am mainly thinking in terms of maintenance if for example someone fixes one 
> if and forgets another. Or if ifs will get separated by some other code and 
> then it is not easy to see that the same thing is handled differently in 
> OpenCL versions. 
> 
> Unfortunately we have a lot of those cases, I know this function has early 
> exists but it is not a common style.
> 
> 
> I was talking about something like:
> 
> 
> ```
> if (((S.getLangOpts().getOpenCLCompatibleVersion() <= 120) &&
> (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam)) ||
>   ParamType == InvalidAddrSpacePtrKernelParam)
> ```
> 
> It would also be ok to separate `InvalidAddrSpacePtrKernelParam` since it's 
> handling different feature.
Sorry I have forgotten that this part of the code is expected to handle the 
diagnostics only. The decision that the kernel parameter is wrong is done in 
`getOpenCLKernelParameterType`. I think you should alter the conditions there 
to check for OpenCL version and avoid classifying cases you care about as 
`PtrKernelParam` or `PtrPtrKernelParam`. Then here you wouldn't need this extra 
if/continue block. 


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

https://reviews.llvm.org/D143849

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

[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:9494
+  // of SVM.
+  if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&
+  (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam))

Ayal wrote:
> Anastasia wrote:
> > I think it should be possible to merge this with `if` below to avoid 
> > condition duplication.
> > 
> > 
> Sure, but that trades one duplication for another, rather than clearly 
> separating the early-continue case early?
> 
> ```
>   if (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam) {
> if (S.getLangOpts().getOpenCLCompatibleVersion() > 120)
>   continue;
> S.Diag(Param->getLocation(),
>diag::err_record_with_pointers_kernel_param)
>   << PT->isUnionType()
>   << PT;
>   } else if (ParamType == InvalidAddrSpacePtrKernelParam) {
> S.Diag(Param->getLocation(),
>diag::err_record_with_pointers_kernel_param)
>   << PT->isUnionType()
>   << PT;
>   } else {
> S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT;
> 
> ```
I am mainly thinking in terms of maintenance if for example someone fixes one 
if and forgets another. Or if ifs will get separated by some other code and 
then it is not easy to see that the same thing is handled differently in OpenCL 
versions. 

Unfortunately we have a lot of those cases, I know this function has early 
exists but it is not a common style.


I was talking about something like:


```
if (((S.getLangOpts().getOpenCLCompatibleVersion() <= 120) &&
(ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam)) ||
  ParamType == InvalidAddrSpacePtrKernelParam)
```

It would also be ok to separate `InvalidAddrSpacePtrKernelParam` since it's 
handling different feature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143849

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

[PATCH] D143348: [Clang][Doc][OpenCL] Release 16 notes

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:840
+  * Fixed conditional definition of the depth image and read_write image3d 
builtins.
+  * Added ``nounwind`` attribute to all builtin functions.
+

svenvh wrote:
> It's slightly more than that: clang adds `nounwind` not only for builtin 
> functions, but for any OpenCL function call.
Thanks, this makes sense... stack unwind in OpenCL kernel is meaningless atm.

However has this change been made in a separate commit?


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

https://reviews.llvm.org/D143348

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

[PATCH] D143348: [Clang][Doc][OpenCL] Release 16 notes

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 496790.
Anastasia added a comment.

Updated bullet about nounwind attr


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

https://reviews.llvm.org/D143348

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -828,10 +828,17 @@
 Objective-C Language Changes in Clang
 -
 
-OpenCL C Language Changes in Clang
+OpenCL Kernel Language Changes in Clang
 --
 
-...
+- Improved diagnostics for C++ templates used in kernel arguments.
+- Removed redundant pragma for the ``arm-integer-dot-product`` extension.
+- Improved support of enqueued block for AMDGPU.
+- Improved builtin functions support:
+  * Allow disabling default header-based feature/extension support by passing 
``-D__undef_``.
+  * Fixed conditional definition of the depth image and read_write image3d 
builtins.
+  * Added ``nounwind`` attribute implicitly to all functions.
+
 
 ABI Changes in Clang
 


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -828,10 +828,17 @@
 Objective-C Language Changes in Clang
 -
 
-OpenCL C Language Changes in Clang
+OpenCL Kernel Language Changes in Clang
 --
 
-...
+- Improved diagnostics for C++ templates used in kernel arguments.
+- Removed redundant pragma for the ``arm-integer-dot-product`` extension.
+- Improved support of enqueued block for AMDGPU.
+- Improved builtin functions support:
+  * Allow disabling default header-based feature/extension support by passing ``-D__undef_``.
+  * Fixed conditional definition of the depth image and read_write image3d builtins.
+  * Added ``nounwind`` attribute implicitly to all functions.
+
 
 ABI Changes in Clang
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

I feel that originally pointers were disallowed because they create the same 
issue as `size_t` and etc as their size is implementation depended but the same 
logic applies to images and other types that are even more implementation 
depended. Overall this bit of the spec is very inconsistent so I don't mind if 
we change the behavior to be whatever we find more helpful. However I would 
encourage to submit an issue to OpenCL-Docs to point out this inconsistency.




Comment at: clang/lib/Sema/SemaDecl.cpp:9493
+  // union. This restriction was lifted in OpenCL v2.0 with the 
introduction
+  // of SVM.
+  if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&

To be honest I feel like it was a bug fix? Do you happen to have any record of 
the fix?



Comment at: clang/lib/Sema/SemaDecl.cpp:9494
+  // of SVM.
+  if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&
+  (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam))

I think it should be possible to merge this with `if` below to avoid condition 
duplication.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143849

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

[PATCH] D143472: [clang] Cast the pointer to right type with different address space

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:12412
   // Cast the pointer to right type.
-  Value *Ptr = CGF.Builder.CreateBitCast(Ops[0],
-   
llvm::PointerType::getUnqual(Ops[1]->getType()));
+  Value *Ptr = CGF.Builder.CreatePointerBitCastOrAddrSpaceCast(
+  Ops[0], llvm::PointerType::getUnqual(Ops[1]->getType()));

I think the semantics of builtin functions is that they accept any address 
space. So you should preserve the address space from Ops[1].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143472

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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/CodeGen/CGOpenCLRuntime.cpp:40
 
   switch (cast(T)->getKind()) {
   default:

I am not sure if this can be called a generic implementation as it has always 
been a workaround to compensate for absence of representation in LLVM IR... we 
should probably move such code into default definition of 
`TargetInfo::getOpenCLType`...



Comment at: clang/lib/CodeGen/TargetInfo.cpp:10986
+  // for more details).
+  SmallVector IntParams = {0, 0, 0, 0, 0, 0};
+

I think the list initialization doesn't do what you are trying to achieve with 
`SmallVector` as it appends the elements. You should probably just be using 
constructor with size i.e. IntParams(6)?



Comment at: clang/lib/CodeGen/TargetInfo.cpp:10989
+  // Choose the dimension of the image--this corresponds to the Dim parameter
+  // (https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#_dim), or
+  // the instances of Dim in llvm/lib/Target/SPIRV/SPIRVSymbolicOperands.td.

Is this URL likely to stay the same? We normally just add a spec section number 
e.g.

```
SPIR-V spec v1.6 rev2 s3.8.
```



Comment at: clang/lib/CodeGen/TargetInfo.cpp:10990
+  // (https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#_dim), or
+  // the instances of Dim in llvm/lib/Target/SPIRV/SPIRVSymbolicOperands.td.
+  if (OpenCLName.startswith("image2d"))

We should avoid adding file paths and names as they can change without updating 
the comment.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:10992
+  if (OpenCLName.startswith("image2d"))
+IntParams[0] = 1; // 1D
+  else if (OpenCLName.startswith("image3d"))

Ok, is the order of parameters defined or documented somewhere? Would it make 
sense to create some kind of a local enum map containing the indices and use 
enum members instead of constants to improve readability/maintenance?



Comment at: clang/lib/CodeGen/TargetInfo.cpp:11015
+
+llvm::Type *CommonSPIRTargetCodeGenInfo::getOpenCLType(CodeGenModule &CGM,
+   const Type *Ty) const {

Ok, so the old SPIR format will change too!

I don't have any objections but want to make sure it's not accidental.  


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] D143348: [Clang][Doc][OpenCL] Release 16 notes

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added a reviewer: svenvh.
Herald added subscribers: Naghasan, ebevhan, yaxunl.
Herald added a project: All.
Anastasia requested review of this revision.

Documented major OpenCL features in release 16.


https://reviews.llvm.org/D143348

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -828,10 +828,17 @@
 Objective-C Language Changes in Clang
 -
 
-OpenCL C Language Changes in Clang
+OpenCL Kernel Language Changes in Clang
 --
 
-...
+- Improved diagnostics for C++ templates used in kernel arguments.
+- Removed redundant pragma for the ``arm-integer-dot-product`` extension.
+- Improved support of enqueued block for AMDGPU.
+- Improved builtin functions support:
+  * Allow disabling default header-based feature/extension support by passing 
``-D__undef_``.
+  * Fixed conditional definition of the depth image and read_write image3d 
builtins.
+  * Added ``nounwind`` attribute to all builtin functions.
+
 
 ABI Changes in Clang
 


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -828,10 +828,17 @@
 Objective-C Language Changes in Clang
 -
 
-OpenCL C Language Changes in Clang
+OpenCL Kernel Language Changes in Clang
 --
 
-...
+- Improved diagnostics for C++ templates used in kernel arguments.
+- Removed redundant pragma for the ``arm-integer-dot-product`` extension.
+- Improved support of enqueued block for AMDGPU.
+- Improved builtin functions support:
+  * Allow disabling default header-based feature/extension support by passing ``-D__undef_``.
+  * Fixed conditional definition of the depth image and read_write image3d builtins.
+  * Added ``nounwind`` attribute to all builtin functions.
+
 
 ABI Changes in Clang
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142948: [OpenCL] Disable vector to scalar types coercion for OpenCL

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

It doesn't seem to make sense to have a `LangOpt` for this, if anything a new 
`CodeGenOpt` feels like a better fit. Then you should probably extend existing 
classify functions to prevent coercion instead of adding yet another function 
otherwise you will need to handle all the different corner cases again as there 
seem to be quite many if I look at `X86_32ABIInfo::classifyReturnType`...

FYI OpenCL vectors are implemented as generic vector extension so this probably 
shouldn't be bind to OpenCL at all:
https://clang.llvm.org/docs/LanguageExtensions.html#vectors-and-extended-vectors?

Perhaps it can be a general feature to prevent coercion? But then other targets 
have similar logic too... should it then be more generalized?

> Such behavior is completely valid for x86, but the backend vectorizer
> cannot work with scalars instead of vectors.
>
> With this patch, argument and result types will be leaved unchanged in
> the CodeGen.
>
> New option fopencl-force-vector-abi is also added to force-disables
> vector to scalar coercion when provided.

Do you have any public examples to look at? The coercion is part of the ABI so 
it should ideally be respected by other tools.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142948

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

[PATCH] D141620: clang/OpenCL: Apply default attributes to enqueued blocks

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/test/CodeGenOpenCL/cl20-device-side-enqueue-attributes.cl:26
+// SPIR32-SAME: (ptr addrspace(1) align 4 [[A:%.*]], ptr addrspace(1) align 4 
[[B:%.*]], i32 [[I:%.*]]) #[[ATTR0:[0-9]+]] !kernel_arg_addr_space !2 
!kernel_arg_access_qual !3 !kernel_arg_type !4 !kernel_arg_base_type !4 
!kernel_arg_type_qual !5 {
+// SPIR32-NEXT:  entry:
+// SPIR32-NEXT:[[A_ADDR:%.*]] = alloca ptr addrspace(1), align 4

`ATTR0` seems to be set but never used.

Do we need to be checking the output of the whole function body?

I imagine checking the function definition and attributes should be enough for 
this patch?



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

https://reviews.llvm.org/D141620

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

[PATCH] D141700: AMDGPU: Move enqueued block handling into clang

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:12440
+/// AMDHSAKernelDescriptor.h)
+static llvm::StructType *getKernelDescriptorType(llvm::LLVMContext &C) {
+  llvm::Type *Int8 = llvm::IntegerType::getInt8Ty(C);

Is this AMDGPU target specific? If so perhaps it's better to reflect this in 
the name.


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

https://reviews.llvm.org/D141700

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

[PATCH] D141297: [OpenCL] Allow undefining header-only features

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D141297#4047148 , @svenvh wrote:

> In D141297#4043122 , @Anastasia 
> wrote:
>
>> Btw I wonder if in the future we could add some error or warning in case 
>> someone uses the same approach for frontend specific features, i.e.
>>
>>   #ifdef __undef___opencl_c_generic_address_space
>>   #error "Feature __opencl_c_generic_address_space can only be disabled via 
>> -cl-ext flag"
>>   #endif
>
> Interesting idea, but I'm a bit hesitant of doing so:
> It increases the size of `opencl-c-base.h` so it will take longer to parse, 
> which will affect every OpenCL compilation. Luckily we can avoid that cost if 
> we keep the `__undef_` mechanism an internal solution, which it will be once 
> we let `-cl-ext=-feature` generate `__undef_` macros for extensions that are 
> not in `OpenCLExtensions.def`. So longer term users will never have to pass 
> `__undef_` macros.

Ok that would be better indeed. Btw I hope we won't have that many feature 
macros that it would be too long to parse those. :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141297

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

[PATCH] D141449: clang/OpenCL: Fix not setting convergent on block invoke kernels

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:12426
 
   // FIXME: Apply default attributes
   F->addFnAttr(llvm::Attribute::NoUnwind);

If `ConstructDefaultFnAttrList` was used it would get convergent correctly... 
Should we be using the helper function here instead?


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

https://reviews.llvm.org/D141449

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

[PATCH] D141297: [OpenCL] Allow undefining header-only features

[Anastasia Stulova via Phabricator via cfe-commits]

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

LGTM, thanks!

Btw I wonder if in the future we could add some error or warning in case 
someone uses the same approach for frontend specific features, i.e.

  #ifdef __undef___opencl_c_generic_address_space
  #error "Feature __opencl_c_generic_address_space can only be disabled via 
-cl-ext flag"
  #endif


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141297

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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2260
 
-return CGF.CGM.getNullPointer(cast(ConvertType(DestTy)),
-  DestTy);
+// The type may be a target extension type instead of a pointer type
+// (e.g., OpenCL types mapped for SPIR-V). In the former case, emit a

Ok, yet this looks strange to me... do you have an example that hits this code?

At some point we added `CK_ZeroToOCLOpaqueType` so I wonder if we should be 
using this instead of `CK_NullToPointer` here i.e. ideally clang should not 
assume too early how type are mapped into target specific representation.




Comment at: clang/lib/CodeGen/CGOpenCLRuntime.cpp:200
+
+  // Choose the dimension of the image--this corresponds to the Dim parameter,
+  // so (e.g.) a 2D image has value 1, not 2.

Any reason for this? Can we create a `constexpr` map or enum type that would 
contain those numbers instead of using hard coded ones scattered around?



Comment at: clang/lib/CodeGen/CGOpenCLRuntime.cpp:226
+
+llvm::Type *CGSpirVOpenCLRuntime::convertOpenCLSpecificType(const Type *T) {
+  assert(T->isOpenCLSpecificType() && "Not an OpenCL specific type!");

This looks good in general but we keep CodeGen hierarchy target agnostic so the 
way to add specifics of an individual target is to extend `TargetCodeGenInfo` 
adding a new target hook member function for example it could be 
`getOpenCLSpecificType` or something like that. Then you can add SPIR-V 
specific logic into either `CommonSPIRTargetCodeGenInfo` or 
`SPIRVTargetCodeGenInfo` subclass. I would recommend to look at 
 `createEnqueuedBlockKernel` for inspiration 
https://clang.llvm.org/doxygen/CodeGen_2TargetInfo_8cpp_source.html#l12407. 





Comment at: clang/lib/CodeGen/CodeGenModule.cpp:239
+  // extension types.
+  if (getTriple().isSPIRV() || getTriple().isSPIR())
+OpenCLRuntime.reset(new CGSpirVOpenCLRuntime(*this));

Do we want to change old SPIR representation or keep it as is? It seems that 
SPIR spec defined them as LLVM's opaque pointer types... but I appreciate that 
for maintenance purposes it's easier to keep those in sync.



Comment at: llvm/docs/SPIRVUsage.rst:103
+
+All integer arguments take the same value as they do in the SPIR-V type name.
+For example, the OpenCL type ``image2d_depth_ro_t`` would be represented in

svenvh wrote:
> 
Any reference to the document/section explaining these instructions would be 
useful here.


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.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

Can you link relevant LLVM reviews or documentation and perhaps add some more 
details into the description?

It's  a bit suboptimal though to emit this form of types as SPIR-V specific.  
Ideally Clang should worry as less as possible about target specifics. So it 
would be better if we emit OpenCL opaque types as opaque in LLVM IR and then 
let backends expand those to whatever they need them to be.  There isn't 
technically anything SPIR-V specific in ``target("spirv.Image", void, 1, 1, 0, 
0, 0, 0, 0)`` as this form  is just a better representation of the type with 
more HL information preserved that can either be used or discarded by the 
optimiser/backend.

My worry is that with the current approach more complexity is being added into 
the common parts of the compiler. At least it might be better to hide this 
alternative generation paths into the target implementation itself just like 
for example we ask target hooks to provide mapping to address spaces. Then we 
could just have a default path from which targets can derive from...




Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2263
+llvm::Type *LlvmTy = ConvertType(DestTy);
+if (auto *PointerTy = dyn_cast(LlvmTy))
+  return CGF.CGM.getNullPointer(PointerTy, DestTy);

This case at least deserve a comment with explanation. Otherwise I am confused 
why we end up with a non-pointer type as destination in NullToPointer cast?



Comment at: clang/lib/CodeGen/CGOpenCLRuntime.cpp:80
+
+  if (useSPIRVTargetExtType(CGM)) {
+switch (cast(T)->getKind()) {

Perhaps it's best to split into separate functions and reflect in naming what 
they correspond to.



Comment at: clang/lib/CodeGen/CGOpenCLRuntime.cpp:100
+#define INTEL_SUBGROUP_AVC_TYPE(Name, Id)  
\
+case BuiltinType::OCLIntelSubgroupAVC##Id: 
\
+  return llvm::TargetExtType::get(Ctx, "spirv.Avc" #Id "INTEL");

Why does this need special handling?


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] D137652: Remove mandatory define of optional features macros for OpenCL C 3.0

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D137652#3957519 , @svenvh wrote:

>> @svenvh I remember that we have also discussed the addition of a vendor 
>> specific header where such feature/extension macro definition can be added 
>> to avoid the macro pollution but I feel this is somewhat orthogonal i.e. the 
>> fine grained control of macro defines is still needed?
>
> Unfortunately I don't remember the details of that discussion, but I agree 
> that it's worth looking into a solution for issue #55674, using e.g. 
> `__undef` macros as you described above.

I assume we could start from something simple i.e. without the need to amend 
`-cl-ext`. So let's say we could add the following guards in the header:

  #ifndef __undef_feature1
  #define feature1 1
  #endif

Then we can pass `-D__undef_feature1` instead of  `-cl-std=-feature1` and 
`-Dfeature1=1` instead of `-cl-std=+feature1`.

Then later if needed we could extend `-cl-ext` to set those  `__undef_` instead.

We could also add a macro that corresponds to `-cl-ext=-all` and etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137652

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

[PATCH] D137652: Remove mandatory define of optional features macros for OpenCL C 3.0

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D137652#3957233 , @FMarno wrote:

> @Anastasia I feel like I am following the guidance you quoted here. The 
> defines I've deleted in `opencl-c-base.h` are blocking the possibility of 
> `-cl-ext` working and would also get in the way of undefine preprocessor 
> symbols working. 
> If there is a problem with the additions to `OpenCLExtensions.def` then we'll 
> need to rework how `InitializeOpenCLFeatureTestMacros` in 
> `initPreprocessor.cpp` works.

Let me clarify this topic a bit better -  the guidelines are about whether a 
feature/extension needs to be added into the frontend based on whether the 
frontend needs to know about the feature and do something different e.g. for 
example it can parse the code differently and etc. However you are adding the 
features just in order for them to appear in the predefined macros. The 
approach you choose is a workaround because ideally it should only be used when 
the fronted needs to query the feature settings to do something useful based on 
those but you don't ever need them in the frontend as far as I understand as 
you only need to add the macros. Since clang includes default header files it 
is very straightforward to add the macros in the headers by simply defining 
them there. No frontend changes needed for that. Now the only problem is that 
the headers don't provide defining the macros conditionally yet because nobody 
has added this functionality yet.

One idea that we discussed in the issue I quoted is to extend `-cl-ext` such 
that it would be able to define or undefine those macros, or alternatively 
feature/extensions macro defines in the headers could be guarded by the 
corresponding `__undef_` macros that can be added from 
the command line during source compilation by either user or a compiler driver. 
It is also possible to use a hybrid approach i.e. `-cl-ext` would add 
`__undef_` macros that guard the feature/extension 
macros in the header. This mechanism of adding header only feature/extension 
macros is not fully working yet as it is currently only sets macros per target. 
I appreciate that a more fine grained control is desirable in some situation as 
in your case. However this doesn't mean that we should continue adding more 
thing into the frontend where it is  not needed. We should work toward more 
scalable solution instead that provides such control in the internal headers. 
Does this make the direction clear? I would recommend to check the discussions 
in the issue (#55674) and linked review as it provides more background.

@svenvh I remember that we have also discussed the addition of a vendor 
specific header where such feature/extension macro definition can be added to 
avoid the macro pollution but I feel this is somewhat orthogonal i.e. the fine 
grained control of macro defines is still needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137652

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

[PATCH] D137652: Remove mandatory define of optional features macros for OpenCL C 3.0

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

Are these features affecting the frontend functionality in any way? If not we 
should implement those in the headers and if headers are not flexible enough to 
condition out the addition of the new macros we should extend them (potentially 
by extending behaviour of `-cl-ext` or introducing `undef` macros, see 
https://github.com/llvm/llvm-project/issues/55674). Please review the 
guidelines here: 
https://clang.llvm.org/docs/OpenCLSupport.html#opencl-extensions-and-features. 
The most relevant part here is:

> If an extension adds functionality that does not modify standard language 
> parsing it should not require modifying anything other than header files and 
> OpenCLBuiltins.td detailed in OpenCL builtins. Most commonly such extensions 
> add functionality via libraries (by adding non-native types or functions) 
> parsed regularly. Similar to other languages this is the most common way to 
> add new functionality.

Overall, the idea is to avoiding modifying frontend changes if we can implement 
features via the headers instead. This is needed to address the scalability 
problems.




Comment at: clang/include/clang/Basic/OpenCLExtensions.def:123
+OPENCL_OPTIONALCOREFEATURE(__opencl_c_work_group_collective_functions, false, 
300, OCL_C_30)
+OPENCL_OPTIONALCOREFEATURE(__opencl_c_int64, false, 300, OCL_C_30)
 

svenvh wrote:
> I am wondering why those features weren't added together with the other 
> OpenCL 3.0 features; there wasn't any discussion around that in D95776.  
> Perhaps it's because these don't affect the compiler behaviour directly? (but 
> then neither does e.g. `__opencl_c_atomic_order_acq_rel`)  Wondering if 
> @Anastasia has any insights here.
Correct, I think Anton wanted to implement some diagnostics affecting 
`__opencl_c_atomic_order_acq_rel`, but it hasn't happened and I am not 
convinced it is doable. So potentially we can remove those from here in the 
future and migrate into the internal headers too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137652

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

[PATCH] D134445: [PR57881][OpenCL] Fix incorrect diagnostics with templated types in kernel arguments

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D134445#3920448 , @rjmccall wrote:

> In D134445#3920257 , @Anastasia 
> wrote:
>
>> In D134445#3920188 , @rjmccall 
>> wrote:
>>
>>> You really can't ask whether a class template pattern is standard layout; 
>>> it's not meaningful.
>>
>> Well the templates have to be instantiated with concrete types as kernels 
>> can't be called from the host code. But if the concrete type is a reference 
>> or pointer there is no full instantiation apparently as AST dumps in the 
>> review description show.
>
> My objection here is to the code that does `CXXRec = 
> CXXRec->getTemplateInstantiationPattern()`.  That is the unsubstituted 
> template pattern.  You then ask if it has standard layout.  I do not think 
> this is the right thing to do.

Yes this only works if the type doesn't depend on the template parameter like 
in the reported test case. But it is not a universal solution.

>>> How are pointers and references passed to kernels?  Does the pointee get 
>>> copied or something?  If so, you may have a requirement that pointee types 
>>> be complete, in which case the only problem is probably that you're doing 
>>> this check too soon, or doing them on declarations rather than on 
>>> definition/use.
>>
>> Ok, the kernel function actually already contains the instantiated templates 
>> since they can't be templated themselves.
>
> Yes, I understand that.
>
>> My understanding is that references and pointers to templated types are not 
>> required to be fully instantiated as they are not ODR-used?
>
> It's not the ODR, but yes, in standard C++, declaring a parameter of pointer 
> type does not require the pointee type to be complete, and if the pointee 
> type is a template type, the compiler is not permitted to immediately try to 
> instantiate it.  Of course, OpenCL `kernel` declarations are not standard C++ 
> and can use different rules, and I think the fact that OpenCL has this 
> semantic restriction on pointer arguments to kernels is pretty good license 
> to do so.  There are some places in C++ (e.g. initialization) that do 
> instantiate templates if a definition is available, and you can request this 
> by calling `Sema::isCompleteType` instead of `hasDefinition()`.

Ok, thanks! I will look into this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134445

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

[PATCH] D134445: [PR57881][OpenCL] Fix incorrect diagnostics with templated types in kernel arguments

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D134445#3920188 , @rjmccall wrote:

> You really can't ask whether a class template pattern is standard layout; 
> it's not meaningful.

Well the templates have to be instantiated with concrete types as kernels can't 
be called from the host code. But if the concrete type is a reference or 
pointer there is no full instantiation apparently as AST dumps in the review 
description show.

> How are pointers and references passed to kernels?  Does the pointee get 
> copied or something?  If so, you may have a requirement that pointee types be 
> complete, in which case the only problem is probably that you're doing this 
> check too soon, or doing them on declarations rather than on definition/use.

Ok, the kernel function actually already contains the instantiated templates 
since they can't be templated themselves. My understanding is that references 
and pointers to templated types are not required to be fully instantiated as 
they are not ODR-used? At least this is what I deduce from my AST dump 
examples. I.e. it work fine if the templates are not references/pointers and it 
fails to create a complete instantiation otherwise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134445

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

[PATCH] D134445: [PR57881][OpenCL] Fix incorrect diagnostics with templated types in kernel arguments

[Anastasia Stulova 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 rG790cbaafc9e2: [OpenCL] Fix diagnostics with templates in 
kernel args. (authored by Anastasia).
Herald added a subscriber: ldrumm.
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D134445?vs=470458&id=474577#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134445

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaOpenCLCXX/invalid-kernel.clcpp


Index: clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
===
--- clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
+++ clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
@@ -93,3 +93,24 @@
 #ifndef UNSAFEKERNELPARAMETER
 //expected-error@-2{{'__global Trivial &__private' cannot be used as the type 
of a kernel parameter}}
 #endif
+
+// Nested types and templates
+struct Outer {
+  struct Inner{
+int i;
+  };
+};
+template
+struct OuterTempl {
+  struct Inner{
+int i;
+  };
+};
+// FIXME: (PR58590) Use of template parameter dependent types doesn't
+// work yet due to lazy instantiation of reference types.
+//template
+//struct Templ {
+//T i;
+//};
+
+extern kernel void nested(constant Outer::Inner& r1, constant 
OuterTempl::Inner& r2/*, constant Templ& r3*/);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9234,10 +9234,23 @@
 // reference if an implementation supports them in kernel parameters.
 if (S.getLangOpts().OpenCLCPlusPlus &&
 !S.getOpenCLOptions().isAvailableOption(
-"__cl_clang_non_portable_kernel_param_types", S.getLangOpts()) &&
-!PointeeType->isAtomicType() && !PointeeType->isVoidType() &&
-!PointeeType->isStandardLayoutType())
+"__cl_clang_non_portable_kernel_param_types", S.getLangOpts())) {
+ auto CXXRec = PointeeType.getCanonicalType()->getAsCXXRecordDecl();
+ bool IsStandardLayoutType = true;
+ if (CXXRec) {
+   // If template type is not ODR-used its definition is only available
+   // in the template definition not its instantiation.
+   // FIXME: This logic doesn't work for types that depend on template
+   // parameter (PR58590).
+   if (!CXXRec->hasDefinition())
+ CXXRec = CXXRec->getTemplateInstantiationPattern();
+   if (!CXXRec || !CXXRec->hasDefinition() || !CXXRec->isStandardLayout())
+ IsStandardLayoutType = false;
+ }
+ if (!PointeeType->isAtomicType() && !PointeeType->isVoidType() &&
+!IsStandardLayoutType)
   return InvalidKernelParam;
+}
 
 return PtrKernelParam;
   }


Index: clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
===
--- clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
+++ clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
@@ -93,3 +93,24 @@
 #ifndef UNSAFEKERNELPARAMETER
 //expected-error@-2{{'__global Trivial &__private' cannot be used as the type of a kernel parameter}}
 #endif
+
+// Nested types and templates
+struct Outer {
+  struct Inner{
+int i;
+  };
+};
+template
+struct OuterTempl {
+  struct Inner{
+int i;
+  };
+};
+// FIXME: (PR58590) Use of template parameter dependent types doesn't
+// work yet due to lazy instantiation of reference types.
+//template
+//struct Templ {
+//T i;
+//};
+
+extern kernel void nested(constant Outer::Inner& r1, constant OuterTempl::Inner& r2/*, constant Templ& r3*/);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9234,10 +9234,23 @@
 // reference if an implementation supports them in kernel parameters.
 if (S.getLangOpts().OpenCLCPlusPlus &&
 !S.getOpenCLOptions().isAvailableOption(
-"__cl_clang_non_portable_kernel_param_types", S.getLangOpts()) &&
-!PointeeType->isAtomicType() && !PointeeType->isVoidType() &&
-!PointeeType->isStandardLayoutType())
+"__cl_clang_non_portable_kernel_param_types", S.getLangOpts())) {
+ auto CXXRec = PointeeType.getCanonicalType()->getAsCXXRecordDecl();
+ bool IsStandardLayoutType = true;
+ if (CXXRec) {
+   // If template type is not ODR-used its definition is only available
+   // in the template definition not its instantiation.
+   // FIXME: This logic doesn't work for types that depend on template
+   // parameter (PR58590).
+   if (!CXXRec->hasDefinition())
+ CXXRec = CXXRec->getTemplateInstantiationPattern();
+   if (!CXXRec || !CXXRec->hasDefinition() || !CXXRec->isStandardLayout())
+ IsStandardLayoutType = false;
+ }
+ if (!PointeeType->isAtomicType() && !Pointe

[PATCH] D134445: [PR57881][OpenCL] Fix incorrect diagnostics with templated types in kernel arguments

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 470458.
Anastasia added a comment.

Addressed review comments


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

https://reviews.llvm.org/D134445

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaOpenCLCXX/invalid-kernel.clcpp


Index: clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
===
--- clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
+++ clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
@@ -93,3 +93,24 @@
 #ifndef UNSAFEKERNELPARAMETER
 //expected-error@-2{{'__global Trivial &__private' cannot be used as the type 
of a kernel parameter}}
 #endif
+
+// Nested types and templates
+struct Outer {
+  struct Inner{
+int i;
+  };
+};
+template
+struct OuterTempl {
+  struct Inner{
+int i;
+  };
+};
+// FIXME: (PR58590) Use of template parameter dependent types doesn't
+// work yet due to lazy instantiation of reference types.
+//template
+//struct Templ {
+//T i;
+//};
+
+extern kernel void nested(constant Outer::Inner& r1, constant 
OuterTempl::Inner& r2/*, constant Templ& r3*/);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9245,10 +9245,23 @@
 // reference if an implementation supports them in kernel parameters.
 if (S.getLangOpts().OpenCLCPlusPlus &&
 !S.getOpenCLOptions().isAvailableOption(
-"__cl_clang_non_portable_kernel_param_types", S.getLangOpts()) &&
-!PointeeType->isAtomicType() && !PointeeType->isVoidType() &&
-!PointeeType->isStandardLayoutType())
+"__cl_clang_non_portable_kernel_param_types", S.getLangOpts())) {
+ auto CXXRec = PointeeType.getCanonicalType()->getAsCXXRecordDecl();
+ bool IsStandardLayoutType = true;
+ if (CXXRec) {
+   // If template type is non-ODR used its definition is only available
+   // in the template definition not its instantiation.
+   // FIXME: This logic doesn't work for types that depend on template
+   // parameter (PR58590).
+   if (!CXXRec->hasDefinition())
+ CXXRec = CXXRec->getTemplateInstantiationPattern();
+   if (!CXXRec || !CXXRec->hasDefinition() || !CXXRec->isStandardLayout())
+ IsStandardLayoutType = false;
+ }
+ if (!PointeeType->isAtomicType() && !PointeeType->isVoidType() &&
+!IsStandardLayoutType)
   return InvalidKernelParam;
+}
 
 return PtrKernelParam;
   }


Index: clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
===
--- clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
+++ clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
@@ -93,3 +93,24 @@
 #ifndef UNSAFEKERNELPARAMETER
 //expected-error@-2{{'__global Trivial &__private' cannot be used as the type of a kernel parameter}}
 #endif
+
+// Nested types and templates
+struct Outer {
+  struct Inner{
+int i;
+  };
+};
+template
+struct OuterTempl {
+  struct Inner{
+int i;
+  };
+};
+// FIXME: (PR58590) Use of template parameter dependent types doesn't
+// work yet due to lazy instantiation of reference types.
+//template
+//struct Templ {
+//T i;
+//};
+
+extern kernel void nested(constant Outer::Inner& r1, constant OuterTempl::Inner& r2/*, constant Templ& r3*/);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9245,10 +9245,23 @@
 // reference if an implementation supports them in kernel parameters.
 if (S.getLangOpts().OpenCLCPlusPlus &&
 !S.getOpenCLOptions().isAvailableOption(
-"__cl_clang_non_portable_kernel_param_types", S.getLangOpts()) &&
-!PointeeType->isAtomicType() && !PointeeType->isVoidType() &&
-!PointeeType->isStandardLayoutType())
+"__cl_clang_non_portable_kernel_param_types", S.getLangOpts())) {
+ auto CXXRec = PointeeType.getCanonicalType()->getAsCXXRecordDecl();
+ bool IsStandardLayoutType = true;
+ if (CXXRec) {
+   // If template type is non-ODR used its definition is only available
+   // in the template definition not its instantiation.
+   // FIXME: This logic doesn't work for types that depend on template
+   // parameter (PR58590).
+   if (!CXXRec->hasDefinition())
+ CXXRec = CXXRec->getTemplateInstantiationPattern();
+   if (!CXXRec || !CXXRec->hasDefinition() || !CXXRec->isStandardLayout())
+ IsStandardLayoutType = false;
+ }
+ if (!PointeeType->isAtomicType() && !PointeeType->isVoidType() &&
+!IsStandardLayoutType)
   return InvalidKernelParam;
+}
 
 return PtrKernelParam;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134445: [PR57881][OpenCL] Fix incorrect diagnostics with templated types in kernel arguments

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/test/SemaOpenCLCXX/invalid-kernel.clcpp:110
+
+// FIXME: Use of templates doesn't work due to lazy instantiation of reference 
types. 
+//template

I think here should be something like - `the use of templated types doesn't 
work...`



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

https://reviews.llvm.org/D134445

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

[PATCH] D134445: [PR57881][OpenCL] Fix incorrect diagnostics with templated types in kernel arguments

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added reviewers: svenvh, olestrohm, rjmccall.
Herald added subscribers: Naghasan, ebevhan, yaxunl.
Herald added a project: All.
Anastasia requested review of this revision.

It seems relying on `isStandardLayoutType` helpers is fragile when kernel 
arguments are references/pointers to templated types.

Clang uses lazy template instantiation in this case and therefore types are not 
fully instantiated when those are used.

Example:

  template
  struct outer{
  public:
  struct inner{
  int size;
  };
  };
  void foo(outer::inner& p)  
  {
  } 

will have the following AST when p is a reference:

  | `-ClassTemplateSpecializationDecl  line:2:8 struct 
outer definition
  |   |-DefinitionData pass_in_registers empty aggregate standard_layout 
trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor 
can_const_default_init
  |   | |-DefaultConstructor exists trivial constexpr needs_implicit 
defaulted_is_constexpr
  |   | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
  |   | |-MoveConstructor exists simple trivial needs_implicit
  |   | |-CopyAssignment simple trivial has_const_param needs_implicit 
implicit_has_const_param
  |   | |-MoveAssignment exists simple trivial needs_implicit
  |   | `-Destructor simple irrelevant trivial needs_implicit
  |   |-TemplateArgument type 'int'
  |   | `-BuiltinType 'int'
  |   |-CXXRecordDecl  col:8 implicit struct outer
  |   |-AccessSpecDecl  col:1 public
  |   `-CXXRecordDecl  col:12 referenced struct inner

and when it is not a reference

  | `-ClassTemplateSpecializationDecl  line:2:8 struct 
outer definition
  |   |-DefinitionData pass_in_registers empty aggregate standard_layout 
trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor 
can_const_default_init
  |   | |-DefaultConstructor exists trivial constexpr needs_implicit 
defaulted_is_constexpr
  |   | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
  |   | |-MoveConstructor exists simple trivial needs_implicit
  |   | |-CopyAssignment simple trivial has_const_param needs_implicit 
implicit_has_const_param
  |   | |-MoveAssignment exists simple trivial needs_implicit
  |   | `-Destructor simple irrelevant trivial needs_implicit
  |   |-TemplateArgument type 'int'
  |   | `-BuiltinType 'int'
  |   |-CXXRecordDecl  col:8 implicit struct outer
  |   |-AccessSpecDecl  col:1 public
  |   `-CXXRecordDecl  line:4:12 referenced struct inner 
definition
  | |-DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
  | | |-DefaultConstructor exists trivial needs_implicit
  | | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
  | | |-MoveConstructor exists simple trivial needs_implicit
  | | |-CopyAssignment simple trivial has_const_param needs_implicit 
implicit_has_const_param
  | | |-MoveAssignment exists simple trivial needs_implicit
  | | `-Destructor simple irrelevant trivial needs_implicit
  | |-CXXRecordDecl  col:12 implicit struct inner
  | `-FieldDecl  col:13 size 'int'

In the first case for reference type `inner` is incomplete while if we use a 
non-reference type the definition of template specialization is complete.

I have attempted to workaround this issue for the reported test cases, however 
it still doesn't quite work when the type is created from the template 
parameter (see FIXME test case in the patch). I presume we want to allow this? 
If so we might need to disable lazy template instantiation in this case. My 
guess is the only issue this this is that we will have performance penalty for 
the code of this format:

  template struct Dummy {
  T i;
  };
  
  extern kernel void foo(Dummy& i);

which doesn't technically need the full instantiation.

Also I have discovered that in OpenCL C we have allowed passing kernel 
parameters with pointers to forward declared structs, but in C++ for OpenCL 
that no longer compiles. Not sure whether we should keep this restriction but 
if not it might interfere with the idea of always instantiating the structs 
definitions fully and even providing the safe diagnostics. So another approach 
we could take is to change this diagnostics into warnings and then if we can't 
fully detect the type provide the messaging that we can't detect whether the 
type is safe...

Another aspect to consider is that we have an extension that allows disabling 
all of this safe checks completely: 
https://clang.llvm.org/docs/LanguageExtensions.html#cl-clang-non-portable-kernel-param-types

While this matter might need more thoughts and investigations I wonder whether 
it makes sense to commit this fix for the time being since it's fixing the 
reported test case at least?


https://reviews.llvm.org/D134445

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaOpenCLCXX/invalid-

[PATCH] D132056: [HLSL] Restrict to supported targets

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia added a comment.

This makes sense. I believe the same applies to some other languages i.e. 
OpenCL, as the way clang evolved currently not all targets are compatible with 
all languages. In the future we might want to generalize this diagnostics to 
use in other cases too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132056

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

[PATCH] D130768: [OpenCL][SPIR-V] Add test for extern functions with a pointer

[Anastasia Stulova via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6b1a04529c8f: [OpenCL][SPIR-V] Test extern functions with a 
pointer arg. (authored by Anastasia).
Herald added a subscriber: ldrumm.
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D130768?vs=448602&id=457208#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130768

Files:
  clang/test/CodeGenOpenCL/opaque-ptr-spirv.cl


Index: clang/test/CodeGenOpenCL/opaque-ptr-spirv.cl
===
--- /dev/null
+++ clang/test/CodeGenOpenCL/opaque-ptr-spirv.cl
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -no-opaque-pointers -emit-llvm -o - -triple spirv64 %s | 
FileCheck %s
+
+// Check that we have a way to recover pointer
+// types for extern function prototypes (see PR56660).
+extern void foo(global int * ptr);
+kernel void k(global int * ptr) {
+  foo(ptr);
+}
+//CHECK: define spir_kernel void @k(i32 {{.*}}*
+//CHECK: declare spir_func void @foo(i32 {{.*}}*


Index: clang/test/CodeGenOpenCL/opaque-ptr-spirv.cl
===
--- /dev/null
+++ clang/test/CodeGenOpenCL/opaque-ptr-spirv.cl
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -no-opaque-pointers -emit-llvm -o - -triple spirv64 %s | FileCheck %s
+
+// Check that we have a way to recover pointer
+// types for extern function prototypes (see PR56660).
+extern void foo(global int * ptr);
+kernel void k(global int * ptr) {
+  foo(ptr);
+}
+//CHECK: define spir_kernel void @k(i32 {{.*}}*
+//CHECK: declare spir_func void @foo(i32 {{.*}}*
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132473: [Docs][OpenCL][SPIR-V] Release 15 notes for Clang

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia closed this revision.
Anastasia added a comment.

Committed in 
https://github.com/llvm/llvm-project/commit/5e1b36ced538cfc80f2400b27e346d2175b04092.


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

https://reviews.llvm.org/D132473

___
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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.
Herald added a subscriber: MaskRay.
Herald added a project: All.



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

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?



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] D132473: [Docs][OpenCL][SPIR-V] Release 15 notes for Clang

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:676
+  LLVM backend when generating SPIR-V binary.
+
 Floating Point Support in Clang

svenvh wrote:
> Should we say anything about opaque pointers?  Something like:
> 
> ```
>  - Although LLVM has switched to opaque pointers with this release, SPIR-V 
> generation still relies on typed pointers in this release.
> ```
Sure, good point! I have added an item to clarify this. Does it look ok to you?


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

https://reviews.llvm.org/D132473

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

[PATCH] D132473: [Docs][OpenCL][SPIR-V] Release 15 notes for Clang

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 455139.
Anastasia added a comment.

- Added clarification about the opaque pointers in SPIR-V.


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

https://reviews.llvm.org/D132473

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -566,10 +566,18 @@
 Objective-C Language Changes in Clang
 -
 
-OpenCL C Language Changes in Clang
---
-
-...
+OpenCL Kernel Language Changes in Clang
+---
+
+- Improved/fixed misc issues in the builtin function support and diagnostics.
+- Improved diagnostics for unknown extension pragma, subgroup functions and
+  implicit function prototype.
+- Added ``-cl-ext`` flag to the Clang driver to toggle extensions/features
+  compiled for.
+- Added ``cl_khr_subgroup_rotate`` extension.
+- Removed some ``printf`` and ``hostcall`` related diagnostics when compiling
+  for AMDGPU.
+- Fixed alignment of pointer types in kernel arguments.
 
 ABI Changes in Clang
 
@@ -660,6 +668,14 @@
   Operations found in the :ref:`Clang Language Extensions `
   document.
 
+SPIR-V Support in Clang
+---
+
+- Added flag ``-fintegrated-objemitter`` to enable use of experimental
+  integrated LLVM backend when generating SPIR-V binary.
+- The SPIR-V generation continues to produce typed pointers in this release
+  despite the general switch of LLVM to opaque pointers.
+
 Floating Point Support in Clang
 ---
 


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -566,10 +566,18 @@
 Objective-C Language Changes in Clang
 -
 
-OpenCL C Language Changes in Clang
---
-
-...
+OpenCL Kernel Language Changes in Clang
+---
+
+- Improved/fixed misc issues in the builtin function support and diagnostics.
+- Improved diagnostics for unknown extension pragma, subgroup functions and
+  implicit function prototype.
+- Added ``-cl-ext`` flag to the Clang driver to toggle extensions/features
+  compiled for.
+- Added ``cl_khr_subgroup_rotate`` extension.
+- Removed some ``printf`` and ``hostcall`` related diagnostics when compiling
+  for AMDGPU.
+- Fixed alignment of pointer types in kernel arguments.
 
 ABI Changes in Clang
 
@@ -660,6 +668,14 @@
   Operations found in the :ref:`Clang Language Extensions `
   document.
 
+SPIR-V Support in Clang
+---
+
+- Added flag ``-fintegrated-objemitter`` to enable use of experimental
+  integrated LLVM backend when generating SPIR-V binary.
+- The SPIR-V generation continues to produce typed pointers in this release
+  despite the general switch of LLVM to opaque pointers.
+
 Floating Point Support in Clang
 ---
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132473: [Docs][OpenCL][SPIR-V] Release 15 notes for Clang

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 454838.
Anastasia added a comment.

Minor formatting changes


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

https://reviews.llvm.org/D132473

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -566,10 +566,18 @@
 Objective-C Language Changes in Clang
 -
 
-OpenCL C Language Changes in Clang
---
-
-...
+OpenCL Kernel Language Changes in Clang
+---
+
+- Improved/fixed misc issues in the builtin function support and diagnostics.
+- Improved diagnostics for unknown extension pragma, subgroup functions and
+  implicit function prototype.
+- Added ``-cl-ext`` flag to the Clang driver to toggle extensions/features
+  compiled for.
+- Added ``cl_khr_subgroup_rotate`` extension.
+- Removed some ``printf`` and ``hostcall`` related diagnostics when compiling
+  for AMDGPU.
+- Fixed alignment of pointer types in kernel arguments.
 
 ABI Changes in Clang
 
@@ -660,6 +668,12 @@
   Operations found in the :ref:`Clang Language Extensions `
   document.
 
+SPIR-V Support in Clang
+---
+
+- Added flag ``-fintegrated-objemitter`` to enable use of experimental 
integrated
+  LLVM backend when generating SPIR-V binary.
+
 Floating Point Support in Clang
 ---
 


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -566,10 +566,18 @@
 Objective-C Language Changes in Clang
 -
 
-OpenCL C Language Changes in Clang
---
-
-...
+OpenCL Kernel Language Changes in Clang
+---
+
+- Improved/fixed misc issues in the builtin function support and diagnostics.
+- Improved diagnostics for unknown extension pragma, subgroup functions and
+  implicit function prototype.
+- Added ``-cl-ext`` flag to the Clang driver to toggle extensions/features
+  compiled for.
+- Added ``cl_khr_subgroup_rotate`` extension.
+- Removed some ``printf`` and ``hostcall`` related diagnostics when compiling
+  for AMDGPU.
+- Fixed alignment of pointer types in kernel arguments.
 
 ABI Changes in Clang
 
@@ -660,6 +668,12 @@
   Operations found in the :ref:`Clang Language Extensions `
   document.
 
+SPIR-V Support in Clang
+---
+
+- Added flag ``-fintegrated-objemitter`` to enable use of experimental integrated
+  LLVM backend when generating SPIR-V binary.
+
 Floating Point Support in Clang
 ---
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132473: [Docs][OpenCL][SPIR-V] Release 15 notes for Clang

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

@svenvh as you made quite a lot of changes in the headers feel free to expand 
the description if you feel we should document some of those in more detail.


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

https://reviews.llvm.org/D132473

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

[PATCH] D132473: [Docs][OpenCL][SPIR-V] Release 15 notes for Clang

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added a reviewer: svenvh.
Herald added subscribers: ebevhan, yaxunl.
Herald added a project: All.
Anastasia requested review of this revision.

Added list of functional changes to release notes.


https://reviews.llvm.org/D132473

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -566,10 +566,18 @@
 Objective-C Language Changes in Clang
 -
 
-OpenCL C Language Changes in Clang
---
-
-...
+OpenCL Kernel Language Changes in Clang
+---
+
+- Improved/fixed misc issues in the builtin function support and diagnostics.
+- Improved diagnostics for unknown extension pragma, subgroup functions and
+  implicit function prototype.
+- Added `-cl-ext` flag to the Clang driver to toggle extensions/features
+  compiled for.
+- Added `cl_khr_subgroup_rotate` extension.
+- Removed some `printf` and `hostcall` related diagnostics when compiling for
+  AMDGPU.
+- Fixed alignment of pointer types in kernel arguments.
 
 ABI Changes in Clang
 
@@ -660,6 +668,12 @@
   Operations found in the :ref:`Clang Language Extensions `
   document.
 
+SPIR-V Support in Clang
+---
+
+- Added flag `-fintegrated-objemitter` to enable use of experimental integrated
+  LLVM backend when generating SPIR-V binary.
+
 Floating Point Support in Clang
 ---
 


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -566,10 +566,18 @@
 Objective-C Language Changes in Clang
 -
 
-OpenCL C Language Changes in Clang
---
-
-...
+OpenCL Kernel Language Changes in Clang
+---
+
+- Improved/fixed misc issues in the builtin function support and diagnostics.
+- Improved diagnostics for unknown extension pragma, subgroup functions and
+  implicit function prototype.
+- Added `-cl-ext` flag to the Clang driver to toggle extensions/features
+  compiled for.
+- Added `cl_khr_subgroup_rotate` extension.
+- Removed some `printf` and `hostcall` related diagnostics when compiling for
+  AMDGPU.
+- Fixed alignment of pointer types in kernel arguments.
 
 ABI Changes in Clang
 
@@ -660,6 +668,12 @@
   Operations found in the :ref:`Clang Language Extensions `
   document.
 
+SPIR-V Support in Clang
+---
+
+- Added flag `-fintegrated-objemitter` to enable use of experimental integrated
+  LLVM backend when generating SPIR-V binary.
+
 Floating Point Support in Clang
 ---
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130951: [HLSL] CodeGen hlsl resource binding.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/CodeGen/CGHLSLRuntime.h:46
 public:
+  struct ResBinding {
+llvm::Optional Reg;

Does this apply to buffers only? In which case it might be better to either 
nest this into Buffer definition or rename into something like  
`BufferResBinding`. Also adding some documenting comments would help here, even 
if they could just refer to the language documentation.



Comment at: clang/lib/CodeGen/CGHLSLRuntime.h:64
   CodeGenModule &CGM;
   uint32_t ResourceCounters[static_cast(
   hlsl::ResourceClass::NumClasses)] = {0};

This is not part of this change but any reason why it is a protected member and 
not private?



Comment at: clang/lib/CodeGen/CGHLSLRuntime.h:77
 private:
+  void addResourceAnnotation(llvm::GlobalVariable *GV, llvm::StringRef TyName,
+ hlsl::ResourceClass RC, ResBinding &Binding);

Similarly - is this buffer specific? Then you could either rename to 
`addBufferResourceAnnotation` or make this a member of a Buffer instead...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130951

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

[PATCH] D130768: [OpenCL][SPIR-V] Add test for extern functions with a pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added reviewers: svenvh, iliya-diyachkov.
Herald added subscribers: ebevhan, yaxunl.
Herald added a project: All.
Anastasia requested review of this revision.

I would like to add this test case that enhances coverage of opaque pointers 
particularly for the problematic case with `extern` functions for which there 
is no solution found yet.


https://reviews.llvm.org/D130768

Files:
  clang/test/CodeGenOpenCL/opaque-ptr-spirv.cl


Index: clang/test/CodeGenOpenCL/opaque-ptr-spirv.cl
===
--- /dev/null
+++ clang/test/CodeGenOpenCL/opaque-ptr-spirv.cl
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -no-opaque-pointers -emit-llvm -o - -triple spirv64 %s | 
FileCheck %s
+
+// Check that we have a way to recover pointer
+// types for extern function prototypes (see PR56660).
+extern void foo(global int * ptr);
+kernel void k(global int * ptr) {
+  foo(ptr);
+}
+//CHECK: define spir_kernel void @k(i32 {{.*}}*
+//CHECK: declare spir_func void @foo(i32 {{.*}}*


Index: clang/test/CodeGenOpenCL/opaque-ptr-spirv.cl
===
--- /dev/null
+++ clang/test/CodeGenOpenCL/opaque-ptr-spirv.cl
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -no-opaque-pointers -emit-llvm -o - -triple spirv64 %s | FileCheck %s
+
+// Check that we have a way to recover pointer
+// types for extern function prototypes (see PR56660).
+extern void foo(global int * ptr);
+kernel void k(global int * ptr) {
+  foo(ptr);
+}
+//CHECK: define spir_kernel void @k(i32 {{.*}}*
+//CHECK: declare spir_func void @foo(i32 {{.*}}*
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130766: [SPIR-V] Disable opaque pointers in relese 15

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a subscriber: linjamaki.
Anastasia added a comment.

CC to @linjamaki in case there is anything else/different needed for HIP.


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

https://reviews.llvm.org/D130766

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

[PATCH] D130766: [SPIR-V] Disable opaque pointers in relese 15

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added reviewers: svenvh, iliya-diyachkov.
Herald added a subscriber: ebevhan.
Herald added a project: All.
Anastasia requested review of this revision.
Herald added a subscriber: MaskRay.

As discussed on RFC mapping into SPIR-V requires pointer being preserved in 
some cases: 
https://discourse.llvm.org/t/rfc-better-support-for-typed-pointers-in-an-opaque-pointer-world/63339/23?u=anastasiastulova

As the work is still unfinished the best approach is to continue using pointer 
types.

Note that this change is only planned to be committed in release 15 branch.

This fixing PR56660.


https://reviews.llvm.org/D130766

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/spirv-toolchain.cl


Index: clang/test/Driver/spirv-toolchain.cl
===
--- clang/test/Driver/spirv-toolchain.cl
+++ clang/test/Driver/spirv-toolchain.cl
@@ -6,6 +6,7 @@
 // RUN: %clang -### --target=spirv64 -x c -c %s 2>&1 | FileCheck 
--check-prefix=SPV64 %s
 
 // SPV64: "-cc1" "-triple" "spirv64"
+// SPV64-SAME: "-no-opaque-pointers"
 // SPV64-SAME: "-o" [[BC:".*bc"]]
 // SPV64: {{llvm-spirv.*"}} [[BC]] "-o" {{".*o"}}
 
@@ -16,6 +17,7 @@
 // RUN: %clang -### --target=spirv32 -x c -c %s 2>&1 | FileCheck 
--check-prefix=SPV32 %s
 
 // SPV32: "-cc1" "-triple" "spirv32"
+// SPV32-SAME: "-no-opaque-pointers"
 // SPV32-SAME: "-o" [[BC:".*bc"]]
 // SPV32: {{llvm-spirv.*"}} [[BC]] "-o" {{".*o"}}
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4630,8 +4630,13 @@
   TC.addClangWarningOptions(CmdArgs);
 
   // FIXME: Subclass ToolChain for SPIR and move this to 
addClangWarningOptions.
-  if (Triple.isSPIR() || Triple.isSPIRV())
+  if (Triple.isSPIR() || Triple.isSPIRV()) {
 CmdArgs.push_back("-Wspir-compat");
+// SPIR-V support still needs pointer types in some cases as recovering
+// type from pointer uses is not always possible e.g. for extern functions
+// (see PR56660).
+CmdArgs.push_back("-no-opaque-pointers");
+  }
 
   // Select the appropriate action.
   RewriteKind rewriteKind = RK_None;


Index: clang/test/Driver/spirv-toolchain.cl
===
--- clang/test/Driver/spirv-toolchain.cl
+++ clang/test/Driver/spirv-toolchain.cl
@@ -6,6 +6,7 @@
 // RUN: %clang -### --target=spirv64 -x c -c %s 2>&1 | FileCheck --check-prefix=SPV64 %s
 
 // SPV64: "-cc1" "-triple" "spirv64"
+// SPV64-SAME: "-no-opaque-pointers"
 // SPV64-SAME: "-o" [[BC:".*bc"]]
 // SPV64: {{llvm-spirv.*"}} [[BC]] "-o" {{".*o"}}
 
@@ -16,6 +17,7 @@
 // RUN: %clang -### --target=spirv32 -x c -c %s 2>&1 | FileCheck --check-prefix=SPV32 %s
 
 // SPV32: "-cc1" "-triple" "spirv32"
+// SPV32-SAME: "-no-opaque-pointers"
 // SPV32-SAME: "-o" [[BC:".*bc"]]
 // SPV32: {{llvm-spirv.*"}} [[BC]] "-o" {{".*o"}}
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4630,8 +4630,13 @@
   TC.addClangWarningOptions(CmdArgs);
 
   // FIXME: Subclass ToolChain for SPIR and move this to addClangWarningOptions.
-  if (Triple.isSPIR() || Triple.isSPIRV())
+  if (Triple.isSPIR() || Triple.isSPIRV()) {
 CmdArgs.push_back("-Wspir-compat");
+// SPIR-V support still needs pointer types in some cases as recovering
+// type from pointer uses is not always possible e.g. for extern functions
+// (see PR56660).
+CmdArgs.push_back("-no-opaque-pointers");
+  }
 
   // Select the appropriate action.
   RewriteKind rewriteKind = RK_None;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130131: [HLSL] CodeGen hlsl cbuffer/tbuffer.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:61
+
+GlobalVariable *replaceCBuffer(CGHLSLRuntime::CBuffer &CB) {
+  const unsigned CBufferAddressSpace = 4;

python3kgae wrote:
> Anastasia wrote:
> > I don't think I understand the intent of this function along with 
> > `CGHLSLRuntime::addConstant` that populates this collection.
> It is translating
> 
> ```
>  cbuffer A {
>float a;
>float b;
> }
> float foo() {
>   return a + b;
> }
> ```
> into
> 
> ```
> struct cb.A.Ty {
>   float a;
>   float b;
> };
> 
> cbuffer A {
>   cb.A.Ty CBA;
> }
> float foo() {
>   return CBA.a + CBA.b;
> }
> ```
> 
> Both a and b are in the global scope and will get a GlobalVariable in clang 
> codeGen.
> By doing the translation, we can ensure each buffer map to one GlobalVariable 
> and save cbuffer layout in the value type of that GlobalVariable.
Ok, I see, so if we are to translate it to C it would be something similar to:


```
struct A {
   float a;
   float b;
} cbuffer_A __attribute__((address_space(256)));

float foo() {
  return cbuffer_A.a + cbuffer_A.b;
}
```
Maybe you can add some comments to explain the intent of this code at a higher 
level... not sure if the generation can reuse or be made a bit close to the 
regular C structs + address spaces...




Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:119
+void CGHLSLRuntime::addConstant(VarDecl *D, Buffer &CB) {
+  if (D->getStorageClass() == SC_Static) {
+// For static inside cbuffer, take as global static.

btw is this covered in the test?



Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:152
+  // as it only refers to globally scoped declarations.
+  CGM.EmitTopLevelDecl(it);
+} else if (NamespaceDecl *ND = dyn_cast(it)) {

Ok I think you don't have this in the tests too and the one below?




Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:183
+  const unsigned CBufferAddressSpace =
+  ASMap[(unsigned)clang::LangAS::hlsl_cbuffer];
+  const unsigned TBufferAddressSpace =

I think it's better to use `getTargetAddressSpace` from `ASTContext`.



Comment at: clang/test/CodeGenHLSL/nest_cbuf.hlsl:8
+  // CHECK: @[[TB:.+]] = external addrspace(5) constant { float, i32, double }
+  tbuffer A : register(t2, space1) {
+float c;

is this generated as nested structs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130131

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

[PATCH] D130131: [HLSL] CodeGen hlsl cbuffer/tbuffer.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:61
+
+GlobalVariable *replaceCBuffer(CGHLSLRuntime::CBuffer &CB) {
+  const unsigned CBufferAddressSpace = 4;

I don't think I understand the intent of this function along with 
`CGHLSLRuntime::addConstant` that populates this collection.



Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:62
+GlobalVariable *replaceCBuffer(CGHLSLRuntime::CBuffer &CB) {
+  const unsigned CBufferAddressSpace = 4;
+  const unsigned TBufferAddressSpace = 5;

It might be better to avoid using hard-coded constants. Are you adding new 
entires in clang's `AddressSpace` enum to represent different logical memory 
segment of the language?



Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:142
+  if (!Inner) {
+DiagnosticsEngine &Diags = CGM.getDiags();
+unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error,

Is this case covered by the test?



Comment at: clang/lib/CodeGen/CGHLSLRuntime.h:39
+llvm::StringRef Name;
+bool IsCBuffer;
+unsigned Reg;

what does this field represent?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130131

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

[PATCH] D128436: [OpenCL] Remove fast_ half geometric builtins

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia 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/D128436/new/

https://reviews.llvm.org/D128436

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

[PATCH] D128434: [OpenCL] Remove half scalar vload/vstore builtins

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia 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/D128434/new/

https://reviews.llvm.org/D128434

___
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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D127579#3598308 , @nikic wrote:

> In D127579#3598306 , @Anastasia 
> wrote:
>
>> In D127579#3595461 , @nikic wrote:
>>
>>> In D127579#3588626 , @Anastasia 
>>> wrote:
>>>
 In D127579#3586092 , @nikic 
 wrote:

> @Anastasia Thanks, that does sound like a legitimate reason to include 
> the information. I want to double check though, does linking the modules 
> actually fail if the functions have signatures that differ only by 
> pointer types? At least for normal LLVM IR this would work fine, and 
> would just result in the insertion of a bitcast during linking (and then 
> typically the bitcast would get shifted from the called function to the 
> call arguments later).

 @nikic If I use `spirv-link` with two modules that have mismatching 
 pointee type in a function parameter I get an error:

   error: 0: Type mismatch on symbol "foo" between imported 
 variable/function %6 and exported variable/function %17. 

 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.
>>>
>>> Okay ... can we maybe turn this around then? Always emit function 
>>> parameters as `i8*` and bitcast them as needed, even if it is possible to 
>>> guess a better type based on the definition? (Let's ignore the image type 
>>> case here, which seems to have different requirements from the rest.)
>>
>> So where would bitcasts be emitted to reconstruct the function prototypes 
>> correctly?
>
> The bitcasts would be in the definition only, when the pointer arguments 
> actually get used in a typed manner. The prototype would always include `i8*` 
> arguments only.

Ok, so does it mean that all pointer parameters in function definitions 
compiled for SPIR-V targets in clang would just be cast to the typed pointer? 
Then we only have untyped pointers in the declarations of functions? Perhaps I 
am missing something but would it not just be easier to keep the pointer types 
completely as it used to be, since it is going to be present nearly everywhere 
apart from declarations?


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] D127961: [OpenCL] Reduce emitting candidate notes for builtins

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia 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/D127961/new/

https://reviews.llvm.org/D127961

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

[PATCH] D128012: [HLSL] Add ExternalSemaSource & vector alias

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D128012#3592126 , @beanz wrote:

> In D128012#3592098 , @Anastasia 
> wrote:
>
>> aha, are you having a lot of these types then? Ok that sounds similar 
>> problem to OpenCL builtin functions as we had to go away from a simple 
>> header include due to long parsing time.
>
> Very similar problem. We have a two-part problem where we both have enough of 
> them that parsing a header would be too slow _and_ the HLSL language actually 
> can't represent them. The later we're working to fix. My hope is that 
> eventually we can write the HLSL libraries in HLSL and use a pre-compilation 
> step (PCH or Module-style) to get past the performance issues.

PCH have large in-memory size compared to the sources, but if it's not an issue 
then it should be reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128012

___
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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D127579#3595461 , @nikic wrote:

> In D127579#3588626 , @Anastasia 
> wrote:
>
>> In D127579#3586092 , @nikic wrote:
>>
>>> @Anastasia Thanks, that does sound like a legitimate reason to include the 
>>> information. I want to double check though, does linking the modules 
>>> actually fail if the functions have signatures that differ only by pointer 
>>> types? At least for normal LLVM IR this would work fine, and would just 
>>> result in the insertion of a bitcast during linking (and then typically the 
>>> bitcast would get shifted from the called function to the call arguments 
>>> later).
>>
>> @nikic If I use `spirv-link` with two modules that have mismatching pointee 
>> type in a function parameter I get an error:
>>
>>   error: 0: Type mismatch on symbol "foo" between imported variable/function 
>> %6 and exported variable/function %17. 
>>
>> 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.
>
> Okay ... can we maybe turn this around then? Always emit function parameters 
> as `i8*` and bitcast them as needed, even if it is possible to guess a better 
> type based on the definition? (Let's ignore the image type case here, which 
> seems to have different requirements from the rest.)

So where would bitcasts be emitted to reconstruct the function prototypes 
correctly?


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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

Btw do we have any issue tracking this failure, I am having a feeling that the 
translation of special types might be related to 
https://github.com/llvm/llvm-project/issues/55121? I am not aware at which 
point libclc is expected to be linked for SPIR-V but if linking happens at 
SPIR-V binary level it is likely going to face the other problem I described 
here.

Either way we should track this problem and we probably need a solution ideally 
within a month or so, as we can't release clang-15 with broken SPIR-V 
support... potentially one solution is to enable pointer types just for SPIR-V 
but this is probably the least option we should consider.


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] D128012: [HLSL] Add ExternalSemaSource & vector alias

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D128012#3591828 , @beanz wrote:

> For just the type alias, adding it during parsing would probably work. Where 
> things get more complicated is we have a whole mess of other data types that 
> we’ll need to add too eventually. One of the advantages of using an external 
> sema source is that we can create incomplete records during initialization, 
> and complete them when the sema source gets the callback from lookup. That 
> gives us cheap and efficient lazy-initialization of our buffer types which 
> (in DXC) has a huge impact on compile time.

aha, are you having a lot of these types then? Ok that sounds similar problem 
to OpenCL builtin functions as we had to go away from a simple header include 
due to long parsing time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128012

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

[PATCH] D128012: [HLSL] Add ExternalSemaSource & vector alias

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

I wonder if accepting type alias with identifier `vector` while parsing would 
be simpler to implement? Then you could just add those type aliases into the 
internal header. I assume user code is not allowed to redefine those i.e. it 
would result in undefined behavior or something like for most of C++ standard 
library features...

It has a drawback of needing to check names for every type alias but it is an 
invalid program when used in other cases anyway so it should not be a concern 
for the parsing speed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128012

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

[PATCH] D127961: [OpenCL] Reduce emitting candidate notes for builtins

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:11224
+  // so do not generate such notes.
+  if (S.getLangOpts().OpenCL && Fn->isImplicit() &&
+  Cand->FailureKind != ovl_fail_bad_conversion)

It would have been nice to print each of those overloads but my guess is that 
it's too much work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127961

___
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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D127579#3585537 , @nikic wrote:

> In D127579#3585516 , @beanz wrote:
>
>> 
>
> Clang is only permitted to encode pointer type information if that pointer 
> type has some kind of direct semantic meaning -- say, if the pointer element 
> type affects the call ABI in some way. If it does not have direct semantic 
> meaning, then deriving the pointer type based on usage (as DXIL does) and 
> using that for emission would be right approach.

@nikic what is the way for clang to encode the pointer type? Is it just 
generated regularly as before or using something like `elementtype` hint?


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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D127579#3590632 , @aeubanks wrote:

> If I'm understanding correctly, previously the LLVM pointee type was used to 
> represent a frontend OpenCL type. An alternative to marking everything with 
> `elementtype` or adding metadata, can something be done in the frontend to 
> mangle the function name with OpenCL pointee types, then the backend 
> translates that to the proper SPIRV types?

We were trying to get away from relying on mangled names in the backend as it 
is harder to target for non-clang and non-OpenCL based languages.


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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D127579#3588651 , @bader wrote:

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

Ok, I see. That confirms I am guessing that in the standard SPIR-V binary we 
won't be able to apply LLVM IR's trick.


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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D127579#3586092 , @nikic wrote:

> @Anastasia Thanks, that does sound like a legitimate reason to include the 
> information. I want to double check though, does linking the modules actually 
> fail if the functions have signatures that differ only by pointer types? At 
> least for normal LLVM IR this would work fine, and would just result in the 
> insertion of a bitcast during linking (and then typically the bitcast would 
> get shifted from the called function to the call arguments later).

If I use `spirv-link `with two modules that have mismatching pointee type in a 
function parameter I get an error:

  error: 0: Type mismatch on symbol "foo" between imported variable/function %6 
and exported variable/function %17. 

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.


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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D127579#3585537 , @nikic wrote:

> 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.
>
> Okay ... I guess my next question would be how the SPIR-V situation differs 
> from the DXIL situation. For DXIL you have already implemented code to 
> "guess" pointer types insofar as they are relevant -- does SPIR-V need 
> something more than that?
>
> Clang is only permitted to encode pointer type information if that pointer 
> type has some kind of direct semantic meaning -- say, if the pointer element 
> type affects the call ABI in some way. If it does not have direct semantic 
> meaning, then deriving the pointer type based on usage (as DXIL does) and 
> using that for emission would be right approach.

I guess the problem might be that we can't always derive the type, for example 
if we have a translation unit:

  extern void foo(int * ptr);
  kernel void k(int * ptr) {
foo(ptr);
  }

then if in another translation module you have something like:

  void foo(int * ptr){
*ptr = 1;
  }

For this program `foo` will translate into SPIR-V with different function 
prototype because in the first unit the type can't be deduced or deduced to 
some default type but in the second case it is deduced to the exact type. So 
the functions in two modules will have two different prototypes when mapped 
into SPIR-V and therefore linking won't be resolved?.. while in OpenCL sources 
it is just the same function `foo`.


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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

The patch seems very straight forward. I wonder though if we should always 
enable this for SPIR-V w/o  adding any flag until the untyped pointers are 
added to SPIR-V so it will become a target setting controlled instead of a flag?

Also is there anything that we don't need from the metadata that are currently 
emitted for the use case i.e. for example argument names? This could reduce the 
number of metadata emitted...

I am not sure if this is something that other languages or backends might also 
need in which case we might need to make this more universal... Ideally it 
would be nice if it was not in metadata as it is somewhat optional... but 
potentially we don't have many other alternatives?


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] D126857: [HLSL] Add WaveActiveCountBits as Langugage builtin function for HLSL

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia 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/D126857/new/

https://reviews.llvm.org/D126857

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

[PATCH] D126857: [HLSL] Add WaveActiveCountBits as Langugage builtin function for HLSL

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/include/clang/Basic/Builtins.def:1697
+// HLSL
+LANGBUILTIN(WaveActiveCountBits, "Uib", "nc", HLSL_LANG)
+

python3kgae wrote:
> Anastasia wrote:
> > FYI we most of time try to add a builtin name using a reserved identifier 
> > which is not part of the language standard (mostly prefixed by `__`). Then 
> > we add regular function that just calls the clang builtins. This way 
> > provides more flexibility to the implementers. However you might not need 
> > this... in which case using original name avoids an extra call.
> Yes. For this one, it is without prefix to avoid extra call.
> I'm following things like enqueue_kernel in opencl.
> For other things support overload which has to make extra call, I'll add the 
> prefix.
Ok, although `enqueue_kernel` was implemented as a builtin because it has a 
weird variadic prototype that can't be implemented using normal features of 
C/C++ languages. Hence it is a builtin with custom SemaChecking.



Comment at: clang/test/SemaHLSL/Wave.hlsl:7
+uint foo(bool b) {
+return WaveActiveCountBits(b);
+}

python3kgae wrote:
> Anastasia wrote:
> > if you are planning to add a proper CodeGen test later then here it might 
> > be sufficient to have a `-verify` test and check that the builtin is 
> > accepted with the right arguments... you might want to add a test checking 
> > that a diagnostic is provided when the arguments are wrong... although this 
> > is not strictly necessary as we already test clang builtins quite 
> > extensively.
> Changed to verify test.
ok, then also `-ast-dump  -o -` can be removed from the Run line. Also you 
probably don't need `-finclude-default-header` here wither?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126857

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

[PATCH] D126857: [HLSL] Add WaveActiveCountBits as Langugage builtin function for HLSL

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/include/clang/Basic/Builtins.def:1697
+// HLSL
+LANGBUILTIN(WaveActiveCountBits, "Uib", "nc", HLSL_LANG)
+

FYI we most of time try to add a builtin name using a reserved identifier which 
is not part of the language standard (mostly prefixed by `__`). Then we add 
regular function that just calls the clang builtins. This way provides more 
flexibility to the implementers. However you might not need this... in which 
case using original name avoids an extra call.



Comment at: clang/test/SemaHLSL/Wave.hlsl:7
+uint foo(bool b) {
+return WaveActiveCountBits(b);
+}

if you are planning to add a proper CodeGen test later then here it might be 
sufficient to have a `-verify` test and check that the builtin is accepted with 
the right arguments... you might want to add a test checking that a diagnostic 
is provided when the arguments are wrong... although this is not strictly 
necessary as we already test clang builtins quite extensively.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126857

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

[PATCH] D126857: [HLSL] Add WaveActiveCountBits as Langugage builtin function for HLSL

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

ok, so the reason you are adding this to clang is that it needs to be mapped 
into a target intrinsic?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126857

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

[PATCH] D124753: [HLSL] Set main as default entry.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D124753#3550670 , @python3kgae 
wrote:

> In D124753#3550337 , @Anastasia 
> wrote:
>
>> In D124753#3546608 , @python3kgae 
>> wrote:
>>
>>> In D124753#3545779 , @Anastasia 
>>> wrote:
>>>
 From the current change it seems to me that what you need to be testing is 
 a just that the frontend options are being passed correctly? This should 
 then be a driver test with `-###` checking for the options to be set for 
 the frontend invocation...
>>>
>>> There's already a driver test with '-###' in 
>>> https://reviews.llvm.org/D124751#change-af6Z62NjlfGb
>>
>> This test doesn't seem to correspond to the change being added as you are 
>> changing the command-line flags. You don't actually add/generate any 
>> attributes in this patch.
>
> Sorry to make things confusing. I should not split default value for -E 
> option as a separate PR :(
>
> There's dxc_E.hlsl (https://reviews.llvm.org/D124751#change-af6Z62NjlfGb) in 
> https://reviews.llvm.org/D124751 where the -E option is added.
> dxc_E.hlsl will test -E option translated into -hlsl-entry for cc1.
>
> There was a test for codeGen of -E option in 
> https://reviews.llvm.org/D124752, but I removed it because it is to almost 
> the same as https://reviews.llvm.org/D124752#change-w4NWvaT68Dhk which test 
> codeGen for ShaderAttr.
>
> And the entry_default.hlsl in current PR test codeGen for the default main 
> and -E option.
>
> I can add a separate codeGen test for -E option if that's what you thought is 
> missing.

Yes, in this patch you are just changing the marchaling flag to `-cc1` right? 
It would be better if you test just exactly that and then in dependent patches 
you can test what is related to other changes. It will make things less 
confusing then. ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124753

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

[PATCH] D126660: [OpenCL] Reword unknown extension pragma diagnostic

[Anastasia Stulova via Phabricator via cfe-commits]

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

Ok, makes sense! Thanks!

Btw I was thinking we should provide some way for developers to know what 
extensions are being supported either through documentation or by querying 
clang somehow? I am guessing documentation would be easier to implement but 
harder to keep in sync?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126660

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

[PATCH] D124752: [HLSL] clang codeGen for HLSLShaderAttr.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia 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/D124752/new/

https://reviews.llvm.org/D124752

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

[PATCH] D124753: [HLSL] Set main as default entry.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D124753#3546608 , @python3kgae 
wrote:

> In D124753#3545779 , @Anastasia 
> wrote:
>
>> From the current change it seems to me that what you need to be testing is a 
>> just that the frontend options are being passed correctly? This should then 
>> be a driver test with `-###` checking for the options to be set for the 
>> frontend invocation...
>
> There's already a driver test with '-###' in 
> https://reviews.llvm.org/D124751#change-af6Z62NjlfGb

This test doesn't seem to correspond to the change being added as you are 
changing the command-line flags. You don't actually add/generate any attributes 
in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124753

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

[PATCH] D125052: [HLSL] Enable vector types for hlsl.

[Anastasia Stulova via Phabricator via cfe-commits]

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

LGTM! Thanks

Please make sure to reflect in your commit message that you are adding the 
clang internal headers too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125052

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

[PATCH] D124753: [HLSL] Set main as default entry.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

From the current change it seems to me that what you need to be testing is a 
just that the frontend options are being passed correctly? This should then be 
a driver test with `-###` checking for the options to be set for the frontend 
invocation...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124753

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

[PATCH] D124752: [HLSL] clang codeGen for HLSLShaderAttr.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2606
+  if (getLangOpts().HLSL && TargetDecl) {
+if (const FunctionDecl *FD = dyn_cast_or_null(TargetDecl))
+  getHLSLRuntime().addHLSLFunctionAttributes(FuncAttrs, RetAttrs, FD);

I think `TargetDecl` can't be nullptr at this point due to the above check? So 
perhaps using `dyn_cast` is better here?



Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:55
+void CGHLSLRuntime::addHLSLFunctionAttributes(llvm::AttrBuilder &FuncAttrs,
+  llvm::AttrBuilder &RetAttrs,
+  const FunctionDecl *FD) {

This parameter seems to be unused?



Comment at: clang/test/CodeGenHLSL/shader_type_attr.hlsl:3
+
+// Make sure not mangle entry.
+// CHECK:define void @foo()

python3kgae wrote:
> Anastasia wrote:
> > I struggle understand the difference in this test? Can it be merged with 
> > the above?
> The difference between these 2 tests is entry.hlsl test for entry which does 
> not have shader attribute on foo while shader_type_attr.hlsl test for entry 
> in a library which has shader attribute (the [shader("compute")]).
Ok, I see although in this patch you only add support for generating the 
attribute in IR so the logic for handling `-E` or input attribute is I assume 
elsewhere and it is tested separately?

But the tests are small so no big concerns to keep both either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124752

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

[PATCH] D124752: [HLSL] clang codeGen for HLSLShaderAttr.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/test/CodeGenHLSL/entry.hlsl:1
+// RUN: %clang --driver-mode=dxc -Tcs_6_1 -Efoo -fcgl -Fo - %s | FileCheck %s
+

Is this test here accidental?



Comment at: clang/test/CodeGenHLSL/shader_type_attr.hlsl:3
+
+// Make sure not mangle entry.
+// CHECK:define void @foo()

I struggle understand the difference in this test? Can it be merged with the 
above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124752

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

[PATCH] D124752: [HLSL] clang codeGen for HLSLShaderAttr.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:54
+
+void clang::CodeGen::CGHLSLRuntime::setHLSLFnuctionAttributes(
+llvm::Function *F, const FunctionDecl *FD) {

I don't think you need `clang::CodeGen::`?



Comment at: clang/lib/CodeGen/CGHLSLRuntime.h:39
+
+  void setHLSLFnuctionAttributes(llvm::Function *, const FunctionDecl *);
 };

typo: `setHLSLFnuctionAttributes` -> `setHLSLFunctionAttributes`



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1654
+  if (getLangOpts().HLSL) {
+if (const FunctionDecl *FD = dyn_cast_or_null(GD.getDecl()))
+  getHLSLRuntime().setHLSLFnuctionAttributes(F, FD);

I think all attributes are expected to be added in `ConstructAttributeList`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124752

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

[PATCH] D124753: [HLSL] Set main as default entry.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/test/CodeGenHLSL/entry_default.hlsl:1
+// RUN: %clang --driver-mode=dxc -Tcs_6_1 -fcgl -Fo - %s | FileCheck %s
+

Would it make sense to test the opposite i.e. if `-E` is passed in the command 
line `main` is not an entry point?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124753

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

[PATCH] D125052: [HLSL] Enable vector types for hlsl.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3490
+  if (!Args.hasArg(options::OPT_dxc_no_stdinc))
+CmdArgs.push_back("-finclude-default-header");
 }

Sharing command line options is great, let's just update the help message 
straight away that this flag now also applies to HLSL: 
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Driver/Options.td#L6053


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125052

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

[PATCH] D125679: [Clang] Added options for integrated backend only used for SPIR-V for now

[Anastasia Stulova via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG730dc4e9bce8: [Clang] Added options for integrated backend. 
(authored by Anastasia).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D125679?vs=429676&id=431938#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125679

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/SPIRV.h
  clang/test/Driver/clang_f_opts.c
  clang/test/Driver/spirv-toolchain.cl

Index: clang/test/Driver/spirv-toolchain.cl
===
--- clang/test/Driver/spirv-toolchain.cl
+++ clang/test/Driver/spirv-toolchain.cl
@@ -69,3 +69,11 @@
 // SPLINK-SAME: "-o" [[BC:".*bc"]]
 // SPLINK: {{llvm-spirv.*"}} [[BC]] "-o" [[SPV2:".*o"]]
 // SPLINK: {{spirv-link.*"}} [[SPV1]] [[SPV2]] "-o" "a.out"
+
+//-
+// Check external vs internal object emission.
+// RUN: %clang -### --target=spirv64 -fno-integrated-objemitter %s 2>&1 | FileCheck --check-prefix=XTOR %s
+// RUN: %clang -### --target=spirv64 -fintegrated-objemitter %s 2>&1 | FileCheck --check-prefix=BACKEND %s
+
+// XTOR: {{llvm-spirv.*"}}
+// BACKEND-NOT: {{llvm-spirv.*"}}
Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -605,3 +605,8 @@
 // CHECK_JMC_WARN_NOT_ELF: -fjmc works only for ELF; option ignored
 // CHECK_NOJMC-NOT: -fjmc
 // CHECK_JMC: -fjmc
+
+// RUN: %clang -### -fintegrated-objemitter -target x86_64 %s 2>&1 | FileCheck -check-prefix=CHECK-INT-OBJEMITTER %s
+// CHECK-INT-OBJEMITTER-NOT: unsupported option '-fintegrated-objemitter' for target
+// RUN: %clang -### -fno-integrated-objemitter -target x86_64 %s 2>&1 | FileCheck -check-prefix=CHECK-NOINT-OBJEMITTER %s
+// CHECK-NOINT-OBJEMITTER: unsupported option '-fno-integrated-objemitter' for target
Index: clang/lib/Driver/ToolChains/SPIRV.h
===
--- clang/lib/Driver/ToolChains/SPIRV.h
+++ clang/lib/Driver/ToolChains/SPIRV.h
@@ -64,8 +64,9 @@
   : ToolChain(D, Triple, Args) {}
 
   bool useIntegratedAs() const override { return true; }
-  bool useIntegratedBackend() const override { return false; }
 
+  bool IsIntegratedBackendDefault() const override { return false; }
+  bool IsNonIntegratedBackendSupported() const override { return true; }
   bool IsMathErrnoDefault() const override { return false; }
   bool isCrossCompiling() const override { return true; }
   bool isPICDefault() const override { return false; }
Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -106,6 +106,34 @@
   IsIntegratedAssemblerDefault());
 }
 
+bool ToolChain::useIntegratedBackend() const {
+  assert(
+  ((IsIntegratedBackendDefault() && IsIntegratedBackendSupported()) ||
+   (!IsIntegratedBackendDefault() || IsNonIntegratedBackendSupported())) &&
+  "(Non-)integrated backend set incorrectly!");
+
+  bool IBackend = Args.hasFlag(options::OPT_fintegrated_objemitter,
+   options::OPT_fno_integrated_objemitter,
+   IsIntegratedBackendDefault());
+
+  // Diagnose when integrated-objemitter options are not supported by this
+  // toolchain.
+  unsigned DiagID;
+  if ((IBackend && !IsIntegratedBackendSupported()) ||
+  (!IBackend && !IsNonIntegratedBackendSupported()))
+DiagID = clang::diag::err_drv_unsupported_opt_for_target;
+  else
+DiagID = clang::diag::warn_drv_unsupported_opt_for_target;
+  Arg *A = Args.getLastArg(options::OPT_fno_integrated_objemitter);
+  if (A && !IsNonIntegratedBackendSupported())
+D.Diag(DiagID) << A->getAsString(Args) << Triple.getTriple();
+  A = Args.getLastArg(options::OPT_fintegrated_objemitter);
+  if (A && !IsIntegratedBackendSupported())
+D.Diag(DiagID) << A->getAsString(Args) << Triple.getTriple();
+
+  return IBackend;
+}
+
 bool ToolChain::useRelaxRelocations() const {
   return ENABLE_X86_RELAX_RELOCATIONS;
 }
Index: clang/include/clang/Driver/ToolChain.h
===
--- clang/include/clang/Driver/ToolChain.h
+++ clang/include/clang/Driver/ToolChain.h
@@ -385,11 +385,23 @@
   /// by default.
   virtual bool IsIntegratedAssemblerDefault() const { return false; }
 
+  /// IsIntegratedBackendDefault - Does this tool chain enable
+  /// -fintegrated-objemitter by default.
+  virtual bool IsIntegratedBackendDefault() const { return true; }
+
+  /// IsIntegratedBackendSup

[PATCH] D125052: [HLSL] Enable vector types for hlsl.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Basic/LangOptions.cpp:122
+  if (Opts.HLSL)
+Includes.push_back("hlsl.h");
 

Is this header expected to be large? You might want to flag up in the 
description of the review and the comments in the header itself what content is 
expected to be there.

If the file is expected to be large it might make sense to add a flag that 
would disable this include. You can then for example use bare clang without 
this header for all the tests that don't require functionality from the header 
to reduce the testing time.



Comment at: clang/test/CodeGenHLSL/basic_types.hlsl:1
+// RUN: %clang_dxc  -Tlib_6_7 -Fo - %s | FileCheck %s
+

Technically mapping into IR types might be target specific, so passing the 
triple is necessary for this test to work correctly. Alternatively you can 
switch to AST dump checking, however even that might be target specific in some 
cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125052

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

[PATCH] D124753: [HLSL] Set main as default entry.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Driver/ToolChains/HLSL.cpp:182
   }
+  // If not set entry, default is main.
+  if (!DAL->hasArg(options::OPT_hlsl_entrypoint)) {

-> `If no set entry` or `If entry is not set explicitly`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124753

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

[PATCH] D124752: [HLSL] clang codeGen for HLSLShaderAttr.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/AST/Mangle.cpp:138
 
+  // HLSL shader entry function never need to be mangled.
+  if (getASTContext().getLangOpts().HLSL && D->hasAttr())

Does HLSL shader entry inherits the same behavior as C-linkage functions  
(extern C) e.g. they can't be overloaded, templated, etc? If that's the case it 
might be easier to add C-linkage to it during parsing and then you can inherit 
the rest of logic including excluding it from mangling... This is how we 
implement kernel function handling in clang for OpenCL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124752

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

[PATCH] D125243: [OpenCL] Make -cl-ext a driver option

[Anastasia Stulova via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd61ded1034bb: [OpenCL] Make -cl-ext a driver option. 
(authored by Anastasia).
Herald added a subscriber: ldrumm.
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D125243?vs=428115&id=431633#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125243

Files:
  clang/docs/OpenCLSupport.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/opencl.cl

Index: clang/test/Driver/opencl.cl
===
--- clang/test/Driver/opencl.cl
+++ clang/test/Driver/opencl.cl
@@ -21,6 +21,7 @@
 // RUN: not %clang -cl-std=invalid -DOPENCL %s 2>&1 | FileCheck --check-prefix=CHECK-INVALID %s
 // RUN: %clang -S -### -target spir-unknown-unknown %s 2>&1 | FileCheck --check-prefix=CHECK-W-SPIR-COMPAT %s
 // RUN: %clang -S -### -target amdgcn-amd-amdhsa-opencl %s 2>&1 | FileCheck --check-prefix=CHECK-NO-W-SPIR-COMPAT %s
+// RUN: %clang -S -### -cl-ext="+test_ext" %s 2>&1 | FileCheck --check-prefix=CHECK-EXT %s
 
 // CHECK-CL: "-cc1" {{.*}} "-cl-std=CL"
 // CHECK-CL10: "-cc1" {{.*}} "-cl-std=CL1.0"
@@ -50,4 +51,6 @@
 // CHECK-W-SPIR-COMPAT: "-Wspir-compat"
 // CHECK-NO-W-SPIR-COMPAT-NOT: "-Wspir-compat"
 
+// CHECK-EXT: "-cc1" {{.*}} "-cl-ext=+test_ext"
+
 kernel void func(void);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3458,6 +3458,9 @@
   if (Arg *A = Args.getLastArg(options::OPT_cl_std_EQ)) {
 std::string CLStdStr = std::string("-cl-std=") + A->getValue();
 CmdArgs.push_back(Args.MakeArgString(CLStdStr));
+  } else if (Arg *A = Args.getLastArg(options::OPT_cl_ext_EQ)) {
+std::string CLExtStr = std::string("-cl-ext=") + A->getValue();
+CmdArgs.push_back(Args.MakeArgString(CLExtStr));
   }
 
   for (const auto &Arg : ForwardedArguments)
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -844,6 +844,7 @@
 def b : JoinedOrSeparate<["-"], "b">, Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass -b  to the linker on AIX (only).">, MetaVarName<"">,
   Group;
+// OpenCL-only Options
 def cl_opt_disable : Flag<["-"], "cl-opt-disable">, Group, Flags<[CC1Option]>,
   HelpText<"OpenCL only. This option disables all optimizations. By default optimizations are enabled.">;
 def cl_strict_aliasing : Flag<["-"], "cl-strict-aliasing">, Group, Flags<[CC1Option]>,
@@ -883,6 +884,11 @@
   MarshallingInfoFlag>;
 def cl_no_stdinc : Flag<["-"], "cl-no-stdinc">, Group,
   HelpText<"OpenCL only. Disables all standard includes containing non-native compiler types and functions.">;
+def cl_ext_EQ : CommaJoined<["-"], "cl-ext=">, Group, Flags<[CC1Option]>,
+  HelpText<"OpenCL only. Enable or disable OpenCL extensions/optional features. The argument is a comma-separated "
+   "sequence of one or more extension names, each prefixed by '+' or '-'.">,
+  MarshallingInfoStringVector>;
+
 def client__name : JoinedOrSeparate<["-"], "client_name">;
 def combine : Flag<["-", "--"], "combine">, Flags<[NoXarchOption, Unsupported]>;
 def compatibility__version : JoinedOrSeparate<["-"], "compatibility_version">;
@@ -6130,15 +6136,6 @@
 
 } // let Flags = [CC1Option, NoDriverOption]
 
-//===--===//
-// OpenCL Options
-//===--===//
-
-def cl_ext_EQ : CommaJoined<["-"], "cl-ext=">,
-  HelpText<"OpenCL only. Enable or disable OpenCL extensions. The argument is a comma-separated sequence of one or more extension names, each prefixed by '+' or '-'.">,
-  Flags<[CC1Option, NoDriverOption]>,
-  MarshallingInfoStringVector>;
-
 //===--===//
 // CUDA Options
 //===--===//
Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -3120,6 +3120,34 @@
 More information about the standard types and functions is provided in :ref:`the
 section on the OpenCL Header `.
 
+.. _opencl_cl_ext:
+
+.. option:: -cl-ext
+
+Enables/Disables support of OpenCL extensions and optional features. All OpenCL
+targets set a list of extensions that they support. Clang allows to amend this using
+the ``-cl-ext`` flag with a comma-separated list of extensions prefixed with
+``'+'`` or ``'-'``. The syntax: ``-cl-ext=<(['-'|'+'][,])+>``,  where
+extensions can be 

[PATCH] D124776: [SPIR-V] Allow setting SPIR-V version via target triple

[Anastasia Stulova 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 rG72832efc941a: [SPIR-V] Allow setting SPIR-V version via 
target triple. (authored by Anastasia).

Changed prior to commit:
  https://reviews.llvm.org/D124776?vs=429678&id=431353#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124776

Files:
  llvm/docs/SPIRVUsage.rst
  llvm/docs/UserGuides.rst
  llvm/include/llvm/ADT/Triple.h
  llvm/lib/Support/Triple.cpp
  llvm/unittests/ADT/TripleTest.cpp

Index: llvm/unittests/ADT/TripleTest.cpp
===
--- llvm/unittests/ADT/TripleTest.cpp
+++ llvm/unittests/ADT/TripleTest.cpp
@@ -243,11 +243,85 @@
 
   T = Triple("spirv32-unknown-unknown");
   EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::NoSubArch, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.0-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v10, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.1-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v11, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.2-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v12, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.3-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v13, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.4-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v14, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.5-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v15, T.getSubArch());
   EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
   EXPECT_EQ(Triple::UnknownOS, T.getOS());
 
   T = Triple("spirv64-unknown-unknown");
   EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::NoSubArch, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.0-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v10, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.1-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v11, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.2-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v12, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.3-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v13, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.4-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v14, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.5-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v15, T.getSubArch());
   EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
   EXPECT_EQ(Triple::UnknownOS, T.getOS());
 
Index: llvm/lib/Support/Triple.cpp
===
--- llvm/lib/Support/Triple.cpp
+++ llvm/lib/Support/Triple.cpp
@@ -495,8 +495,10 @@
 .Case("hsail64", Triple::hsail64)
 .Case("spir", Triple::spir)
 .Case("spir64", Triple::spir64)
-.Case("spirv32", Triple::spirv32)
-.Case("spirv64", Triple::spirv64)
+.Cases("spirv32", "spirv32v1.0", "spirv32v1.1", "spirv32v1.2",
+   "spirv32v1.3", "spirv32v1.4", "spirv32v1.5", Triple::spirv32)
+.Cases("spirv64", "spirv64v1.0", "spirv64v1.1", "spirv64v1.2",
+   "spirv64v1.3", "spirv64v1.4", "spirv64v1.5", Triple::spirv64)
 .StartsWith("kalimba", Triple::kalimba)
 .Case("lanai", Triple::lanai)
 .Case("renderscript

[PATCH] D124382: [Clang] Recognize target address space in superset calculation

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia requested changes to this revision.
Anastasia added a comment.
This revision now requires changes to proceed.

I feel that to progress further on this change, it would be good to get details 
about the use cases and the limitations first.

However, if there is sufficient evidence of the need to extend clang builtins 
with language address spaces I am not convinced the approach here is suitable 
as:

1. It doesn't allow uses of language builtins with language address spaces 
generically as mapping to the target address spaces is not portable. In general 
there can also be generic buitins that are normally mapped to native LLVM 
intrinsics (not target intrinsics!) shared among multiple targets that might 
also benefit from having this implemented in a target agnostic way. Such 
intrinsics could be shares for example between PTX and SPIR-V targets as there 
is quite some overlap in the functionality.
2. It has much wider impact on the language semantics then just allowing 
language address spaces being used in builtins i.e. it results in implicit 
conversions more broadly. This might not be desirable evolution and we might 
need to reach some consensus with more languages or targets using the address 
spaces in order to proceed with such change. In fact current title and 
description doesn't adequately reflect the impact of the change.

Has extending the builtin definition syntax been considered for this problem? 
That seems like a more natural and fairly localized change. For example the 
syntax in 
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Builtins.def#L65
 could be changed to:

  // * -> pointer (optionally followed by an address space number for target 
address space
  //   or by 00 and a number for language address space as it is 
set in LangAS, if no
  //   address space is specified than any address space will be 
accepted)

Note that we will likely need to set LangAS entry values explicitly which would 
make maintaing the enum slightly more painful but it doesn't seem like a 
concern.

If the only use case we have right now is ability to specify generic address 
space in kernel-like langauges we could also just reserve a special `00` number 
in address space field of the buitin prototype description  for `generic 
address space` and leave full support as a future work.

If we understand the use case better we might be able to look at other 
alternatives too...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124382

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

[PATCH] D124382: [Clang] Recognize target address space in superset calculation

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D124382#3496417 , @jchlanda wrote:

> In D124382#3480600 , @Anastasia 
> wrote:
>
>> 
>
>
>
>> And I think we could add this feature in a very light way for example by 
>> reserving the numbers from the clang `LangAS` enum to be used with the 
>> language address spaces in the prototypes of builtins.
>
> I'm not sure I understand how that would look, could you please elaborate?

We could reserve the IDs from LangAS enums to be used in 
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Builtins.def#L65,
 but we could also extend the syntax more naturally i.e. numbers could be used 
for target address spaces and we could add some letter based syntax for 
language address spaces.

>> Just to understand further - do you need the builtins to have a specific 
>> address space prototype? And do you need this to improve error handling in 
>> libclc code base?
>
> With the wrappers suggested we could declare all the pointers to be in 
> generic AS and that would get us around the target vs language AS problem. I 
> don't think that would improve the situation, as from llvm perspective/use 
> case all those builtins would be incorrect and there would be no way for 
> users to tell that there is a specific AS requirement on them, nor would the 
> compiler be able to warn/error. Then the only thing making it work would be 
> those wrappers, embedded deeply in the source of libclc, which at the moment 
> is not even shipped with upstream llvm.
> The builtins in question are not exclusively OpenCL/SYCL related, say 
> `TARGET_BUILTIN(__nvvm_cp_async_ca_shared_global_16, "vv*3vC*1", "", 
> AND(SM_80,PTX70))` would need to take both pointer in a generic address space 
> here. It feels like explicitly providing AS in the prototype is needed.

My understanding was that Clang low level builtins are not desirable for uses 
in the application code directly. They are more targeted at tooling developers 
and low level libraries uses.  Which is why this problem has been worked around 
by declaring the wrapper overload with correct address spaces in the tooling 
projects.

My understanding was that you need those builtins in the libclc code?

>> this for example won’t work for builtins that are shared between targets.
>
> While I agree in principle, I'm not sure if there are any target agnostic and 
> AS specific builtins, sounds like a dangerous thing to introduce. And in any 
> case, it's the target that provides the AS map.

In OpenCL we actually have quite some target agnostic builtins.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124382

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

[PATCH] D110685: [HIPSPV][4/4] Add option to use llc to emit SPIR-V

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

Actually after some more thinking I have decided to go for a generic flag and I 
have created the review for it: https://reviews.llvm.org/D125679. Perhaps you 
can built your functionality on top of it when you get to it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110685

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

[PATCH] D124776: [SPIR-V] Allow setting SPIR-V version via target triple

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 429678.
Anastasia added a comment.

Fixed typo in docs


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

https://reviews.llvm.org/D124776

Files:
  llvm/docs/SPIRVUsage.rst
  llvm/docs/UserGuides.rst
  llvm/include/llvm/ADT/Triple.h
  llvm/lib/Support/Triple.cpp
  llvm/unittests/ADT/TripleTest.cpp

Index: llvm/unittests/ADT/TripleTest.cpp
===
--- llvm/unittests/ADT/TripleTest.cpp
+++ llvm/unittests/ADT/TripleTest.cpp
@@ -243,11 +243,85 @@
 
   T = Triple("spirv32-unknown-unknown");
   EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::NoSubArch, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.0-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v10, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.1-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v11, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.2-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v12, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.3-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v13, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.4-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v14, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.5-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v15, T.getSubArch());
   EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
   EXPECT_EQ(Triple::UnknownOS, T.getOS());
 
   T = Triple("spirv64-unknown-unknown");
   EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::NoSubArch, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.0-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v10, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.1-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v11, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.2-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v12, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.3-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v13, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.4-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v14, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.5-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v15, T.getSubArch());
   EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
   EXPECT_EQ(Triple::UnknownOS, T.getOS());
 
Index: llvm/lib/Support/Triple.cpp
===
--- llvm/lib/Support/Triple.cpp
+++ llvm/lib/Support/Triple.cpp
@@ -495,8 +495,10 @@
 .Case("hsail64", Triple::hsail64)
 .Case("spir", Triple::spir)
 .Case("spir64", Triple::spir64)
-.Case("spirv32", Triple::spirv32)
-.Case("spirv64", Triple::spirv64)
+.Cases("spirv32", "spirv32v1.0", "spirv32v1.1", "spirv32v1.2",
+   "spirv32v1.3", "spirv32v1.4", "spirv32v1.5", Triple::spirv32)
+.Cases("spirv64", "spirv64v1.0", "spirv64v1.1", "spirv64v1.2",
+   "spirv64v1.3", "spirv64v1.4", "spirv64v1.5", Triple::spirv64)
 .StartsWith("kalimba", Triple::kalimba)
 .Case("lanai", Triple::lanai)
 .Case("renderscript32", Triple::renderscript32)
@@ -654,6 +656,16 @@
   if (SubArchName == "arm64e")
 return Triple::AArch64SubArch_arm64e;
 
+  if (SubArchName.startswith("spirv"))
+return StringSwitch(SubArchName)
+.EndsWith("v1.0", Triple::SPIRVSubArch_v10)
+.EndsWi

[PATCH] D125679: [Clang] Added options for integrated backend only used for SPIR-V for now

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added reviewers: mpaszkowski, iliya-diyachkov, svenvh, linjamaki.
Herald added subscribers: ThomasRaoux, ebevhan.
Herald added a project: All.
Anastasia requested review of this revision.
Herald added a subscriber: MaskRay.

Following the new flow for external object code emission provide flags to 
switch between integrated and external backend similar to the integrated 
assembler options.

SPIR-V target is the only user of this functionality at this point.

This patch also updated SPIR-V documentation to clarify that integrated object 
code emission for SPIR-V is an experimental feature.


https://reviews.llvm.org/D125679

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/SPIRV.h
  clang/test/Driver/clang_f_opts.c
  clang/test/Driver/spirv-toolchain.cl

Index: clang/test/Driver/spirv-toolchain.cl
===
--- clang/test/Driver/spirv-toolchain.cl
+++ clang/test/Driver/spirv-toolchain.cl
@@ -69,3 +69,11 @@
 // SPLINK-SAME: "-o" [[BC:".*bc"]]
 // SPLINK: {{llvm-spirv.*"}} [[BC]] "-o" [[SPV2:".*o"]]
 // SPLINK: {{spirv-link.*"}} [[SPV1]] [[SPV2]] "-o" "a.out"
+
+//-
+// Check external vs internal object emission.
+// RUN: %clang -### --target=spirv64 -fno-integrated-objemitter %s 2>&1 | FileCheck --check-prefix=XTOR %s
+// RUN: %clang -### --target=spirv64 -fintegrated-objemitter %s 2>&1 | FileCheck --check-prefix=BACKEND %s
+
+// XTOR: {{llvm-spirv.*"}}
+// BACKEND-NOT: {{llvm-spirv.*"}}
Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -605,3 +605,8 @@
 // CHECK_JMC_WARN_NOT_ELF: -fjmc works only for ELF; option ignored
 // CHECK_NOJMC-NOT: -fjmc
 // CHECK_JMC: -fjmc
+
+// RUN: %clang -### -fintegrated-objemitter -target x86_64 %s 2>&1 | FileCheck -check-prefix=CHECK-INT-OBJEMITTER %s
+// CHECK-INT-OBJEMITTER-NOT: unsupported option '-fintegrated-objemitter' for target
+// RUN: %clang -### -fno-integrated-objemitter -target x86_64 %s 2>&1 | FileCheck -check-prefix=CHECK-NOINT-OBJEMITTER %s
+// CHECK-NOINT-OBJEMITTER: unsupported option '-fno-integrated-objemitter' for target
Index: clang/lib/Driver/ToolChains/SPIRV.h
===
--- clang/lib/Driver/ToolChains/SPIRV.h
+++ clang/lib/Driver/ToolChains/SPIRV.h
@@ -64,8 +64,9 @@
   : ToolChain(D, Triple, Args) {}
 
   bool useIntegratedAs() const override { return true; }
-  bool useIntegratedBackend() const override { return false; }
 
+  bool IsIntegratedBackendDefault() const override { return false; }
+  bool IsNonIntegratedBackendSupported() const override { return true; }
   bool IsMathErrnoDefault() const override { return false; }
   bool isCrossCompiling() const override { return true; }
   bool isPICDefault() const override { return false; }
Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -106,6 +106,34 @@
   IsIntegratedAssemblerDefault());
 }
 
+bool ToolChain::useIntegratedBackend() const {
+  assert(
+  ((IsIntegratedBackendDefault() && IsIntegratedBackendSupported()) ||
+   (!IsIntegratedBackendDefault() || IsNonIntegratedBackendSupported())) &&
+  "(Non-)integrated backend set incorrectly!");
+
+  bool IBackend = Args.hasFlag(options::OPT_fintegrated_objemitter,
+   options::OPT_fno_integrated_objemitter,
+   IsIntegratedBackendDefault());
+
+  // Diagnose when integrated-objemitter options are not supported by this
+  // toolchain.
+  unsigned DiagID;
+  if ((IBackend && !IsIntegratedBackendSupported()) ||
+  (!IBackend && !IsNonIntegratedBackendSupported()))
+DiagID = clang::diag::err_drv_unsupported_opt_for_target;
+  else
+DiagID = clang::diag::warn_drv_unsupported_opt_for_target;
+  Arg *A = Args.getLastArg(options::OPT_fno_integrated_objemitter);
+  if (A && !IsNonIntegratedBackendSupported())
+D.Diag(DiagID) << A->getAsString(Args) << Triple.getTriple();
+  A = Args.getLastArg(options::OPT_fintegrated_objemitter);
+  if (A && !IsIntegratedBackendSupported())
+D.Diag(DiagID) << A->getAsString(Args) << Triple.getTriple();
+
+  return IBackend;
+}
+
 bool ToolChain::useRelaxRelocations() const {
   return ENABLE_X86_RELAX_RELOCATIONS;
 }
Index: clang/include/clang/Driver/ToolChain.h
===
--- clang/include/clang/Driver/ToolChain.h
+++ clang/include/clang/Driver/ToolChain.h
@@ -385,11 +385,23 @@
   /// by default.
   virtual bool IsIntegratedAssemblerDef

[PATCH] D125208: [OpenCL] Fix __remove_address_space documentation code example

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia 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/D125208/new/

https://reviews.llvm.org/D125208

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

[PATCH] D125401: [OpenCL] Do not guard vload/store_half builtins

[Anastasia Stulova via Phabricator via cfe-commits]

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

LGTM! Thanks


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

https://reviews.llvm.org/D125401

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

[PATCH] D125243: [OpenCL] Make -cl-ext a driver option

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/docs/UsersManual.rst:3145-3146
+
+Note that some targets e.g. SPIR/SPIR-V enable all extensions/features in 
clang by
+default.
+

svenvh wrote:
> Was this meant to go after the command example?
true, will fix this in the final commit! Thanks!


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

https://reviews.llvm.org/D125243

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

[PATCH] D124776: [SPIR-V] Allow setting SPIR-V version via target triple

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 428118.
Anastasia added a comment.

- Added v1.5.
- Changed wording in documentation.


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

https://reviews.llvm.org/D124776

Files:
  llvm/docs/SPIRVUsage.rst
  llvm/docs/UserGuides.rst
  llvm/include/llvm/ADT/Triple.h
  llvm/lib/Support/Triple.cpp
  llvm/unittests/ADT/TripleTest.cpp

Index: llvm/unittests/ADT/TripleTest.cpp
===
--- llvm/unittests/ADT/TripleTest.cpp
+++ llvm/unittests/ADT/TripleTest.cpp
@@ -243,11 +243,85 @@
 
   T = Triple("spirv32-unknown-unknown");
   EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::NoSubArch, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.0-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v10, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.1-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v11, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.2-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v12, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.3-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v13, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.4-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v14, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv32v1.5-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v15, T.getSubArch());
   EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
   EXPECT_EQ(Triple::UnknownOS, T.getOS());
 
   T = Triple("spirv64-unknown-unknown");
   EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::NoSubArch, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.0-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v10, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.1-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v11, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.2-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v12, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.3-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v13, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.4-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v14, T.getSubArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64v1.5-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::SPIRVSubArch_v15, T.getSubArch());
   EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
   EXPECT_EQ(Triple::UnknownOS, T.getOS());
 
Index: llvm/lib/Support/Triple.cpp
===
--- llvm/lib/Support/Triple.cpp
+++ llvm/lib/Support/Triple.cpp
@@ -495,8 +495,10 @@
 .Case("hsail64", Triple::hsail64)
 .Case("spir", Triple::spir)
 .Case("spir64", Triple::spir64)
-.Case("spirv32", Triple::spirv32)
-.Case("spirv64", Triple::spirv64)
+.Cases("spirv32", "spirv32v1.0", "spirv32v1.1", "spirv32v1.2",
+   "spirv32v1.3", "spirv32v1.4", "spirv32v1.5", Triple::spirv32)
+.Cases("spirv64", "spirv64v1.0", "spirv64v1.1", "spirv64v1.2",
+   "spirv64v1.3", "spirv64v1.4", "spirv64v1.5", Triple::spirv64)
 .StartsWith("kalimba", Triple::kalimba)
 .Case("lanai", Triple::lanai)
 .Case("renderscript32", Triple::renderscript32)
@@ -654,6 +656,16 @@
   if (SubArchName == "arm64e")
 return Triple::AArch64SubArch_arm64e;
 
+  if (SubArchName.startswith("spirv"))
+return StringSwitch(SubArchName)
+.EndsWith("v1.0", Triple::SPI

[PATCH] D125243: [OpenCL] Make -cl-ext a driver option

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added a reviewer: svenvh.
Herald added subscribers: Naghasan, ebevhan, yaxunl.
Herald added a project: All.
Anastasia requested review of this revision.
Herald added a subscriber: MaskRay.

For generic targets such as SPIR-V clang sets all OpenCL extensions/features as 
supported by default. However targets are unlikely to support all 
extensions/features which creates a problem when such generic SPIR-V binary is 
compiled for a specific target later on.

For SPIR-V we will likely provide control of extensions via vendor components 
in the target triple: 
https://discourse.llvm.org/t/setting-version-environment-and-extensions-for-spir-v-target,
 however this doesn't eliminate the need of finer-grained control of extensions 
for example to support some specific or emerging target architectures.

Therefore this patch exposes `-cl-ext` flag for finer-grained control of 
extensions and features being targeted in the OpenCL kernels to improve error 
reporting and control of features generated in generic binaries like SPIR-V.


https://reviews.llvm.org/D125243

Files:
  clang/docs/OpenCLSupport.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/opencl.cl

Index: clang/test/Driver/opencl.cl
===
--- clang/test/Driver/opencl.cl
+++ clang/test/Driver/opencl.cl
@@ -21,6 +21,7 @@
 // RUN: not %clang -cl-std=invalid -DOPENCL %s 2>&1 | FileCheck --check-prefix=CHECK-INVALID %s
 // RUN: %clang -S -### -target spir-unknown-unknown %s 2>&1 | FileCheck --check-prefix=CHECK-W-SPIR-COMPAT %s
 // RUN: %clang -S -### -target amdgcn-amd-amdhsa-opencl %s 2>&1 | FileCheck --check-prefix=CHECK-NO-W-SPIR-COMPAT %s
+// RUN: %clang -S -### -cl-ext="+test_ext" %s 2>&1 | FileCheck --check-prefix=CHECK-EXT %s
 
 // CHECK-CL: "-cc1" {{.*}} "-cl-std=CL"
 // CHECK-CL10: "-cc1" {{.*}} "-cl-std=CL1.0"
@@ -50,4 +51,6 @@
 // CHECK-W-SPIR-COMPAT: "-Wspir-compat"
 // CHECK-NO-W-SPIR-COMPAT-NOT: "-Wspir-compat"
 
+// CHECK-EXT: "-cc1" {{.*}} "-cl-ext=+test_ext"
+
 kernel void func(void);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3453,6 +3453,9 @@
   if (Arg *A = Args.getLastArg(options::OPT_cl_std_EQ)) {
 std::string CLStdStr = std::string("-cl-std=") + A->getValue();
 CmdArgs.push_back(Args.MakeArgString(CLStdStr));
+  } else if (Arg *A = Args.getLastArg(options::OPT_cl_ext_EQ)) {
+std::string CLExtStr = std::string("-cl-ext=") + A->getValue();
+CmdArgs.push_back(Args.MakeArgString(CLExtStr));
   }
 
   for (const auto &Arg : ForwardedArguments)
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -844,6 +844,7 @@
 def b : JoinedOrSeparate<["-"], "b">, Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass -b  to the linker on AIX (only).">, MetaVarName<"">,
   Group;
+// OpenCL-only Options
 def cl_opt_disable : Flag<["-"], "cl-opt-disable">, Group, Flags<[CC1Option]>,
   HelpText<"OpenCL only. This option disables all optimizations. By default optimizations are enabled.">;
 def cl_strict_aliasing : Flag<["-"], "cl-strict-aliasing">, Group, Flags<[CC1Option]>,
@@ -883,6 +884,11 @@
   MarshallingInfoFlag>;
 def cl_no_stdinc : Flag<["-"], "cl-no-stdinc">, Group,
   HelpText<"OpenCL only. Disables all standard includes containing non-native compiler types and functions.">;
+def cl_ext_EQ : CommaJoined<["-"], "cl-ext=">, Group, Flags<[CC1Option]>,
+  HelpText<"OpenCL only. Enable or disable OpenCL extensions/optional features. The argument is a comma-separated "
+   "sequence of one or more extension names, each prefixed by '+' or '-'.">,
+  MarshallingInfoStringVector>;
+
 def client__name : JoinedOrSeparate<["-"], "client_name">;
 def combine : Flag<["-", "--"], "combine">, Flags<[NoXarchOption, Unsupported]>;
 def compatibility__version : JoinedOrSeparate<["-"], "compatibility_version">;
@@ -6106,15 +6112,6 @@
 
 } // let Flags = [CC1Option, NoDriverOption]
 
-//===--===//
-// OpenCL Options
-//===--===//
-
-def cl_ext_EQ : CommaJoined<["-"], "cl-ext=">,
-  HelpText<"OpenCL only. Enable or disable OpenCL extensions. The argument is a comma-separated sequence of one or more extension names, each prefixed by '+' or '-'.">,
-  Flags<[CC1Option, NoDriverOption]>,
-  MarshallingInfoStringVector>;
-
 //===--===//
 // CUDA Options
 //===--===//
Index: clang/docs/UsersManual.rst

[PATCH] D124776: [SPIR-V] Allow setting SPIR-V version via target triple

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D124776#3497689 , @iliya-diyachkov 
wrote:

> @Anastasia, when are you going to add support for v1.5? We would use it in 
> the SPIR-V backend.

Ok, I can add it straight away if there is a use-case. So far I have taken the 
list from what the `llvm-spirv` translator supports.


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

https://reviews.llvm.org/D124776

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