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

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/

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

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

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

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_as

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

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 ac

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

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

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

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 _

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

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 __

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

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

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

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: > a

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

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 __

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

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 head

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

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 f

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

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 t

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

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

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

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

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

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 _

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

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] D60455: [SYCL] Implement SYCL device code outlining

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' a

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

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

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

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 SYC

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

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 mess

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

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 li

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

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/

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

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

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

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 ___

[PATCH] D76620: [SYCL] Implement __builtin_unique_stable_name.

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

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

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 wr

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

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: r

[PATCH] D78807: Fix gendered documentation

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 o

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

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

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

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 se

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

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 Gi

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

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

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

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{[&]() { +

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

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 se

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

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:/

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

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 applicat

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

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

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

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://isocp

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

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

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

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 >

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

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 +

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

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

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

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 looke

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

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.

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

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/

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

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

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

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 ACTI

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

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

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

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}`? Reposito

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

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

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

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 focuse

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

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 wi

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

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: > > > Fi

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

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 Anastasi

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

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 (SomeAPIReturningAdd