[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

jdoerfert added inline comments. Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93 ; -#pragma omp target parallel for private(ps) is_device_ptr(ps) // expected-error{{private variable cannot be in a is_device_ptr clause in '#pragma omp targe

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

jdoerfert added inline comments. Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93 ; -#pragma omp target parallel for private(ps) is_device_ptr(ps) // expected-error{{private variable cannot be in a is_device_ptr clause in '#pragma omp targe

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

jdoerfert added inline comments. Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93 ; -#pragma omp target parallel for private(ps) is_device_ptr(ps) // expected-error{{private variable cannot be in a is_device_ptr clause in '#pragma omp targe

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

jdoerfert added inline comments. Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93 ; -#pragma omp target parallel for private(ps) is_device_ptr(ps) // expected-error{{private variable cannot be in a is_device_ptr clause in '#pragma omp targe

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

jdoerfert added inline comments. Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93 ; -#pragma omp target parallel for private(ps) is_device_ptr(ps) // expected-error{{private variable cannot be in a is_device_ptr clause in '#pragma omp targe

[PATCH] D66173: [Codegen] Updated test for D66158

jdoerfert added a comment. Might not be the best test given that this should arguably be a memset not a memcpy. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66173/new/ https://reviews.llvm.org/D66173 ___ cfe-commit

[PATCH] D66173: [Codegen] Updated test for D66158

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. In D66173#1628079 , @xbolva00 wrote: > hm.. memset? When I say memset, here and in the other review, I mean memmove. In D66173#1628088

[PATCH] D59922: [Attributor] Deduce "no-capture" argument attribute

jdoerfert updated this revision to Diff 215498. jdoerfert added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59922/new/ https://reviews.llvm.org/D59922 Files: llvm/include/llvm/Transforms/IPO/Attributor.h llvm/lib/Analysis/C

[PATCH] D59922: [Attributor] Deduce "no-capture" argument attribute

jdoerfert updated this revision to Diff 215504. jdoerfert added a comment. Extracted CaptureTracking changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59922/new/ https://reviews.llvm.org/D59922 Files: llvm/include/llvm/Transforms/IPO/Attribu

[PATCH] D59922: [Attributor] Deduce "no-capture" argument attribute

jdoerfert updated this revision to Diff 215713. jdoerfert added a comment. Add more test coverage and improve capture information based on dereferenceability Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59922/new/ https://reviews.llvm.org/D59922

[PATCH] D59922: [Attributor] Deduce "no-capture" argument attribute

jdoerfert updated this revision to Diff 215716. jdoerfert added a comment. Add tests accidentally removed Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59922/new/ https://reviews.llvm.org/D59922 Files: llvm/include/llvm/Transforms/IPO/Attributor

[PATCH] D63845: [WIP] Create a clang attribute that lets users specify LLVM attributes

jdoerfert added a comment. Please add full context to the patches (http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface) Also, some general comments inlined. Comment at: clang/include/clang/Basic/Attr.td:1652 + let Args = [StringArgument<"AttrName">

[PATCH] D63845: [WIP] Create a clang attribute that lets users specify LLVM attributes

jdoerfert added a comment. In D63845#1560605 , @aaron.ballman wrote: > In D63845#1559995 , @lebedev.ri > wrote: > > > What's the target use-case here? What can't be solved with normal > > attributes? > With "no

[PATCH] D59922: [Attributor] Deduce "no-capture" argument attribute

jdoerfert added a comment. @hfinkel ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59922/new/ https://reviews.llvm.org/D59922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.l

[PATCH] D63845: [WIP] Create a clang attribute that lets users specify LLVM attributes

jdoerfert added a comment. In D63845#1561983 , @lebedev.ri wrote: > In D63845#1561793 , @jdoerfert wrote: > > > In D63845#1560605 , @aaron.ballman > > wrote: > > > > > In D6

[PATCH] D59980: [Attributor] Deduce memory behavior argument attributes

jdoerfert added a subscriber: fhahn. jdoerfert added a comment. @hfinkel @fhahn ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59980/new/ https://reviews.llvm.org/D59980 ___ cfe-commits mailing lis

[PATCH] D59919: [Attributor] Deduce "returned" argument attribute

jdoerfert updated this revision to Diff 207924. jdoerfert added a comment. Rebase (tests will be run later today) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59919/new/ https://reviews.llvm.org/D59919 Files: clang/test/CodeGenOpenCL/as_type.cl

[PATCH] D59919: [Attributor] Deduce "returned" argument attribute

jdoerfert added a comment. `ninja check-all` passed Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59919/new/ https://reviews.llvm.org/D59919 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http

[PATCH] D61467: [Rewrite] Extend to further accept CharSourceRange

jdoerfert added a comment. Could you describe the unit tests a little, what is tested and why. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61467/new/ https://reviews.llvm.org/D61467 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D63845: [WIP] Create a clang attribute that lets users specify LLVM attributes

jdoerfert added a comment. I think we should postpone a detailed discussion until we are ready to send an RFC on this. Nevertheless, I inlined some responses below. In D63845#1570639 , @aaron.ballman wrote: > In D63845#1566987

[PATCH] D61467: [Rewrite] Extend to further accept CharSourceRange

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. The comments help a lot. LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61467/new/ https://reviews.llvm.org/D61467 ___ cfe-co

[PATCH] D64375: [OpenMP][Docs] Provide implementation status details

jdoerfert created this revision. jdoerfert added reviewers: kkwli0, ABataev, RaviNarayanaswamy, gtbercea, Hahnfeld. Herald added subscribers: jfb, guansong, bollu. Herald added a project: clang. This adds a more fine-grained list of OpenMP features with their implementation status and associated

[PATCH] D64375: [OpenMP][Docs] Provide implementation status details

jdoerfert marked 2 inline comments as done. jdoerfert added a comment. In D64375#1574662 , @ABataev wrote: > The scheme itself looks good in general. Good. Once we agreed on a scheme we can improve the actual process, excel sheet, generator script, etc.

[PATCH] D59919: [Attributor] Deduce "returned" argument attribute

This revision was automatically updated to reflect the committed changes. Closed by commit rL365407: [Attributor] Deduce the "returned" argument attribute (authored by jdoerfert, committed by ). Changed prior to commit: https://reviews.llvm.org/D59919?vs=207924&id=208536#toc Repository: rL L

[PATCH] D64423: [OpenMP] Simplify getFloatTypeSemantics

jdoerfert added a comment. Could we generally require some message for all reviews and commits? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64423/new/ https://reviews.llvm.org/D64423 ___ cfe-commits mailing list cf

[PATCH] D59922: [Attributor] Deduce "no-capture" argument attribute

jdoerfert updated this revision to Diff 208811. jdoerfert added a comment. Herald added a subscriber: jfb. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59922/new/ https://reviews.llvm.org/D59922 Files: llvm/include/llvm/Transforms/IPO/At

[PATCH] D64375: [OpenMP][Docs] Provide implementation status details

jdoerfert added a comment. I will update the table (hopefully tomorrow) and we can then see if we commit it and change it in-place or if we have more initial feedback. Thanks everyone for providing all this information! @lebedev.ri I'd like to see if we can transition this one into a more gener

[PATCH] D64375: [OpenMP][Docs] Provide implementation status details

jdoerfert updated this revision to Diff 209073. jdoerfert marked 29 inline comments as done. jdoerfert added a comment. Add colors, updates according to comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64375/new/ https://reviews.llvm.org/D643

[PATCH] D64375: [OpenMP][Docs] Provide implementation status details

jdoerfert added inline comments. Comment at: clang/docs/OpenMPSupport.rst:163 ++--+--+--++ +| memory mangagement | allocate di

[PATCH] D64375: [OpenMP][Docs] Provide implementation status details

jdoerfert updated this revision to Diff 209089. jdoerfert marked 4 inline comments as done. jdoerfert added a comment. Fixes according to comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64375/new/ https://reviews.llvm.org/D64375 Files: cl

[PATCH] D61809: [BPF] Preserve debuginfo array/union/struct type/access index

jdoerfert reopened this revision. jdoerfert added a comment. This revision is now accepted and ready to land. The test coverage here is not acceptable: 1. The command line of the tests is far from what it should be (see other tests). 2. The check lines do little to prevent regressions in the fut

[PATCH] D61765: [OpenMP][Clang][BugFix] Split declares and math functions inclusion.

jdoerfert added a comment. This always includes the declare file but not the define file, correct? Could we have 4 tests that are compiled in target mode with: // with and without math.h/cmath (clang/clang++) #include long abs(long __i) { return (__i < 0 ? -i : i); } Repository: rC

[PATCH] D61765: [OpenMP][Clang][BugFix] Split declares and math functions inclusion.

jdoerfert added a comment. What about `abs` tests? Comment at: lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h:17 + #include + #include +#endif Why do we need the stdlib includes again? Comment at: test/Headers/Inputs/include/c

[PATCH] D61765: [OpenMP][Clang][BugFix] Split declares and math functions inclusion.

jdoerfert added inline comments. Comment at: lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h:17 + #include + #include +#endif gtbercea wrote: > jdoerfert wrote: > > Why do we need the stdlib includes again? > They are both prone to abs inclusion. >

[PATCH] D61765: [OpenMP][Clang][BugFix] Split declares and math functions inclusion.

jdoerfert added a comment. Last issue I have (in addition to the check @tra suggested) is the order in which we include math.h and cstdlib. can you flip it in one of the test cases? Comment at: lib/Headers/__clang_cuda_math_forward_declares.h:30-38 +#ifndef _OPENMP +__DEVICE__

[PATCH] D59919: [Attributor] Deduce "returned" argument attribute

jdoerfert updated this revision to Diff 199114. jdoerfert added a comment. Rebase on newest Attributor design Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59919/new/ https://reviews.llvm.org/D59919 Files: clang/test/CodeGenOpenCL/as_type.cl l

[PATCH] D61765: [OpenMP][Clang][BugFix] Split declares and math functions inclusion.

jdoerfert added a comment. Two small changes and then it is fine with me. @tra ? 1. we need to use ifdef to not define clock 2. Can you switch the include order in `test/Headers/nvptx_device_math_functions.cpp`? P.S. I'm currently at the OpenMP standard meeting to get the OpenMP variants fixed

[PATCH] D61765: [OpenMP][Clang][BugFix] Split declares and math functions inclusion.

jdoerfert added a comment. In D61765#1500351 , @gtbercea wrote: > - Exclude clock functions. Reverse inclusion order. LGTM from my side. I don't have strong feelings about testing libc++ now, though it is probably a good idea to have such a testbed. I a

[PATCH] D61949: [OpenMP][bugfix] Fix issues with C++ 17 compilation when handling math functions

jdoerfert added a comment. Missing `defined(__cplusplus)` to avoid warnings and we have to resolve the two new includes. Otherwise, this looks good. So, if you "fix" both go ahead and commit, if not, lets discuss. Comment at: lib/Headers/__clang_cuda_device_functions.h:1477

[PATCH] D61949: [OpenMP][bugfix] Fix issues with C++ 17 compilation when handling math functions

jdoerfert requested changes to this revision. jdoerfert added inline comments. This revision now requires changes to proceed. Comment at: lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h:24 + #include + #include #endif jdoerfert wrote: > I ask this

[PATCH] D62046: [OpenMP][bugfix] Add missing math functions variants for log and abs.

jdoerfert added inline comments. Comment at: lib/Headers/__clang_cuda_cmath.h:55-56 +#if defined(_OPENMP) && defined(__cplusplus) +__DEVICE__ const float abs(const float __x) { return ::fabsf((float)__x); } +__DEVICE__ const double abs(const double __x) { return ::fabs((double)__

[PATCH] D62046: [OpenMP][bugfix] Add missing math functions variants for log and abs.

jdoerfert added a comment. This LGTM me, @tra or @efriedma any objections? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62046/new/ https://reviews.llvm.org/D62046 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D55483: Introduce the callback attribute and emit !callback metadata

jdoerfert marked 2 inline comments as done. jdoerfert added inline comments. Comment at: include/clang/Basic/Builtins.def:942 LIBBUILTIN(alloca, "v*z", "f", "stdlib.h", ALL_GNU_LANGUAGES) +LIBBUILTIN(qsort_r, "", "fC<3,-1,-1,4>", "stdlib.h", ALL_GNU_LANGUA

[PATCH] D55483: Introduce the callback attribute and emit !callback metadata

jdoerfert updated this revision to Diff 180710. jdoerfert marked 3 inline comments as done. jdoerfert added a comment. Update encoding, rebase to HEAD Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55483/new/ https://reviews.llvm.org/D55483 Files: include/clang

[PATCH] D55483: Introduce the callback attribute and emit !callback metadata

jdoerfert updated this revision to Diff 180714. jdoerfert added a comment. Fix tests Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55483/new/ https://reviews.llvm.org/D55483 Files: include/clang/AST/ASTContext.h include/clang/Basic/Attr.td include/clang/Ba

[PATCH] D55483: Introduce the callback attribute and emit !callback metadata

jdoerfert updated this revision to Diff 180971. jdoerfert added a comment. Allow to use names in the callback metadata Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55483/new/ https://reviews.llvm.org/D55483 Files: include/clang/AST/ASTContext.h include/clan

[PATCH] D55483: Introduce the callback attribute and emit !callback metadata

jdoerfert marked 3 inline comments as done. jdoerfert added inline comments. Comment at: include/clang/Basic/Attr.td:1204 + VariadicUnsignedArgument<"PayloadIndices">]; + let Subjects = SubjectList<[Function]>; + let Documentation = [Undocumented];

[PATCH] D59980: [Attributor] Deduce memory behavior argument attributes

jdoerfert abandoned this revision. jdoerfert added a comment. Obsolete, D67384 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59980/new/ https://reviews.llvm.org/D59980 __

[PATCH] D60076: [Attributor] Deduce memory behavior function attributes

jdoerfert added a comment. Obsolete, D67384 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60076/new/ https://reviews.llvm.org/D60076 ___ cfe-commits mailing list cfe-

[PATCH] D67294: Register and parse a simplified version of '#pragma omp declare variant'

jdoerfert added a comment. I left some minor notes but overall I think we should put this in and improve on it in-tree. @ABataev, would that be OK with you? Comment at: lib/Parse/ParseOpenMP.cpp:748 } +/// Parses clauses for 'declare variant' directive. Rem

[PATCH] D68410: [AttrDocs] document always_inline

jdoerfert added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:4401 +disabled. Does not guarantee that inline substitution actually occurs. +}]; + let Heading = "always_inline"; It is more than that. This would imply that with optimizations e

[PATCH] D69498: IR: Invert convergent attribute handling

jdoerfert added a reviewer: JonChesterfield. jdoerfert added a comment. Before we get lost in review details let's make sure we agree that his is the right path. I mean, transition from `convergent` to `noconvergent` (or a similar spelling). I do support this by the way. In D69498#1723606

[PATCH] D69498: IR: Invert convergent attribute handling

jdoerfert added a comment. @rjmccall Thanks for the quick response! I tried to describe my view below. To avoid confusion on my side, could you maybe describe what you think when/which attribute should be created, derived, and used during the optimization pipeline? In D69498#1724232

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

jdoerfert added a comment. > Could you please list the other patches that are being held back by this one? > I'd be interested to have a look at them. :) We need the target type support for D80222 , D79739 can go in but we need

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

jdoerfert added a comment. In D79675#2045563 , @fghanim wrote: > In D79675#2045405 , @jdoerfert wrote: > > > > Could you please list the other patches that are being held back by this > > > one? I'd be interested t

[PATCH] D71739: [AssumeBundles] Use operand bundles to encode alignment assumptions

jdoerfert added a comment. I guess alignment is one of the main users of the old format, great to see it go :) First set of comments. Comment at: llvm/lib/Analysis/AssumeBundleQueries.cpp:104 +return 1; + }; if (BOI.End - BOI.Begin > ABA_Argument) I t

[PATCH] D79966: [OPENMP]Fix PR45911: Data sharing and lambda capture.

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79966/new/ https://reviews.llvm.org/D79966 _

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

jdoerfert added a comment. In D79744#2047482 , @arsenm wrote: > For the purpose here, only the callee exists. This is essentially a > freestanding function, the entry point to the program. There is no caller > function, and in the future I would like to

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

jdoerfert added a comment. We pass all clang tests, right? Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:203 + static Function *getOrCreateRuntimeFunction(Module &Md, + omp::RuntimeFunction FnID); N

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

jdoerfert added a comment. > libomp used to pass until I changed it to us getOrCreateRuntimeFunction I > think. Those tests are flaky, I think. Just run them a few times. I don't expect this to influence them at all. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

jdoerfert added a subscriber: fghanim. jdoerfert added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:244 -// TODO: Replace this with the real size_t type -#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME, Type::getInt64Ty(Ctx)) +#define __OMP_SIZE_TYPE(

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

jdoerfert added a comment. In D79675#2047154 , @fghanim wrote: > In D79675#2045826 , @jdoerfert wrote: > > > In D79675#2045563 , @fghanim wrote: > > > > > So this whole thing

[PATCH] D79754: [OpenMP][AMDGCN] Support OpenMP offloading for AMDGCN architecture - Part 1

jdoerfert added a comment. I'm generally fine with this, don't wait for my approval. Comment at: clang/test/OpenMP/amdgcn_device_function_call.cpp:27 + } +} Not for this patch: FWIW, we will need to make math functions work inside target regions. The way aomp

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

jdoerfert added a comment. In D80222#2055149 , @jhuber6 wrote: > This passes all checks except the libomp ones. Those still seg-fault, I'm not > entirely sure why. Cuda failure is again because I don't have it configured > on my machine. > > Failing Test

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

jdoerfert added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:250 +__OMP_SIZE_TYPE(SizeTy) +#undef __OMP_SIZE_TYPE + Why the indirection via `__OMP_SIZE_TYPE`? Wouldn't `OMP_TYPE(SizeTy, M.getDataLayout().getIntPtrType(Ctx))` suffic

[PATCH] D80590: [WIP][OPENMP] Fix assertion error for using alignas with OpenMP directive

jdoerfert added a comment. > We can avoid the assertion failure by either removing alignas or OpenMP > directive in the above code. What OpenMP directive? > Haven't added test yet since I'm not sure in which file should I add the test. Let's go with something like `clang/test/OpenMP/sema_align

[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

jdoerfert added a comment. In D74387#2053742 , @Fznamznon wrote: > Re-implemented diagnostic itself, now only usages of declarations > with unsupported types are diagnosed. > Generalized approach between OpenMP and SYCL. Great, thanks a lot! In D74387

[PATCH] D80439: Replace separator in OpenMP variant name mangling.

jdoerfert added a comment. Changes looks good to me. We don't have tests that check the mangling, interesting (and my fault), should we add one? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80439/new/ https://reviews.llvm.org/D80439 _

[PATCH] D80404: [OPENMP50]Initial support for use_device_addr clause.

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM, one nit below Comment at: clang/lib/Sema/SemaOpenMP.cpp:10179 Diag(StartLoc, diag::err_omp_no_clause_for_directive) -<< "'map' or 'use_device_ptr'" -

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

jdoerfert added a comment. In D79675#2051682 , @fghanim wrote: > > My goal is to save us time, during development, review, maintenance, and > > future extensions. I hope you know that. > > I am certain of that. However, I am starting to have doubts if my

[PATCH] D71739: [AssumeBundles] Use operand bundles to encode alignment assumptions

jdoerfert added a comment. I think the code looks good, we should make the test changes clearer, see below. Comment at: clang/test/CodeGen/align_value.cpp:7 double & z __attribute__((align_value(128 { }; -// CHECK: define void @_Z3fooPdS_Rd(double* align 64 %x, do

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

jdoerfert added inline comments. Comment at: openmp/runtime/test/tasking/kmp_taskloop.c:100 th_counter[i] = 0; -#pragma omp parallel num_threads(N) +#pragma omp parallel // num_threads(N) { jhuber6 wrote: > AndreyChurbanov wrote: > > jhuber6

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

jdoerfert added inline comments. Comment at: openmp/runtime/test/tasking/kmp_taskloop.c:100 th_counter[i] = 0; -#pragma omp parallel num_threads(N) +#pragma omp parallel // num_threads(N) { jhuber6 wrote: > jdoerfert wrote: > > jhuber6 wrote

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

jdoerfert added inline comments. Comment at: openmp/runtime/test/tasking/kmp_taskloop.c:100 th_counter[i] = 0; -#pragma omp parallel num_threads(N) +#pragma omp parallel // num_threads(N) { jhuber6 wrote: > jdoerfert wrote: > > jhuber6 wrote

[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. > This change even doesn't break my local check-clang LIT tests run, but I'm > not really sure that such change is in scope of this patch, because > DiagnoseUseOfDecl contains a lot of o

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

jdoerfert added a comment. Take a look at D80735 , it works fine for me locally. Is that what you did? What problems do you observe now? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80222/new/ https://reviews.llvm

[PATCH] D80735: [OpenMP][NFC] Reuse `llvm::omp::types::IdentPtr` in clang

jdoerfert created this revision. jdoerfert added reviewers: jhuber6, fghanim. Herald added subscribers: sstefan1, guansong, bollu, yaxunl. Herald added a project: clang. We should replace the uses of `QualType IdentQTy` with DataLayout queries encapsulated in a helper (inside `llvm::omp::types`)

[PATCH] D77918: [OpenMP] Avoid crash in preparation for diagnose of unsupported type

jdoerfert planned changes to this revision. jdoerfert added a comment. Herald added a subscriber: sstefan1. I think this will be fixed by D74387 , we should include the tests somewhere though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D77918: [OpenMP] Avoid crash in preparation for diagnose of unsupported type

jdoerfert abandoned this revision. jdoerfert added a comment. Great. I'll ask in the other patch to add these three lines just to be sure ;) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77918/new/ https://reviews.llvm.org/D77918 ___

[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

jdoerfert added a comment. Can you include these: long double qa, qb; decltype(qa + qb) qc; double qd[sizeof(-(-(qc * 2)))]; Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74387/new/ https://reviews.llvm.org/D74387

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

jdoerfert added a comment. This passes all the tests? I think we should go with it and investigate the cast thing later. Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:124 + return {FnTy, Fn}; +} } Are you sure we need to do the cast here? I

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

jdoerfert added a comment. I figured out what the problem is and we can relatively easily fix it *once* we always have an OpenMPIRBuilder available in Clangs CG. As noted in D80735 , we should remove `IdentQTy` as it is used to create a new ident_t in some situa

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM. Thanks for taking this one, it was more complex than I thought but it is a really nice step in the right direction. I'll commit it for you soon if you don't have access yet. Feel f

[PATCH] D80240: [OPENMP50]Initial codegen for 'affinity' clauses.

jdoerfert added a comment. Few comments, the alloca question is my only real concern. Parts of this are trivial NFC and can go in to make the patch smaller. Parts won't be needed after D80222 landed :) Comment at: clang/lib/CodeGen/CGOpenMPRu

[PATCH] D80240: [OPENMP50]Initial codegen for 'affinity' clauses.

jdoerfert accepted this revision. jdoerfert added subscribers: jhuber6, clementval, fghanim, DavidTruby. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM. Let's wait until D80222 landed :) Comment at: clang/lib/C

[PATCH] D80735: [OpenMP][NFC] Reuse OMPIRBuilder `struct ident_t` handling in Clang

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:376 + /// generation. + llvm::OpenMPIRBuilder InternalOMPBuilder; jhuber6 wrote: > Should D80222 be changed to use this instead and re

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

jdoerfert added a comment. Initialize the internal OpenMPIRbuilder (see my patch). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80222/new/ https://reviews.llvm.org/D80222 ___ cfe-commits mailing list

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

jdoerfert added a comment. Oh, sorry. You merged all of my patch, right? It was not ready and even now it is unclear if the representation change in my patch is good or not. I was trying to suggest you only take the InternalOMPIRBuilder stuff so you can avoid the static function, not all of my

[PATCH] D80897: [OpenMP] Initial support for std::complex in target regions

jdoerfert created this revision. jdoerfert added reviewers: tra, hfinkel, ABataev, JonChesterfield. Herald added subscribers: sstefan1, guansong, bollu, yaxunl, mgorny. Herald added a project: clang. This simply follows the scheme we have for other wrappers. It resolves the current link problem, e

[PATCH] D80897: [OpenMP] Initial support for std::complex in target regions

jdoerfert updated this revision to Diff 267531. jdoerfert added a comment. Fix tests, add C support Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80897/new/ https://reviews.llvm.org/D80897 Files: clang/lib/Headers/CMakeLists.txt clang/lib/Head

[PATCH] D80584: [utils] change update_test_checks.py use of 'TMP' value names

jdoerfert added a comment. First, I think the warning is strictly a good thing. Thanks for keeping that in! I don't think the options are really "complexity to the user" given a sensible default (= the old value). If a user sees a warning that says: `Name conflict, use --nameless-prefix=X,

[PATCH] D80584: [utils] change update_test_checks.py use of 'TMP' value names

jdoerfert added a comment. > I think we're good enough. I agree. If this doesn't bother anyone anymore there is no need for any options ;) Thanks for the test! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80584/new/ https://reviews.llvm.org/D80

[PATCH] D80917: [OpenMP][AMDGCN] Support OpenMP offloading for AMDGCN architecture - Part 2

jdoerfert added inline comments. Comment at: clang/include/clang/Basic/OpenMPGridValues.h:134 +} // namespace clang +#endif Can we move this into llvm core (subfolder `Frontend/OpenMP/`)? We probably need this during OpenMPOpt, and for Flang as well. Repositor

[PATCH] D80897: [OpenMP] Initial support for std::complex in target regions

jdoerfert marked 2 inline comments as done. jdoerfert added a comment. In D80897#2066723 , @tra wrote: > Hmm. I'm pretty sure tensorflow is using std::complex for various types. I'm > surprised that we haven't seen these functions missing. Which functio

[PATCH] D80947: Add to the Coding Standard our that single-line bodies omit braces

jdoerfert added a comment. Thanks for finally writing this down :) Two minor comments. Comment at: llvm/docs/CodingStandards.rst:1580 +statement is accompanied by a comment that loses its meaning if hoisted above the if +or loop statement, or where the single statement is comp

[PATCH] D80439: Replace separator in OpenMP variant name mangling.

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM. Thanks a lot! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80439/new/ https://reviews.llvm.org/D80439 ___ cfe-commits maili

[PATCH] D81037: [OPENMP]Fix PR46170: partial mapping for array sections of data members.

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM. Thanks for the quick fix! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81037/new/ https://reviews.llvm.org/D81037 __

[PATCH] D80897: [OpenMP] Initial support for std::complex in target regions

jdoerfert marked an inline comment as done. jdoerfert added a comment. I tried to determine why we don't emit such calls for c++11 and stdc++ but I was not successful :( Tracking back from the emission lead to the generic expression codegen without any (obvious) check of the runtime library or s

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

jdoerfert added a comment. In D79744#2069786 , @arsenm wrote: > In D79744#2069774 , @arsenm wrote: > > > I think this is converging to adding a new IR attribute that essentially > > just provides the pointee type f

[PATCH] D81031: [OpenMP] Add Additional Function Attribute Information to OMPKinds.def

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. Apologies for the wait. I think with the comments below this is good to go. If you have questions or concerns, let me know. Thanks for these changes! Comment at: llvm/

<    1   2   3   4   5   6   7   8   9   10   >