[PATCH] D106809: [clang-offload-bundler] Make Bundle Entry ID backward compatible

grokos accepted this revision. grokos added a comment. LGTM as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106809/new/ https://reviews.llvm.org/D106809 ___ cfe-commits mailing list

[PATCH] D106809: [clang-offload-bundler] Make Bundle Entry ID backward compatible

grokos added a comment. LG. One possible suggestion is that you leave the double dash (`--`) variant in some tests so that we can make sure both variants (e.g. both `openmp-amdgcn-amd-amdhsa--gfx906` and `openmp-amdgcn-amd-amdhsa-gfx906`) are correctly parsed. Repository: rG LLVM Github

[PATCH] D106509: [OpenMP][OpenACC] Implement `ompx_hold` map type modifier extension in Clang (1/2)

grokos added a comment. In D106509#2943239 , @protze.joachim wrote: > I was wondering about the connection to OpenACC, so I had a quick look into > the OpenACC spec to try and understand the background. > OpenACC uses two separate reference counters

[PATCH] D105191: [Clang][OpenMP] Add support for Static Device Libraries

grokos added inline comments. Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1689 + : "lib" + libname + "-" + archname + "-" + gpuname, + "a"); + "a" --> ".a" (add a dot) Comment at:

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

grokos added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:147 +Target.split(Components, '-', 5); +Components.resize(6); +this->OffloadKind = Components[0]; saiislam wrote: > grokos wrote: > > Leftover?

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

grokos added inline comments. Comment at: clang/docs/ClangOffloadBundler.rst:128 + + --- + A bit of wordplay, but it's weird that a *triple* now has 4 elements... Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:147 +

[PATCH] D99551: [clang-offload-wrapper] Add standard notes for ELF offload images

grokos accepted this revision. grokos added a comment. This revision is now accepted and ready to land. Change looks good, so it's accepted on my end. I'll let the other reviewers have a look and post their comments. Please do not commit until we have reached an agreement for all 4 patches

[PATCH] D92195: [OPENMP50]Mapping of the subcomponents with the 'default' mappers.

grokos accepted this revision. grokos added a comment. This revision is now accepted and ready to land. Libomptarget changes look good, I'll let @jdoerfert provide feedback for the clang part. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D97003: [Clang][OpenMP] Require CUDA 9+ for OpenMP offloading on NVPTX target

grokos added a comment. This change makes much sense. In fact, CUDA 8 was so problematic for use with the nvptx runtime that (if memory serves me well) we declared it unsupported. So essentially this patch drops support for CUDA version 7 (and lower), which is already six years old. If the

[PATCH] D86119: [OPENMP50]Allow overlapping mapping in target constrcuts.

grokos added a comment. In D86119#2561163 , @abhinavgaba wrote: > Thanks for the changes, Alexey! I tried the patch locally, and it looks > stable. It handled several tests I tried, including the following case > involving array section on a pointer to

[PATCH] D86119: [OPENMP50]Allow overlapping mapping in target constrcuts.

grokos added inline comments. Comment at: openmp/libomptarget/src/omptarget.cpp:233 MapperComponents -.Components[target_data_function == targetDataEnd ? I : E - I - 1]; +.Components[target_data_function == targetDataEnd ? E - I - 1 : I];

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

grokos added a comment. In D87946#2325756 , @jhuber6 wrote: > Current build, fails `offloading/target_depend_nowait` for an unknown reason > after calling cuStreamSynchronize in __tgt_target_teams_mapper_nowait. Is your tree up to date? We had a problem

[PATCH] D88829: [OpenMP][RTL] Remove dead code

grokos added a comment. In D88829#2311768 , @JonChesterfield wrote: > Rolling the reduction in leading whitespace in > nvptx_target_parallel_reduction_codegen.cpp in with the patch might be > contentious, added a couple more reviewers to see if other

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

grokos added inline comments. Comment at: openmp/libomptarget/include/Ident.h:48-51 +auto removePath = [](const std::string ) { +std::size_t pos = path.rfind('/'); +return path.substr(pos + 1); +}; jhuber6 wrote: > This will probably

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

grokos added a comment. In D87946#2287744 , @jhuber6 wrote: > Seems like a hacky solution to just keep adding suffixed whenever we want a > new interface though. Yes, this used to be a point of contention within the community. We discussed the issue

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

grokos added a comment. In D87946#2286413 , @jhuber6 wrote: > I wasn't aware they were explicitly deprecated. If we're keeping around old > interfaces for backwards compatibility I should also add in the old mapper > functions without the `ident_t`

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

grokos added a comment. In D87946#2286024 , @jhuber6 wrote: > Added ident_t structs to additional runtime functions. Why are we adding the extra parameter to those additional functions? Non-mapper API functions have been deprecated, clang does not emit

[PATCH] D84767: [OPENMP]Fix PR46824: Global declare target pointer cannot be accessed in target region.

grokos added inline comments. Comment at: clang/test/OpenMP/target_data_codegen.cpp:659-660 // PRESENT=0x1000 | TARGET_PARAM=0x20 = 0x1020 - // MEMBER_OF_9=0x9 | PRESENT=0x1000 | FROM=0x2 | TO=0x1 = 0x91003 - // MEMBER_OF_9=0x9 |

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

grokos added a comment. This looks much better now. I don't have any other comments. Since this patch is now essentially a clang-only patch, I'll let @ABataev accept it or post comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84422/new/ https://reviews.llvm.org/D84422

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

grokos added a comment. In D84422#2173500 , @jdenny wrote: > I've added a comment to the runtime code that performs the check. As you can > see, the check is performed regardless. It's just a question of whether the > runtime treats it as an error. I

[PATCH] D84182: [OPENMP]Fix PR46012: declare target pointer cannot be accessed in target region.

grokos added a comment. @ABataev: After this patch was committed, I tried to run the following example: #include int *yptr; int main() { int y[10]; y[1] = 1; yptr = [0]; printf(" = %p\n", ); printf("[0] = %p\n", [0]); #pragma omp target data map(to:

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

grokos added a comment. So let's proceed with the patch. Instead of introducing new API functions and making all these changes in all these files, wouldn't it be easier if we just unset the `PRESENT` flag from arg_types in clang when we generate the call to `__tgt_target_data_end_*` if we are

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

grokos added a comment. So is the test case that motivated this patch illegal OpenMP code? #pragma omp target enter data map(alloc:i) #pragma omp target data map(present, alloc: i) { #pragma omp target exit data map(delete:i) } // fails presence check here Repository: rG LLVM

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

grokos added a comment. What confuses me about this interpretation of the standard is the inconsistency at `data exit`. So if we have an explicit `omp target exit data map(present...)` then we should respect the "present" semantics, whereas when we have a scoped data exit: #pragma omp

[PATCH] D83959: Fix compiling warnings in OpenMP declare mapper codegen

This revision was automatically updated to reflect the committed changes. Closed by commit rGfc47c0e0a6a2: [clang] Fix compilation warnings in OpenMP declare mapper codegen. (authored by grokos). Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github

[PATCH] D67833: [OpenMP 5.0] Codegen support to pass user-defined mapper functions to runtime

This revision was automatically updated to reflect the committed changes. Closed by commit rG537b16e9b8da: [OpenMP 5.0] Codegen support to pass user-defined mapper functions to runtime (authored by grokos). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D67833: [OpenMP 5.0] Codegen support to pass user-defined mapper functions to runtime

grokos added a comment. OK, now it works. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67833/new/ https://reviews.llvm.org/D67833 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D67833: [OpenMP 5.0] Codegen support to pass user-defined mapper functions to runtime

grokos added a comment. I tried to build clang with this patch and I get errors like: CGOpenMPRuntime.cpp:9463:38: error: ‘OMPRTL___tgt_target_teams_nowait_mapper’ was not declared in this scope ? OMPRTL___tgt_target_teams_nowait_mapper Where are these

[PATCH] D83057: [OpenMP][NFC] Remove hard-coded line numbers from more tests

grokos accepted this revision. grokos added a comment. This revision is now accepted and ready to land. Like D82224 , looks good. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83057/new/

[PATCH] D74262: [clang-offload-bundler] Enable handling of partially-linked fat objects

grokos abandoned this revision. grokos added a comment. The partial linking scheme has been found to not work correctly in all cases (it fails when we have libraries with device code only). A new patch will be uploaded which will be based on archive extraction. Repository: rG LLVM Github

[PATCH] D75223: [clang-offload-wrapper] Lower priority of __tgt_register_lib in favor of __tgt_register_requires

This revision was automatically updated to reflect the committed changes. Closed by commit rGfca49fe8e34f: [clang-offload-wrapper] Lower priority of __tgt_register_lib in favor of… (authored by grokos). Changed prior to commit: https://reviews.llvm.org/D75223?vs=246870=247991#toc Repository:

[PATCH] D75223: [clang-offload-wrapper] Lower priority of __tgt_register_lib in favor of __tgt_register_requires

grokos added a comment. @ABataev, @jdoerfert, does the patch look good to you? Can someone accept it if it's ready to go? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75223/new/ https://reviews.llvm.org/D75223

[PATCH] D75223: [clang-offload-wrapper] Lower priority of __tgt_register_lib in favor of __tgt_register_requires

grokos updated this revision to Diff 246870. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75223/new/ https://reviews.llvm.org/D75223 Files: clang/test/Driver/clang-offload-wrapper.c clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp

[PATCH] D75223: [clang-offload-wrapper] Lower priority of __tgt_register_lib in favor of __tgt_register_requires

grokos created this revision. grokos added reviewers: ABataev, vzakhari. grokos added projects: OpenMP, clang. Herald added a reviewer: jdoerfert. Currently, the offload-wrapper tool inserts `__tgt_register_lib` to the list of global ctors of a target module with `Priority=0`. This means that

[PATCH] D74262: [clang-offload-bundler] Enable handling of partially-linked fat objects

grokos marked 2 inline comments as done. grokos added a comment. In D74262#1867245 , @ABataev wrote: > Partial linking may lead to some incorrect results with global constructors. > How are you going to handle this? Can you give me an example of what

[PATCH] D74262: [clang-offload-bundler] Enable handling of partially-linked fat objects

grokos created this revision. grokos added reviewers: hfinkel, jdoerfert, ABataev, mdtoguchi, kbobrovs, sdmitriev. grokos added a project: clang. Herald added a subscriber: Anastasia. This is the bundler-side patch for enabling static library support in clang. The scheme has been discussed

[PATCH] D68746: [Clang][OpenMP Offload] Move offload registration code to the wrapper

grokos added inline comments. Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:74 + IntegerType *getSizeTTy() { +switch (M.getDataLayout().getPointerTypeSize(Type::getInt8PtrTy(C))) { +case 4u: sdmitriev wrote: > ABataev wrote: > >

[PATCH] D38798: [OpenMP] Support for implicit "declare target" functions - Sema patch

grokos abandoned this revision. grokos added a comment. @ABataev came up with a much simpler solution to the implementation of `declare target`: https://reviews.llvm.org/rL327636 I am abandoning this obsolete revision. Repository: rL LLVM https://reviews.llvm.org/D38798

[PATCH] D43026: [OpenMP] CodeGen for the "declare target" directive - variables, functions, ctors/dtors

grokos abandoned this revision. grokos added a comment. @ABataev came up with a much simpler solution to the implementation of `declare target`: https://reviews.llvm.org/rL327636 I am abandoning this obsolete revision. Repository: rC Clang https://reviews.llvm.org/D43026

[PATCH] D43197: [OpenMP] Add flag for linking runtime bitcode library

grokos added inline comments. Comment at: test/Driver/openmp-offload-gpu.c:150 +/// bitcode library and add it to the LIBRARY_PATH. +// RUN: touch %T/libomptarget-nvptx-sm_60.bc +// RUN: env LIBRARY_PATH=%T %clang -### -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda \

[PATCH] D43197: [OpenMP] Add flag for linking runtime bitcode library

grokos accepted this revision. grokos added a comment. This revision is now accepted and ready to land. I don't have any other remarks, looks good. Repository: rC Clang https://reviews.llvm.org/D43197 ___ cfe-commits mailing list

[PATCH] D43197: [OpenMP] Add flag for linking runtime bitcode library

grokos added inline comments. Comment at: lib/Driver/ToolChains/Cuda.cpp:559 +if (!FoundBCLibrary) + getDriver().Diag(diag::remark_drv_omp_offload_target_missingbcruntime); + } Should we be more specific when it comes to the name of the missing bc file

[PATCH] D43026: [OpenMP] Support for implicit "declare target" functions - CodeGen patch

grokos created this revision. grokos added a reviewer: ABataev. grokos added projects: clang, OpenMP. Herald added a subscriber: guansong. This patch implements CodeGen support for the "declare target" directive. Code is generated for variables, functions and ctors/dtors. I understand that the

[PATCH] D40977: [OpenMP] NVPTX: Set default/minimum compute capability to sm_35

This revision was automatically updated to reflect the committed changes. Closed by commit rC320082: [OpenMP] NVPTX: Set default/minimum compute capability to sm_35 (authored by grokos). Repository: rC Clang https://reviews.llvm.org/D40977 Files: CMakeLists.txt Index: CMakeLists.txt

[PATCH] D40977: [OpenMP] NVPTX: Set default/minimum compute capability to sm_35

grokos created this revision. grokos added a project: OpenMP. Herald added a subscriber: mgorny. The current implementation of the nvptx runtime (to be upstreamed shortly) uses the `atomicMax` operation on 64-bit integers. This is only supported in compute capabilities 3.5 and later. I've

[PATCH] D39745: Clang/libomptarget map interface flag renaming - NFC patch

This revision was automatically updated to reflect the committed changes. Closed by commit rL317598: Clang/libomptarget map interface flag renaming - NFC patch (authored by grokos). Changed prior to commit: https://reviews.llvm.org/D39745?vs=121928=121931#toc Repository: rL LLVM

[PATCH] D38968: [OpenMP] Implement omp_is_initial_device() as builtin

grokos added a comment. Now that this issue has been addressed and regressions tests pass, should we re-enable Cmake to build libomptarget by default? https://reviews.llvm.org/D38968 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D38798: [OpenMP] Support for implicit "declare target" functions - Sema patch

grokos created this revision. grokos added a project: clang. This patch completes the support for the "declare target" directive in Sema. With this patch Sema handles implicitly used functions (i.e. functions which are used inside a target region without having been "declared target")

[PATCH] D33509: [OpenMP] Create COMDAT group for OpenMP offload registration code to avoid multiple copies

grokos added a comment. I committed the patch. Thanks for submitting it! Repository: rL LLVM https://reviews.llvm.org/D33509 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33509: [OpenMP] Create COMDAT group for OpenMP offload registration code to avoid multiple copies

This revision was automatically updated to reflect the committed changes. Closed by commit rL304056: [OpenMP] Create COMDAT group for OpenMP offload registration code to avoid… (authored by grokos). Changed prior to commit: https://reviews.llvm.org/D33509?vs=100134=100518#toc Repository: rL