[PATCH] D155769: [HIP][Clang][docs][RFC] Add documentation for C++ Parallel Algorithm Offload

[Ronan Keryell via Phabricator via cfe-commits]

keryell accepted this revision.
keryell added a comment.

Thanks for the clarifications.


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

https://reviews.llvm.org/D155769

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

[PATCH] D155769: [HIP][Clang][docs][RFC] Add documentation for C++ Parallel Algorithm Offload

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



Comment at: clang/docs/HIPSupport.rst:216
+
+Given the following C++ code, which assumes the ``std`` namespace is included:
+

Since this does not sounds like an official wording and this is not a 
recommended practice 
https://isocpp.org/wiki/faq/coding-standards#using-namespace-std, I suggest 
just adding `std::` everywhere since this is an end-user document.
Further more it makes clear that your extension can work with the standard 
library instead of something that would be declared in a namespace from your 
extension.



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

https://reviews.llvm.org/D155769

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

[PATCH] D155850: [Clang][CodeGen][RFC] Add codegen support for C++ Parallel Algorithm Offload

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:5542
+  if (!getLangOpts().HIPStdPar)
+ErrorUnsupported(E, "builtin function");
 

AlexVlx wrote:
> efriedma wrote:
> > AlexVlx wrote:
> > > efriedma wrote:
> > > > This doesn't make sense; we can't just ignore bits of the source code.  
> > > > I guess this is related to "the decision on their validity is 
> > > > deferred", but I don't see how you expect this to work.
> > > This is one of the weirder parts, so let's consider the following example:
> > > 
> > > ```cpp
> > > void foo() { __builtin_ia32_pause(); }
> > > void bar() { __builtin_trap(); }
> > > 
> > > void baz(const vector& v) {
> > > return for_each(par_unseq, cbegin(v), cend(v), [](auto&& x) { if (x 
> > > == 42) bar(); });
> > > }
> > > ```
> > > 
> > > In the case above, what we'd offload to the accelerator, and ask the 
> > > target BE to lower, is the implementation of `for_each`, and `bar`, 
> > > because it is reachable from the latter. `foo` is not reachable by any 
> > > execution path on the accelerator side, however it includes a builtin 
> > > that is unsupported by the accelerator (unless said accelerator is x86, 
> > > which is not impossible, but not something we're dealing with at the 
> > > moment). If we were to actually error out early, in the FE, in these 
> > > cases, there's almost no appeal to what is being proposed, because 
> > > standard headers, as well as other libraries, are littered with various 
> > > target specific builtins that are not going to be supported. This all 
> > > builds on the core invariant of this model / extension / thingamabob, 
> > > which is that the algorithms, and only the algorithms, are targets for 
> > > offload. It thus follows that as long as code that is reachable from an 
> > > algorithm's implementation is safe, all is fine, but we cannot know this 
> > > in the FE / on an AST level, because we need the actual CFG. This part is 
> > > handled in LLVM in the `SelectAcceleratorCodePass` that's in the last 
> > > patch in this series.
> > > 
> > > Now, you will quite correctly observe that there's nothing preventing an 
> > > user from calling `foo` in the callable they pass to an algorithm; they 
> > > might read the docs / appreciate that this won't work, but even there 
> > > they are not safe, because there via some opaque call chain they might 
> > > end up touching some unsupported builtin. My intuition here, which is 
> > > reflected above in letting builtins just flow through, is that such cases 
> > > are better served with a compile time error, which is what will obtain 
> > > once the target BE chokes trying to lower an unsupported builtin. It's 
> > > not going to be a beautiful error, and we could probably prettify it 
> > > somewhat if we were to check after we've done the accelerator code 
> > > selection pass, but it will happen at compile time. Another solution 
> > > would be to emit these as traps (poison + trap for value returning ones), 
> > > but I am concerned that it would lead to really fascinating debug 
> > > journeys.
> > > 
> > > Having said this, if there's a better way to deal with these scenarios, 
> > > it would be rather nice. Similarly, if the above doesn't make sense, 
> > > please let me know.
> > > 
> > Oh, I see; you "optimistically" compile everything assuming it might run on 
> > the accelerator, then run LLVM IR optimizations, then determine late which 
> > bits of code will actually run on the accelerator, which then prunes the 
> > code which shouldn't run.
> > 
> > I'm not sure I really like this... would it be possible to infer which 
> > functions need to be run on the accelerator based on the AST?  I mean, if 
> > your API takes a lambda expression that runs on the accelerator, you can 
> > mark the lambda's body as "must be emitted for GPU", then recursively mark 
> > all the functions referred to by the lambda.
> > 
> > Emiting errors lazily from the backend means you get different diagnostics 
> > depending on the optimization level.
> > 
> > If you do go with this codegen-based approach, it's not clear to me how you 
> > detect that a forbidden builtin was called; if you skip the error handling, 
> > you just get a literal "undef".
> `I'm not sure I really like this...` - actually, I am not a big fan either, 
> however I think it's about the best one can do, given the constraints 
> (consume standard C++, no annotations on the user side etc.). Having tried a 
> few times in the past (and at least once in a different compiler), I don't 
> quite think this can be done on an AST level. It would add some fairly 
> awkward checking during template instantiation (no way to know earlier that a 
> `CallableFoo` was passed to an offloadable algorithm), and it's a bit 
> unwieldy to basically compute the CFG on the AST and mark reachable Callees 
> at that point. Ignoring those, the main reason for which we cannot do this is 
> 

[PATCH] D155769: [Clang][docs][RFC] Add documentation for C++ Parallel Algorithm Offload

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

Interesting.




Comment at: clang/docs/StdParSupport.rst:349
+
+thread t0{[&]() {
+  hipSetDevice(accelerator_0);





Comment at: clang/docs/StdParSupport.rst:354
+}};
+thread t1{[&]() {
+  hitSetDevice(accelerator_1);





Comment at: clang/docs/StdParSupport.rst:360-362
+t0.join();
+t1.join();
+





Comment at: clang/docs/StdParSupport.rst:366-367
+
+   Note that this is a temporary, unsafe workaround for a deficiency in the C++
+   Standard.
+

Another way could be to hide somehow a way to select the device in the policy 
like in https://github.com/KhronosGroup/SyclParallelSTL, which might be 
something included in your point "4." of "Open Questions / Future Developments".
Perhaps better than opening the TLS Pandora box?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155769

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

[PATCH] D154123: [HIP] Start document HIP support by clang

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



Comment at: clang/docs/HIPSupport.rst:30
+
+Clang provides partial HIP support on Intel GPUs using the CHIP-Star project 
``_. CHIP-Star implements the HIP runtime 
over oneAPI Level Zero or OpenCL runtime. The Clang driver uses the HIPSPV 
toolchain to compile HIP device code into LLVM IR, which is subsequently 
translated to SPIRV via the SPIRV backend or the out-of-tree LLVM-SPIRV 
translator. The SPIRV is then bundled and embedded into the host executables.
+

Fix the spelling when we talk about the Khronos SPIR-V standard, by opposition 
to tool names.
Perhaps you can split the line in shorter ones so the diff are more obvious.



Comment at: clang/docs/HIPSupport.rst:65
+
+   clang++ --offload-arch=gfx906 -xhip sample.cpp -o sample
+

yaxunl wrote:
> arsenm wrote:
> > scchan wrote:
> > > missing --hip-link
> > What does hip-link do? Why is it needed? I never use it and it works
> It indicates HIP offloading at the linking stage and creates the HIP 
> toolchain.
> 
> For -fno-gpu-rdc mode, it locates the HIP runtime library and links it.
> 
> For -fgpu-rdc mode, it unbundles object files and does device linking and 
> embedding.
I am a little lost here.
I have the feeling that the newbie syntax (compile+link in one invocation) 
should be
```
clang++ --offload-arch=gfx906 -xhip sample.cpp -o sample
```
since it should be obvious to the Driver that we want to produce a working 
program for a GPU.
Or does this work producing a working program with another meaning?


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

https://reviews.llvm.org/D154123

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

[PATCH] D154123: [HIP] Start document HIP support by clang

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



Comment at: clang/docs/HIPSupport.rst:24
+
+Clang supports HIP on `ROCm platform `_.
+

keryell wrote:
> I thought the idea of HIP was to also target Nvidia GPU? Otherwise I do not 
> understand "portable applications for GPUs" above.
Thinking to the fact this is used by other projects like 
https://github.com/CHIP-SPV/chipStar this is even wider but at the same time 
that project is not up-streamed.


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

https://reviews.llvm.org/D154123

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

[PATCH] D154123: [HIP] Start document HIP support by clang

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



Comment at: clang/docs/HIPSupport.rst:24
+
+Clang supports HIP on `ROCm platform `_.
+

I thought the idea of HIP was to also target Nvidia GPU? Otherwise I do not 
understand "portable applications for GPUs" above.



Comment at: clang/docs/HIPSupport.rst:33
+
+   clang++ -c --offload-arch=gfx906 -xhip test.cpp -o test.o
+

I suggest giving also a simpler example without `-c` to generate directly an 
executable without split compile and link.
Also avoid using `test` as an example as it is confusing as it is also a shell 
builtin on Unix and it might take sometime to understand why the `test` does 
not use the GPU. :-)



Comment at: clang/docs/HIPSupport.rst:35-37
+This command will compile a .cpp file with the -xhip option to indicate that
+the source is a HIP program. However, if the file has a .hip extension, you
+don't need the -xhip option. Clang will automatically recognize it as a HIP





Comment at: clang/docs/HIPSupport.rst:141
+ - Defined with a value of 1 when the target device lacks support for HIP 
image functions.
+   * - ``HIP_API_PER_THREAD_DEFAULT_STREAM``
+ - Defined when the GPU default stream is set to per-thread mode.

Should this one be with underscores too?



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

https://reviews.llvm.org/D154123

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

[PATCH] D140226: [NVPTX] Introduce attribute to mark kernels without a language mode

[Ronan Keryell via Phabricator via cfe-commits]

keryell added subscribers: bader, keryell.
keryell added a comment.

I wonder whether we could not factorize some code/attribute/logic with AMDGPU 
or SYCL.
Is the use case to have for example CUDA+HIP+SYCL in the same TU and thus there 
is a need for different attributes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140226

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

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

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

Yes, your comment idea looks good and helpful to me.
Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118935

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

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

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In D118935#3389452 , @hvdijk wrote:

> It does make sense, then. A slightly more verbose commit message might have 
> helped though :)

Even better, some comments in the code explaining the "why" would have helped. 
The commit messages disappear while the comments are right there when the code 
is read/reviewed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118935

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

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

[Ronan Keryell via Phabricator via cfe-commits]

keryell accepted this revision.
keryell added a comment.

That looks good.




Comment at: clang/include/clang/Basic/AttrDocs.td:459
+   SpecialType T;
+   cgh.single_task([=]() {
+ T.getF2();




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

https://reviews.llvm.org/D114483

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

[PATCH] D114025: [clang][NFC] Inclusive terms: replace some uses of sanity in clang

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In D114025#3140192 , @Quuxplusone 
wrote:

> I think "sanity-check" could be reasonably replaced with "smoke-test," but 
> (1) this PR doesn't do that, and (2) the phrase "smoke-test" is probably 
> //harder// to understand,

It seems difficult considering the potential atmospheric pollution, carbon 
footprint, health issues, lung cancer, drug abuse, etc. implied.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114025

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

[PATCH] D109609: [C++4OpenCL] Add support for multiple address spaced destructors

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In D109609#3006225 , @olestrohm wrote:

> 



> re: @keryell 
> This might work, though I have no idea how that `SomeAPIReturningAddrSpace` 
> would work, since at this point the variable would likely be cast to be in 
> __generic addrspace.
> Also I'm not sure how that would interact with deleted destructors.

You mean for example say that the constructor in `global` address-space would 
have some definition but the constructor in `constant` is deleted?
Ouch. You are killing my suggestion. :-)


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

https://reviews.llvm.org/D109609

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

[PATCH] D109609: [C++4OpenCL] Add support for multiple address spaced destructors

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

While it might be possible to extend arbitrarily C++, I have the feeling that 
having just 1 destructor and have a different code path-code according the 
address space would not be enough.
It is possible to write:

  ~MyDestructor() {
if constexpr (SomeAPIReturningAddressSpace() == ...::Global) {
  /* Global address space code */
}
else if constexpr (...) {
}
  }

It is possible to build higher-level dispatching constructs from this in OpenCL 
C++ library, for example à la `std::visit` from `std::variant`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109609

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

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

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



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

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

All the official Xilinx OpenCL stack is based on legacy SPIR (encoded in LLVM 
6.x IR but this is another story) and I suspect this is the case for other 
companies.
So, do not deprecate or discontinue, please. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108621

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

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

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



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

bader wrote:
> Naghasan wrote:
> > > First, it must be possible to use any host compiler
> > 
> > I don't understand the link with the integration header. SYCL being 
> > implementable as a library is a design principle of the specs but it 
> > doesn't means the clang host compiler has to remain a vanilla C++ compiler.
> > 
> > > information provided in the integration header is used (included) by the 
> > > SYCL runtime implementation, so the header must be available before the 
> > > host compilation starts
> > 
> > Another approach to the integration header would be for clang as the host 
> > compiler to generate the used type traits.
> > > First, it must be possible to use any host compiler
> > 
> > I don't understand the link with the integration header. SYCL being 
> > implementable as a library is a design principle of the specs but it 
> > doesn't means the clang host compiler has to remain a vanilla C++ compiler.
> > 
> > > information provided in the integration header is used (included) by the 
> > > SYCL runtime implementation, so the header must be available before the 
> > > host compilation starts
> > 
> > Another approach to the integration header would be for clang as the host 
> > compiler to generate the used type traits.
> 
> If there are no objections from @keryell, I'd like to prototype this approach 
> for SYCL first to make sure there are no blocking issues.
> This option seems to be worth to explore considering integration header 
> approach disadvantages.
> > > First, it must be possible to use any host compiler
> > 
> > I don't understand the link with the integration header. SYCL being 
> > implementable as a library is a design principle of the specs but it 
> > doesn't means the clang host compiler has to remain a vanilla C++ compiler.
> > 
> > > information provided in the integration header is used (included) by the 
> > > SYCL runtime implementation, so the header must be available before the 
> > > host compilation starts
> > 
> > Another approach to the integration header would be for clang as the host 
> > compiler to generate the used type traits.

Type traits are somehow misleading and probably implementation details.
I guess what this means is a meta-representation of the kernels and their 
API/ABI.

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

No objection.
triSYCL did not have this integration header since we were using Clang as the 
host compiler and were able to generate directly the kernel call on the host 
https://github.com/triSYCL/llvm/blob/sycl/master/lib/SYCL/SYCLSerializeArguments.cpp
 and the kernel stub on device 
https://github.com/triSYCL/llvm/blob/sycl/master/lib/SYCL/SYCLSerializeArgumentsInside.cpp

Obviously, it was in the context of late outlining in LLVM but it can work with 
early outlining in Clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99190

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

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

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In D99190#2659571 , @tschuett wrote:

> The OMPIRBuilder is the modern version of late outlining:
> https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
> You may consider to go that way with SYCL as well.

triSYCL had a late outlining version, giving different trade-offs. 
https://github.com/triSYCL/llvm/blob/sycl/master/lib/SYCL/SYCLKernelFilter.cpp
Now we are just using the early outliner of oneAPI DPC++ because there are 
quite more developers working on it. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99190

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

[PATCH] D73967: Implement _ExtInt as an extended int type specifier.

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In D73967#2653250 , @erichkeane wrote:

> 



>> It comes from an FPGA tool programmer who knows about HLS *and* modern C++, 
>> so probably a very rare specie. :-)
>> The typical FPGA programmer and tool writer is often more focused on C, so 
>> knows barely nothing about generic programming.



> Our FPGA team DOES know HLS and (most of) modern C++, they are the 
> implementers of 
> https://www.intel.com/content/www/us/en/software/programmable/quartus-prime/hls-compiler.html.

"Uses untimed ANSI C++ as the golden design source": is this pre ISO C++98? :-)
Perhaps you should put ISO C++ with some version number there.

Kidding aside, the HLS world comes historically from the C world and thus 
solves problem in the `#pragma` and `__attribute__`. And there is some value in 
this for customers not wanting to dive into C++ and just having minimal change 
to the code besides these `#pragma`. But a the same time it is done with a lot 
of atrocities on the type system when extending this to C++...

> That said, I can see potential value (though the SFINAE to get the unsigned 
> vs signed seems easy enough), but would suggest either the WG14 paper, or to 
> suggest it to the authors of the WG14 paper and see what they say.  Feel free 
> to include me in the chain.

I am not following at all the WG14 papers. I have enough pain just running 
after the C++ papers... :-( But I might have a look on this one...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73967

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

[PATCH] D73967: Implement _ExtInt as an extended int type specifier.

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In D73967#2653044 , @erichkeane wrote:

> In D73967#2653042 , @keryell wrote:
>
>> 



>> Why do you not allow a type able to represent `{-1, 0}`?
>
> Our FPGA team didn't see any value in it, so we didn't propose it to WG14, so 
> we didn't implement it here.  I'm sure a follow-up paper to WG14 would be 
> considered, and if it shows promise/positive reception, we'd welcome a patch 
> here.

It comes from an FPGA tool programmer who knows about HLS *and* modern C++, so 
probably a very rare specie. :-)
The typical FPGA programmer and tool writer is often more focused on C, so 
knows barely nothing about generic programming.
In the meantime we can probably use C++20 concepts, a C++ wrapping type and 
some specialization for the 1bit case to handle this negative cases or just 
have a SYCL-specific fork...
Probably simpler to have a fork than trying to convince the C committee or FPGA 
people not understanding C++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73967

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

[PATCH] D73967: Implement _ExtInt as an extended int type specifier.

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.
Herald added a subscriber: dexonsmith.

An FPGA programmer is hitting this issue from your unit test:

  c++
signed _ExtInt(1) m; // expected-error{{signed _ExtInt must have a bit size 
of at least 2}}

Why do you not allow a type able to represent `{-1, 0}`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73967

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

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

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

> ! In D99190#2650326 , @bader wrote:
>
> 2. I'm looking for suggestions on "OpenCL extensions" clarification.

I said that "OpenCL extensions" are misleading because it can be understood as 
either extensions inside OpenCL (`cl_khr_ext`...) or extensions to C/C++ with 
specific OpenCL features (like `kernel` or `global`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99190

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

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.
Herald added subscribers: jansvoboda11, dang, sstefan1, yaxunl.

By looking at this, did we forgot about adding some documentation along what we 
have for https://clang.llvm.org/docs/ClangOffloadBundler.html ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68166

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

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

[Ronan Keryell via Phabricator via cfe-commits]

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

Great to have some design documentation!
Just a feedback on the first 20%...




Comment at: clang/docs/SYCLSupport.md:23
+files (which are really LLVM IR files) are linked with the `llvm-link` tool.
+The resulting LLVM IR module is then translated into a SPIR-V module using the
+`llvm-spirv` tool and wrapped in a host object file using the

SPIR-V is just an example



Comment at: clang/docs/SYCLSupport.md:35
+- perform compilation separately from linkage
+- compile the device SPIR-V module ahead-of-time for one or more targets
+- perform device code splitting so that device code is distributed across





Comment at: clang/docs/SYCLSupport.md:48
+applies additional restrictions on the device code (e.g. no exceptions or
+virtual calls), generates LLVM IR for the device code only and "integration
+header" which provides information like kernel name, parameters order and data





Comment at: clang/docs/SYCLSupport.md:55
+  - Any LLVM IR transformation can be applied with only one limitation:
+back-end compiler should be able to handle transformed LLVM IR. NOTE: loop
+transformation passes performance impact depends on accurate target





Comment at: clang/docs/SYCLSupport.md:56
+back-end compiler should be able to handle transformed LLVM IR. NOTE: loop
+transformation passes performance impact depends on accurate target
+information, so it make sense to disable such transformation for "virtual"





Comment at: clang/docs/SYCLSupport.md:57
+transformation passes performance impact depends on accurate target
+information, so it make sense to disable such transformation for "virtual"
+targets like SPIR.





Comment at: clang/docs/SYCLSupport.md:60
+  - Optionally: Address space inference pass
+  - Optionally: LLVM IR → SPIR-V translator.
+- **Back-end** - produces native "device" code. It is shown as

Should we add somewhere other back-ends like PTX?



Comment at: clang/docs/SYCLSupport.md:69
+compiler to produce SYCL heterogeneous applications. Second, even if the
+same clang compiler is used for the host compilation, information provided in 
the
+integration header is used (included) by the SYCL runtime implementation, so 
the





Comment at: clang/docs/SYCLSupport.md:80
+- SYCL kernel function object (functor or lambda) lowering. This component
+creates an SPIR kernel function interface for SYCL kernels.
+- Device code diagnostics. This component enforces language restrictions on





Comment at: clang/docs/SYCLSupport.md:95
+...
+using namespace cl::sycl;
+queue Q;





Comment at: clang/docs/SYCLSupport.md:108
+In this example, the compiler needs to compile the lambda expression passed
+to the `cl::sycl::handler::parallel_for` method, as well as the function `foo`
+called from the lambda expression for the device.





Comment at: clang/docs/SYCLSupport.md:114
+portion of the source code (the contents of the lambda expression passed to the
+`cl::sycl::handler::parallel_for` and any function called from this lambda
+expression).





Comment at: clang/docs/SYCLSupport.md:118
+The current approach is to use the SYCL kernel attribute in the runtime to
+mark code passed to `cl::sycl::handler::parallel_for` as "kernel functions".
+The runtime library can't mark foo as "device" code - this is a compiler





Comment at: clang/docs/SYCLSupport.md:125
+
+All SYCL memory objects shared between host and device must be accessed through
+special `accessor` classes. The "device" side implementation of these classes

We need to introduce USM too somehow...



Comment at: clang/docs/SYCLSupport.md:127
+special `accessor` classes. The "device" side implementation of these classes
+contains pointers to the device memory. As there is no way in OpenCL to pass
+structures with pointers inside as kernel arguments all memory objects shared

To be generalized to non OpenCL case.



Comment at: clang/docs/SYCLSupport.md:139
+
+To facilitate the mapping of SYCL kernel data members to SPIR kernel arguments
+and overcome OpenCL limitations we added the generation of an SPIR kernel

To generalize to non SPIR & non OpenCL



Comment at: clang/docs/SYCLSupport.md:140
+To facilitate the mapping of SYCL kernel data members to SPIR kernel arguments
+and overcome OpenCL limitations we added the generation of an SPIR kernel
+function inside the compiler. A SPIR kernel 

[PATCH] D78807: Fix gendered documentation

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/google-objc-global-variable-declaration.rst:47
 The check will give a warning message but will not be able to suggest a fix. 
The
-user need to fix it on his own.
+user needs to fix it on their own.

I wonder whether it would not be more coherent to have more than 1 user here.
But this is just a French point-of-view. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78807

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

[PATCH] D74130: [clang] fix consteval call in default arguements

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:16329
 
+  /// If we can't handle immediate invocations yet. add them to the outer 
scope.
+  /// This occurs for default argument of lambdas as we can't know if the 
lambda




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74130

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

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3704-3705
+  Result = E->EvaluateAsRValue(Eval, Context, true);
+else
+  Result = E->EvaluateAsLValue(Eval, Context, true);
+

Tyker wrote:
> keryell wrote:
> > aaron.ballman wrote:
> > > Tyker wrote:
> > > > aaron.ballman wrote:
> > > > > Under what circumstances would we want the constant expressions to be 
> > > > > lvalues? I would have guessed you would want to call 
> > > > > `Expr::EvaluateAsConstantExpr()` instead of either of these.
> > > > Expr::EvaluateAsConstantExpr will evaluate expression in there value 
> > > > category.
> > > > this can be quite surprising in some situations like:
> > > > ```
> > > > const int g_i = 0;
> > > > [[clang::annotate("test", g_i)]] void t() {} // annotation carries a 
> > > > pointer/reference on g_i
> > > > [[clang::annotate("test", (int)g_i)]] void t1() {} // annotation 
> > > > carries the value 0
> > > > [[clang::annotate("test", g_i + 0)]] void t1() {} // annotation carries 
> > > > the value 0
> > > > 
> > > > ```
> > > > with EvaluateAsRValue in all of the cases above the annotation will 
> > > > carry the value 0.
> > > > 
> > > > optionally we could wrap expression with an LValue to RValue cast and 
> > > > call EvaluateAsConstantExpr this should also work.
> > > Thank you for the explanation. I think wrapping with an lvalue to rvalue 
> > > conversion may make more sense -- `EvaluateAsRValue()` tries really hard 
> > > to get an rvalue out of something even if the standard says that's not 
> > > okay. It'll automatically apply the lvalue to rvalue conversion if 
> > > appropriate, but it'll also do more than that (such as evaluating side 
> > > effects).
> > This needs some motivating comments and explanations in the code.
> > Also the variations on `g_i` annotation above are quite interesting and 
> > should appear in the unit tests. I am unsure they are now.
> > Perhaps more interesting with other values than `0` in the example. :-)
> > I guess we can still use `[[clang::annotate("test", _i)]] void t() {}` to 
> > have a pointer on `g_i` and `(int&)g_i` for reference? To add to the unit 
> > tests if not already checked.
> > 
> i changed to use EvaluateAsConstantExpr + LValueToRvalue cast.
> 
> > This needs some motivating comments and explanations in the code.
> > Also the variations on `g_i` annotation above are quite interesting and 
> > should appear in the unit tests. I am unsure they are now.
> they are tested not as expressively as above but they are tested.
> 
> > Perhaps more interesting with other values than `0` in the example. :-)
> > I guess we can still use `[[clang::annotate("test", _i)]] void t() {}` to 
> > have a pointer on `g_i`
> yes this is how it is working.
> > `(int&)g_i` for reference? To add to the unit tests if not already checked.
> (int&)g_i has the salve value category and same type as g_i it just has a 
> noop cast around it.
> pointer an reference are indistinguishable in an APValue the only difference 
> is the c++ type of what they represent.
> and they are of course lowered to the same thing. so we only need the pointer 
> case.
> 
I see. Thanks for the clarification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88645

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

[PATCH] D76620: [SYCL] Implement __builtin_unique_stable_name.

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a reviewer: Ralender.
keryell added a comment.

Enabling this feature only with SYCL seems like an easy and quick mitigation, 
as SYCL compilers downstream can easily update their runtime to the new builtin 
name.
Otherwise, just removing a feature used for almost 6 months will cause some 
kind of forking pain...
Anyway, improving the feature and implementation at the same time with an RFC 
for a wider usage looks like a great idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76620

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

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

@erichkeane @alexandre.isoard @Naghasan: any feedback in the context of SYCL & 
HLS C++ about this feature extension?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88645

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

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

Great feature for all the weird pieces of hardware all around the world! :-)




Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2229-2233
+  SmallVector Args = {
+  AnnotatedVal,
+  Builder.CreateBitCast(CGM.EmitAnnotationString(AnnotationStr), 
Int8PtrTy),
+  Builder.CreateBitCast(CGM.EmitAnnotationUnit(Location), Int8PtrTy),
+  CGM.EmitAnnotationLineNo(Location),

Curious reindenting with mix-and-match of spaces & tabulations?



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2386-2390
+  llvm::ConstantExpr::getBitCast(ASZeroGV, Int8PtrTy),
+  llvm::ConstantExpr::getBitCast(AnnoGV, Int8PtrTy),
+  llvm::ConstantExpr::getBitCast(UnitGV, Int8PtrTy),
+  LineNoCst,
+  Args,

Same curious reindenting.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3688
+Expr * = Attr->args_begin()[Idx];
+ConstantExpr *CE = dyn_cast(E);
+if (!E) {

aaron.ballman wrote:
> Tyker wrote:
> > aaron.ballman wrote:
> > > `auto *` since the type is spelled out in the initialization.
> > > 
> > > Also, I think this is unsafe -- it looks like during template 
> > > instantiation, any arguments that have a substitution failure will come 
> > > through as `nullptr`, and this will crash.
> > > 
> > > What should happen on template instantiation failure for these arguments? 
> > > I think the whole attribute should probably be dropped with appropriate 
> > > diagnostics rather than emitting a partial attribute, but perhaps you 
> > > have different ideas.
> > When template instantation fails nullptr is put in the Expr arguments and  
> > AddAnnotationAttr will emit a diagnostic and not associate the attribut to 
> > the declaration.
> Great, thank you for the fix!
Perhaps a few comments on this clarification would be useful in the code for 
the plain mortals...



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3704-3705
+  Result = E->EvaluateAsRValue(Eval, Context, true);
+else
+  Result = E->EvaluateAsLValue(Eval, Context, true);
+

aaron.ballman wrote:
> Tyker wrote:
> > aaron.ballman wrote:
> > > Under what circumstances would we want the constant expressions to be 
> > > lvalues? I would have guessed you would want to call 
> > > `Expr::EvaluateAsConstantExpr()` instead of either of these.
> > Expr::EvaluateAsConstantExpr will evaluate expression in there value 
> > category.
> > this can be quite surprising in some situations like:
> > ```
> > const int g_i = 0;
> > [[clang::annotate("test", g_i)]] void t() {} // annotation carries a 
> > pointer/reference on g_i
> > [[clang::annotate("test", (int)g_i)]] void t1() {} // annotation carries 
> > the value 0
> > [[clang::annotate("test", g_i + 0)]] void t1() {} // annotation carries the 
> > value 0
> > 
> > ```
> > with EvaluateAsRValue in all of the cases above the annotation will carry 
> > the value 0.
> > 
> > optionally we could wrap expression with an LValue to RValue cast and call 
> > EvaluateAsConstantExpr this should also work.
> Thank you for the explanation. I think wrapping with an lvalue to rvalue 
> conversion may make more sense -- `EvaluateAsRValue()` tries really hard to 
> get an rvalue out of something even if the standard says that's not okay. 
> It'll automatically apply the lvalue to rvalue conversion if appropriate, but 
> it'll also do more than that (such as evaluating side effects).
This needs some motivating comments and explanations in the code.
Also the variations on `g_i` annotation above are quite interesting and should 
appear in the unit tests. I am unsure they are now.
Perhaps more interesting with other values than `0` in the example. :-)
I guess we can still use `[[clang::annotate("test", _i)]] void t() {}` to 
have a pointer on `g_i` and `(int&)g_i` for reference? To add to the unit tests 
if not already checked.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88645

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

[PATCH] D87282: [SYCL] Assume SYCL device functions are convergent

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

Be lazy




Comment at: clang/test/CodeGenSYCL/convergent.cpp:18
+int main() {
+  kernel_single_task([]() { foo(); });
+  return 0;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87282

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

[PATCH] D83340: Prohibit use of _ExtInt in atomic intrinsic

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

The idea of this `_ExtInt` is to have some extensions.
Since it is an extension, why preventing its use?
For example if I want my 18 bit FPGA BRAM to be accessed atomically?
Or is there an assumption that atomic access can be enabled back with some 
other mode, such as SYCL or HLS C++?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83340



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

[PATCH] D75285: Mark restrict pointer or reference to const as invariant

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In D75285#1897502 , @jeroen.dobbelaere 
wrote:

> I don't think that 'restrict' is a good match for this behavior. For c++, the 
> alias_set proposal 
> (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4150.pdf) would be 
> a better match.


Oh yes. Thanks for the reminder. We have only 18 months left if we want 
something like that in C++23...


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

https://reviews.llvm.org/D75285



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

[PATCH] D73967: Implement _ExtInt as an extended int type specifier.

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:4020
+
+  auto *New = new (*this, TypeAlignment) ExtIntType(IsUnsigned, NumBits);
+  ExtIntTypes.InsertNode(New, InsertPos);

Why no just `auto` without a `*` everywhere?


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

https://reviews.llvm.org/D73967



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

[PATCH] D72857: [SYCL] Driver option to enable SYCL mode and select SYCL version

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



Comment at: clang/include/clang/Driver/Options.td:3419
+def sycl_std_EQ : Joined<["-"], "sycl-std=">, Group, 
Flags<[CC1Option, NoArgumentUnused, CoreOption]>,
+  HelpText<"SYCL language standard to compile for.">, Values<"2015, 121, 
1.2.1, sycl-1.2.1">;
 

I suggest replacing all the 2015 by 2017.
While this is true SYCL 1.2 was published in 2015, SYCL 1.2.1 was published in 
2017. Only 1.2.1 matters here since 1.2 was never fully implemented by any 
conformant implementation. https://en.wikipedia.org/wiki/SYCL



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4034
+  // Ensure the default version in SYCL mode is 1.2.1 (aka 2015)
+  CmdArgs.push_back("-sycl-std=2015");
+}

Replace 2015 by 2017 in both lines above.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2554
+  Opts.SYCLVersion = llvm::StringSwitch(A->getValue())
+ .Cases("2015", "1.2.1", "121", "sycl-1.2.1", 2015)
+ .Default(0U);

Replace 2015 by 2017.



Comment at: clang/lib/Frontend/InitPreprocessor.cpp:456
+// SYCL Version is set to a value when building SYCL applications
+if (LangOpts.SYCLVersion == 2015)
+  Builder.defineMacro("CL_SYCL_LANGUAGE_VERSION", "121");

Replace 2015 by 2017.



Comment at: clang/test/Driver/sycl.c:5
+// RUN: %clang -### -fsycl -sycl-std=121 %s 2>&1 | FileCheck %s 
--check-prefix=ENABLED
+// RUN: %clang -### -fsycl -sycl-std=2015 %s 2>&1 | FileCheck %s 
--check-prefix=ENABLED
+// RUN: %clang -### -fsycl -sycl-std=sycl-1.2.1 %s 2>&1 | FileCheck %s 
--check-prefix=ENABLED

Replace all the 2015 by 2017 here and below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72857



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

[PATCH] D73967: Implement _ExtInt as an extended int type specifier.

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In D73967#1862273 , @erichkeane wrote:

> At the moment it doesn't work because of parsing.  I'm unaware of there is a 
> deeper limitation, but I'll look at it.  I was unaware that we allow integral 
> types for _Complex, I'd looked it up on cppreference and only saw the FP 
> types so I was basically just confused as to what your intent was.


Nice! Telecommunication applications love complex numbers of weird integers on 
FPGA. Of course, it is outside of any common CPU ABI but useful for HLS 
tool-chains based on Clang. Useful also for SYCL extensions for FPGA.


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

https://reviews.llvm.org/D73967



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

[PATCH] D73651: [OpenCL][CUDA][HIP][SYCL] Add norecurse

[Ronan Keryell via Phabricator via cfe-commits]

keryell requested changes to this revision.
keryell added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:918
+  //
+  // SYCL v2.2 s2.10:
+  // kernels cannot include RTTI information, exception classes,

Can you change the SYCL version to "SYCL 1.2.1 s3.10"?
SYCL 2.2 is non normative: it was just a provisional version which is now 
deprecated and the SYCL committee is working on a newer version.
Thanks.


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

https://reviews.llvm.org/D73651



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

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

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



Comment at: clang/lib/Sema/SemaSYCL.cpp:44
+
+class KernelBodyTransform : public TreeTransform {
+public:

Feel free to add more comments in this area up to line 103.



Comment at: clang/lib/Sema/SemaSYCL.cpp:417
+  Util::DeclContextDesc{clang::Decl::Kind::ClassTemplateSpecialization,
+"accessor"}};
+  return matchQualifiedTypeName(Ty, Scopes);

After more thought, perhaps the solution proposed by Victor Lomüller during the 
last SYCL upstreaming meeting about marking accessor classes with some 
attribute instead of detecting concrete type names is a better generic approach.
I am more convinced now by his argument allowing more experimenting with 
alternative runtimes, since it looks like it is the only place we detect type 
names. For example the kernels are marked with an attribute in the runtime 
instead of concretely detecting the `parallel_for()` functions and so on.



Comment at: clang/test/CodeGenSYCL/device-functions.cpp:2
+// RUN: %clang_cc1 -triple spir64 -fsycl-is-device -S -emit-llvm %s -o - | 
FileCheck %s
+
+template 

Missing description about the purpose of this test



Comment at: clang/test/SemaSYCL/fake-accessors.cpp:2
+// RUN: %clang_cc1 -I %S/Inputs -fsycl-is-device -ast-dump %s | FileCheck %s
+
+#include 

Missing description about the purpose of this test.
I am struggling about understanding what this test is for...
OK, after coming back later, I think I got it. I was confused by the fact that 
in the kernels you are using both true accessors (A, B & C) *and* some objects 
with names similar to SYCL accessor.
Is it possible to have some tests without true accessors?



Comment at: clang/test/SemaSYCL/mangle-kernel.cpp:3
+// RUN: %clang_cc1 -triple spir-unknown-unknown-unknown -I %S/Inputs -I 
%S/../Headers/Inputs/include/ -fsycl-is-device -ast-dump %s | FileCheck %s 
--check-prefix=CHECK-32
+#include 
+#include 

Missing description about the purpose of this test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71016



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

[PATCH] D64418: [Docs][OpenCL] Documentation of C++ for OpenCL mode

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

Good improvement!




Comment at: docs/LanguageExtensions.rst:1758
+to enqueue constructor initialization kernel that has a name
+``@_GLOBAL__sub_I_``. This kernel is only present if there
+are any global objects to be initialized in the compiled binary. One way to

"https://reviews.llvm.org/D64418/new/

https://reviews.llvm.org/D64418



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

[PATCH] D60455: [SYCL] Implement SYCL device code outlining

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



Comment at: clang/test/SemaSYCL/device-attributes.cpp:3
+
+[[clang::sycl_kernel]] int gv2 = 0; // expected-warning {{'sycl_kernel' 
attribute only applies to functions}}
+__attribute((sycl_kernel)) int gv3 = 0; // expected-warning {{'sycl_kernel' 
attribute only applies to functions}}

bader wrote:
> aaron.ballman wrote:
> > Fznamznon wrote:
> > > aaron.ballman wrote:
> > > > I'd like to see some more tests covering less obvious scenarios. Can I 
> > > > add this attribute to a lambda? What about a member function? How does 
> > > > it work with virtual functions? That sort of thing.
> > > Actually there is no restrictions for adding this attribute to any 
> > > function to outline device code so I just checked the simplest variant.
> > > 
> > > But I'm working on new patch which will put some requirements on function 
> > > which is marked with `sycl_kernel` attribute. 
> > > This new patch will add generation of OpenCL kernel from function marked 
> > > with `sycl_kernel` attribute. The main idea of this approach is described 
> > > in this [[ 
> > > https://github.com/intel/llvm/blob/sycl/sycl/doc/SYCL_compiler_and_runtime_design.md#lowering-of-lambda-function-objects-and-named-function-objects
> > >  | document ]] (in this document generated kernel is called "kernel 
> > > wrapper").
> > > And to be able to generate OpenCL kernel using function marked with 
> > > `sycl_kernel` attribute we put some requirements on this function, for 
> > > example it must be a template function. You can find these requirements 
> > > and example of proper function which can be marked with `sycl_kernel` in 
> > > this [[ https://github.com/intel/llvm/pull/177#discussion_r290451286 | 
> > > comment ]] .
> > > 
> > > 
> > > Actually there is no restrictions for adding this attribute to any 
> > > function to outline device code so I just checked the simplest variant.
> > 
> > So there are no concerns about code like:
> > ```
> > struct Base {
> >   __attribute__((sycl_kernel)) virtual void foo();
> >   virtual void bar();
> > };
> > 
> > struct Derived : Base {
> >   void foo() override;
> >   __attribute__((sycl_kernel)) void bar() override;
> > };
> > 
> > void f(Base *B, Derived *D) {
> >   // Will all of these "do the right thing"?
> >   B->foo();
> >   B->bar();
> > 
> >   D->foo();
> >   D->bar();
> > }
> > ```
> > Actually there is no restrictions for adding this attribute to any function 
> > to outline device code so I just checked the simplest variant.
> > But I'm working on new patch which will put some requirements on function 
> > which is marked with sycl_kernel attribute.
> 
> @aaron.ballman, sorry for confusing. The  usage scenarios should have been 
> articulated more accurately.
> We have only four uses of this attribute in our implementation:
> https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L538 
> (lines 538-605).
> All four uses are applied to member functions of `cl::sycl::handler` class 
> and all of them have similar prototype (which is mentioned by Mariya in the 
> previous 
> [comment](https://github.com/intel/llvm/pull/177#discussion_r290451286):
> 
> ```
> namespace cl { namespace sycl {
> class handler {
>   template 
>   __attribute__((sycl_kernel)) void sycl_kernel_function(KernelType 
> KernelFuncObj) {
> KernelFuncObj();
>   }
> };
> }}
> ```
> 
> Here is the list of SYCL device compiler expectations with regard to the 
> function marked with `sycl_kernel` attribute.
>   - Function template with at least one parameter is expected. The 
> compiler generates OpenCL kernel and uses first template parameter as unique 
> name to the generated OpenCL kernel. Host application uses this unique name 
> to invoke the OpenCL kernel generated for the `sycl_kernel_function` 
> specialized by this name and KernelType (which might be a lambda type).
>   - Function must have at least one parameter. First parameter expected 
> to be a function object type (named or unnamed i.e. lambda). Compiler uses 
> function object type field to generate OpenCL kernel parameters.
> 
> Aaron, I hope it makes more sense now.
> 
> We don't plan in any use cases other than in SYCL standard library 
> implementation mentioned above.
> If I understand you concerns correctly, you want to be sure that clang 
> prohibits other uses of this attribute, which are not intended. Right?
> What is the best way to do this? Add more negative tests cases and make sure 
> that clang generate error diagnostic messages?
> 
> If I understand you concerns correctly, you want to be sure that clang 
> prohibits other uses of this attribute, which are not intended. Right?

But since it is an attribute to be used by SYCL run-time writers, I am not sure 
there is a lot of value in over-engineering the restrictions of its use. It 
diverts brain power from the real implementation & review and might even 
prevent innovation due to some creative 

[PATCH] D61506: [OpenCL] Switch to C++17

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In D61506#1490555 , @rsmith wrote:

> Per the OpenCL C++ 1.0 specification, section 2:
>
> > The OpenCL C++ programming language is based on the ISO/IEC JTC1 SC22 WG21 
> > N 3690 language specification (a.k.a. C++14 specification).
>
> I think it would be reasonable to permit changing the base C++ standard in 
> OpenCL C++ mode


Indeed! There should be an option to pick the version of C++ the user wants for 
OpenCL.


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

https://reviews.llvm.org/D61506



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

[PATCH] D61506: [OpenCL] Switch to C++17

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



Comment at: include/clang/Frontend/LangStandards.def:162
  OpenCL, "OpenCL C++ 1.0",
- LineComment | CPlusPlus | CPlusPlus11 | CPlusPlus14 | Digraphs | 
OpenCL)
+ LineComment | CPlusPlus | CPlusPlus11 | CPlusPlus14 | CPlusPlus17 
| Digraphs | OpenCL)
 

kpet wrote:
> Suggest you add `HexFloat` as well. It is part of c++17 and all OpenCL 
> versions.
Why only C++17?
I would love to have `CPlusPlus2a` here too...


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

https://reviews.llvm.org/D61506



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

[PATCH] D61304: [OpenCL][PR41609] Deduce static data members to __global addr space

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In D61304#1485814 , @Anastasia wrote:

> In D61304#1485125 , @rjmccall wrote:
>
> > Presumably a similar rule would apply to thread-locals if you supported 
> > them.
>
>
> We don't support them in OpenCL.


But this is also about OpenCL extensions here anyway...
Is thread-local not somewhat equivalent to work-item local, i.e. `__private`?
But I am unsure we want to really handle this, while it is already an issue in 
the discussion about execution contexts in ISO C++... :-)


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

https://reviews.llvm.org/D61304



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

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In D60455#1470402 , @Anastasia wrote:

>




> In the interest of speeding up the upstreaming work, would you be able to 
> highlight the differences and similarity at least for SYCL and OpenCL kernel 
> modes? Not sure if you are familiar enough with both. Because apart from the 
> public announcements I can't see what has been changed in SYCL that would 
> disallow to use OpenCL mode. It would be a valuable input to determine the 
> implementation choices.

SYCL is similar to OpenMP 5 for C++, where you use only C++ classes instead of 
`#pragma`. So it is quite C++-friendlier than OpenMP.
But that means also there is not the same concept of explicit kernel like in 
OpenCL or CUDA. In OpenCL or CUDA, when there is a function with a specific 
attribute, you know it is a kernel and you can compile as such.

In SYCL or OpenMP you need an outliner that will estimate what should be 
executed as an heterogeneous kernel, split the code between the host side and 
the device side, add some glue/stub to implement an RPC between the host and 
the device, manage potentially some allocation/memory transfers, etc. This is 
quite more complex than compiling OpenCL, CUDA or other graphics shader 
languages. This is also why, while SYCL is technically pure standard C++, you 
need some specific compiler magic to do the code massaging to have everything 
working well between a host and some devices.

The attribute we discuss here is just an implementation detail to help the 
coordination between the compiler and the SYCL frontend classes to mark some 
area to outline, without relying to do some precise pattern matching, allowing 
more flexibility in the runtime without changing the compiler every time. So 
while it defines a zone to be outlined as a kernel, it is not really a kernel 
in the sense of OpenCL.

In triSYCL I made some completely different choices, using late outlining in 
LLVM and detecting some specific functions such as 
`cl::sycl::detail::instantiate_kernel()` that defines some stuff I 
want to outline 
https://github.com/triSYCL/triSYCL/blob/master/doc/architecture.rst#low-level-view-of-the-device-compiler-workflow
For me an attribute was not an option because I wanted to change Clang as 
little as possible. But at the end, I think it is quite more brittle than doing 
early outlining in Clang as discussed here, which also requires quite more 
knowledge of Clang than I have. :-)

So at the end, I think we should use a different keyword from OpenCL or CUDA 
because the semantics is different.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455



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

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In D60455#1470030 , @Anastasia wrote:

>




> I am not sure we need to add a keyword actually, the attribute can just be 
> added in AST since it's not supposed to be used in the source code?

The attribute is used by the SYCL headers to mark the functions to be outlined 
to the device.
https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L267
https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L295
https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L308
https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L325

> My understanding of SYCL kernel is that it mainly matches OpenCL kernel 
> functionality because the original intent of SYCL was to provide single 
> source functionality on top of OpenCL.

Yes, this was the idea, when OpenCL was announced in November 2008, to build 
some higher-level programming models on top of it.
Now the SYCL standard is evolving to something more general to bring 
heterogeneous computing to modern C++.
So we could reuse in the same way some attributes from OpenMP 5 or CUDA if the 
Clang/LLVM community thinks it is better.

> But I am not an expert in SYCL to confirm that.

I am pretty sure that the SYCL standard committee would love some active 
participation from ARM. ;-)

> I think what we are missing currently is a thorough analysis/comparison 
> between SYCL device mode and OpenCL kernel language mode to understand what's 
> the best implementation strategy. That would apply to many other features: 
> kernel function restrictions, address spaces, vectors, special types, etc.

That would make definitely sense when we target OpenCL.

> I still see no point in polluting our code base with extra code that just 
> does the same thing. It will save us a lot of time to just work cooperatively 
> on the same problem and even improve readability of the code. But of course 
> this can only be done if there is no need to diverge the implementation 
> significantly.

Yes. Probably that even when the target is not OpenCL, the general principles 
remain similar. Probably the same for CUDA & OpenMP 5 too...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455



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

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

It would be better to rename `clang/test/SemaSYCL/device-attrubutes.cpp` to 
`clang/test/SemaSYCL/device-attributes.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455



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

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

[Ronan Keryell via Phabricator via cfe-commits]

keryell accepted this revision.
keryell added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1000
+def SYCLDevice : InheritableAttr {
+  let Spellings = [GNU<"sycl_device">];
+  let Subjects = SubjectList<[Function, Var]>;

aaron.ballman wrote:
> aaron.ballman wrote:
> > keryell wrote:
> > > Fznamznon wrote:
> > > > aaron.ballman wrote:
> > > > > Is there a reason to not also introduce a C++11 and C2x style 
> > > > > spelling in the `clang` namespace? e.g., `[[clang::sycl_device]]`
> > > > I don't think that it makes sense because these attributes not for 
> > > > public consumption. These attributes is needed to separate code which 
> > > > is supposed to be offloaded from regular host code. I think SYCLDevice 
> > > > attribute actually doesn't need a spelling because it will be added 
> > > > only implicitly by compiler.
> > > 
> > > If we go towards this direction, `[[clang::sycl::device]]` or 
> > > `[[clang::sycl::kernel]]` look more compatible with the concept of name 
> > > space.
> > > While not a public interface, if we have a kind of "standard" outlining 
> > > in Clang/LLVM, some people might want to use it in some other contexts 
> > > too.
> > If these are only being added implicitly by the compiler, then they should 
> > not be given any `Spelling`. See `AlignMac68k` for an example.
> I'm still confused -- are these created implicitly or are they spelled out by 
> the user explicitly? Right now, it looks like they're spelled out explicitly, 
> but I was under the impression they are only intended to be created 
> implicitly by the compiler.
> 
> If they are expected to be explicitly specified by the user, the spelling 
> should be using `Clang<>` instead of using `GNU<>`, `C2x<>`, and `CXX11<>` 
> explicitly.
> 
> > If we go towards this direction, [[clang::sycl::device]] or 
> > [[clang::sycl::kernel]] look more compatible with the concept of name space.
> 
> Attribute namespaces do not work that way. There is the vendor namespace and 
> then the attribute name.
> Attribute namespaces do not work that way. There is the vendor namespace and 
> then the attribute name.

After diving into "9.11.1 Attribute syntax and semantics [dcl.attr.grammar]" of 
the latest C++ draft standard, it looks you are right... There is only 1 level 
of `::` allowed. :-(



Comment at: clang/include/clang/Basic/AttrDocs.td:286
+help.
+  }];
+}

aaron.ballman wrote:
> I'm still not entirely certain how I would know what to mark and how. From 
> the description, it sounds like whoever authors `parallel_for` needs to do 
> this marking, or it somehow happens automatically?
> 
> (I'll do another editorial pass once I understand the intended behavior a bit 
> better -- I expect there will be a few more wording issues to address.)
In normal SYCL it happens automatically.
In the compiler unit-tests it is done manually to exercise the framework.
I am the one who suggested that in some other contexts, it could be used 
manually for some special purpose like using some weird hardware, but I do not 
want to derail the main review with this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455



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

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

While we are thinking about recycling some attributes across languages, we have 
to think about the fact that real applications are often made from various 
languages, such as SYCL + OpenMP 5 or whatever.
It would be nice not to forbid such a combination inside the same TU by design.




Comment at: clang/include/clang/Basic/Attr.td:1000
+def SYCLDevice : InheritableAttr {
+  let Spellings = [GNU<"sycl_device">];
+  let Subjects = SubjectList<[Function, Var]>;

Fznamznon wrote:
> aaron.ballman wrote:
> > Is there a reason to not also introduce a C++11 and C2x style spelling in 
> > the `clang` namespace? e.g., `[[clang::sycl_device]]`
> I don't think that it makes sense because these attributes not for public 
> consumption. These attributes is needed to separate code which is supposed to 
> be offloaded from regular host code. I think SYCLDevice attribute actually 
> doesn't need a spelling because it will be added only implicitly by compiler.

If we go towards this direction, `[[clang::sycl::device]]` or 
`[[clang::sycl::kernel]]` look more compatible with the concept of name space.
While not a public interface, if we have a kind of "standard" outlining in 
Clang/LLVM, some people might want to use it in some other contexts too.



Comment at: clang/include/clang/Basic/Attr.td:1003
+  let LangOpts = [SYCL];
+  let Documentation = [Undocumented];
+}

Fznamznon wrote:
> aaron.ballman wrote:
> > No new, undocumented attributes, please.
> As I said, these attributes are not for public consumption. Should I add 
> documentation in this case too?
Yes, documentation and comments are always appreciated.



Comment at: clang/include/clang/Basic/Attr.td:1010
+  let LangOpts = [SYCL];
+  let Documentation = [Undocumented];
+}

Cf supra.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455



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

[PATCH] D59603: [PR40707][PR41011][OpenCL] Allow addr space spelling without double underscore in C++ mode

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In D59603#1437684 , @Anastasia wrote:

> Updated test to use root namespace (commented by Ronan).


Thank you for testing! :-)


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

https://reviews.llvm.org/D59603



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

[PATCH] D59603: [PR40707][PR41011][OpenCL] Allow addr space spelling without double underscore in C++ mode

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

I was a little bit worried about

  struct top_level { int i; };
  
  private ::top_level a;

but it should be fine because in that case we have a `tok::coloncolon`


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

https://reviews.llvm.org/D59603



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

[PATCH] D58179: [OpenCL][PR40707] Allow OpenCL C types in C++ mode

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

LGTM.


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

https://reviews.llvm.org/D58179



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

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Ronan Keryell via Phabricator via cfe-commits]

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

That looks good. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57768



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

[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In https://reviews.llvm.org/D53705#1285096, @rjmccall wrote:

>




> If your plan is to compile code for the CPU with a normal C++ compiler but 
> for the GPU with an OpenCL-aware compiler, you are going to have 
> significantly divergent behavior between the CPU and GPU language dialects 
> because of the non-standardness of the GPU dialect.

We are trying to minimize the divergence. Thus the use of conditional tense and 
*mostly" word in the sentence "could mostly run on a CPU without a specific 
compiler", as a wishful thinking design trade-off, trying to minimize the 
rewriting a user has to do starting from a plain C++ function.

Probably we can discuss this next week off-line during a ISO C++ SG14 or SG1 
session...


Repository:
  rC Clang

https://reviews.llvm.org/D53705



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

[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In https://reviews.llvm.org/D53705#1284268, @rjmccall wrote:

>




> Okay.  So the purpose of OpenCL C++ is to provide a non-unified model that 
> allows you to easily run C++ code on the CPU.

It is just the second-order purpose.
A C++-based kernel language to program accelerators without C++ language 
extension, so that the code could also mostly run on a CPU without a specific 
compiler.


Repository:
  rC Clang

https://reviews.llvm.org/D53705



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

[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In https://reviews.llvm.org/D53705#1281743, @rjmccall wrote:

>




> New versions of C++ have semi-regularly added keywords like `static_assert` 
> and `thread_local` that are not in the reserved space for identifiers.  When 
> C adopted these, it spelled them `_Static_assert` and `_Thread_local`, which 
> are in the reserved space.  I was under the mistaken (right?) impression 
> thought that e.g. `__constant` was a standardized spelling, and I thought 
> that Khronos had just decided to avoid using the unreserved identifiers in 
> C++ when it had gladly adopted them in C.

Thank you for the clarification.

>   But now I understand that OpenCL C++ is attempting to be a radically 
> different language model that does not actually have qualifiers at all.

Yes.

> By "0 language extension", do you just mean no *syntactic* extensions?  
> Because something like `cl::priv` certainly seems like it requires intrinsic 
> language support, and I don't know what `add_global_t` could possibly do 
> besides add a not-described-in-the-standard-but-still-obviously-there 
> address-space qualifier so that member accesses continue to work and properly 
> preserve the qualifier on the resulting l-value.  C++ doesn't provide 
> adequate language tools to replace pointers and references at a language 
> level, not by a long shot; the tools are only really good enough for resource 
> management.

Since it is C++, you can do a lot with some wrapper classes and so not 
requiring visible language extensions.
But yes, if you dig into the implementation, it will use some attributes, 
address-spaces and so on.

> The "just compile C++ code for the GPU" idea that (IIRC) AMD has been 
> exploring seems quite interesting, but unlike OpenCL C++ it really is a 
> largely no-extensions approach: it's implementing the classic C++ language 
> model (with facilities for taking more control) like any other frontend, just 
> with a weird target on the backend.  It's also just hoping that the optimizer 
> will be able to eliminate the overheads when mapping to a 
> multiple-address-space model, and I'm not sure whether that's been borne out. 
>  OpenCL C++ seems to be trying to strike a middle ground where there are 
> minimal syntactic extensions but the type system is rather radically 
> different in ways that often work but also often don't, and it's basically 
> held together by the underlying extensions that it's pretending aren't there. 
>  That doesn't seem like a successful language approach.

I was really meaning "run on the CPU" and not "run on the GPU", it was not a 
typo from me. :-)
If there are no visible language extensions besides C++ classes, it is easier 
to run the kernel code on a CPU without any specific compiler support, with 
just a plain C++ compiler and just by changing the runtime to launch the kernel.
It was part of the design motivation to remove the alien keywords.
This is even more true with SYCL.

>> That seems like a good discussion topic next Thursday at the LLVM Bay Area 
>> Meetup. :-)
> 
> Alas, I live in New York; I was just out there for the Developer's Meeting 
> but won't be back for some time.

We will miss you, then. :-)


Repository:
  rC Clang

https://reviews.llvm.org/D53705



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

[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In https://reviews.llvm.org/D53705#1278066, @rjmccall wrote:

> I don't suppose there's any chance you can just tell Khronos to fix their 
> stuff.  It's a little funny to be more conservative about keywords in C++ 
> when the C++ committee is actually much more aggressive about adding keywords 
> in the non-reserved space than C is.


You are actually telling this to Khronos since a lot of Khronos members are on 
the LLVM mailing lists and are reading this right now... :-)
I am unsure to understand what you mean about "the C++ committee is actually 
much more aggressive about adding keywords in the non-reserved space than C is".
The motivation behind this was to have 0 language extension to C++ in OpenCL 
C++, like with the single-source version of the standard, SYCL, where you can 
have a program which is executed purely on CPU without any compiler support, to 
avoid the CUDA & C++AMP atrocities.
That seems like a good discussion topic next Thursday at the LLVM Bay Area 
Meetup. :-)


Repository:
  rC Clang

https://reviews.llvm.org/D53705



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

[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

We could submit to the standard an OpenCL C++ extension to accept the OpenCL C 
syntax...


Repository:
  rC Clang

https://reviews.llvm.org/D53705



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