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

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

Starting with the commit of this patch, the libomptarget test 
`llvm-project/openmp/libomptarget/test/offloading/target_depend_nowait.cpp` 
fails. Here is a stacktrace of the segfault:

  $ gdb target_depend_nowait.cpp.tmp-x86_64-pc-linux-gnu
  (gdb) run
  ...
  Program received signal SIGSEGV, Segmentation fault.
  (gdb) bt
  #0  0x00400fcf in .omp_outlined._debug__ 
(.global_tid.=0x2aaab96b9be0, .bound_tid.=0x2aaab96b9bd8) at 
target_depend_nowait.cpp:22
  #1  0x00401e8d in .omp_outlined..23 (.global_tid.=0x2aaab96b9be0, 
.bound_tid.=0x2aaab96b9bd8) at target_depend_nowait.cpp:18
  #2  0x2b574213 in __kmp_invoke_microtask () from libomp.so
  #3  0x2b51338e in __kmp_invoke_task_func () from libomp.so
  #4  0x2b5126bf in __kmp_launch_thread () from libomp.so
  #5  0x2b55d3d0 in __kmp_launch_worker(void*) () from libomp.so
  #6  0x2bbd6e65 in start_thread () from libpthread.so.0
  #7  0x2bee988d in clone () from libc.so.6


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

I'm executing:

  $ /usr/bin/python bin/llvm-lit -vv -a 
projects/openmp/libomptarget/test/offloading/target_depend_nowait.cpp

which executes:

  "/dev/shm/jprotze/build-tsan-fiber-as/./bin/clang++" "-fopenmp" "-pthread" 
"-fno-experimental-isel" "-I" 
"/dev/shm/jprotze/llvm-project/openmp/libomptarget/test" "-I" 
"/dev/shm/jprotze/build-tsan-fiber-as/projects/openmp/libomptarget/../runtime/src"
 "-L" "/dev/shm/jprotze/build-tsan-fiber-as/lib" 
"-fopenmp-targets=x86_64-pc-linux-gnu" 
"/dev/shm/jprotze/llvm-project/openmp/libomptarget/test/offloading/target_depend_nowait.cpp"
 "-o" 
"/dev/shm/jprotze/build-tsan-fiber-as/projects/openmp/libomptarget/test/offloading/Output/target_depend_nowait.cpp.tmp-x86_64-pc-linux-gnu"

According to gdb, the segfault happens in  0x400fcf. %rcx is 0 at that time.

   400f89:   e8 62 fc ff ff  callq  400bf0 
<__kmpc_omp_target_task_alloc@plt>
400f8e:   45 31 c9xor%r9d,%r9d
400f91:   31 c9   xor%ecx,%ecx
400f93:   48 8d 55 88 lea-0x78(%rbp),%rdx
400f97:   48 8b 75 e0 mov-0x20(%rbp),%rsi
400f9b:   48 89 70 28 mov%rsi,0x28(%rax)
400f9f:   48 8b 75 e8 mov-0x18(%rbp),%rsi
400fa3:   48 89 70 30 mov%rsi,0x30(%rax)
400fa7:   48 8b 75 d0 mov-0x30(%rbp),%rsi
400fab:   48 89 70 38 mov%rsi,0x38(%rax)
400faf:   48 8b 75 d8 mov-0x28(%rbp),%rsi
400fb3:   48 89 70 40 mov%rsi,0x40(%rax)
400fb7:   48 8b 34 25 40 1f 40mov0x401f40,%rsi
400fbe:   00 
400fbf:   48 89 70 48 mov%rsi,0x48(%rax)
400fc3:   48 8b 34 25 48 1f 40mov0x401f48,%rsi
400fca:   00 
400fcb:   48 89 70 50 mov%rsi,0x50(%rax)
  > 400fcf:   48 8b 31mov(%rcx),%rsi
400fd2:   48 89 70 58 mov%rsi,0x58(%rax)
400fd6:   48 8b 49 08 mov0x8(%rcx),%rcx
400fda:   48 89 48 60 mov%rcx,0x60(%rax)
400fde:   48 b9 00 61 60 00 00movabs $0x606100,%rcx
400fe5:   00 00 00 
400fe8:   48 89 4d 88 mov%rcx,-0x78(%rbp)
400fec:   48 c7 45 90 04 00 00movq   $0x4,-0x70(%rbp)
400ff3:   00 
400ff4:   c6 45 98 03 movb   $0x3,-0x68(%rbp)
400ff8:   48 c7 45 80 01 00 00movq   $0x1,-0x80(%rbp)
400fff:   00 
401000:   48 8b 4d f8 mov-0x8(%rbp),%rcx
401004:   8b 31   mov(%rcx),%esi
401006:   48 b9 97 1f 40 00 00movabs $0x401f97,%rcx
40100d:   00 00 00 
401010:   48 89 4d b0 mov%rcx,-0x50(%rbp)
401014:   48 8d 7d a0 lea-0x60(%rbp),%rdi
401018:   48 89 95 18 fe ff ffmov%rdx,-0x1e8(%rbp)
40101f:   48 89 c2mov%rax,%rdx
401022:   b9 01 00 00 00  mov$0x1,%ecx
401027:   4c 8b 85 18 fe ff ffmov-0x1e8(%rbp),%r8
40102e:   48 c7 04 24 00 00 00movq   $0x0,(%rsp)
401035:   00 
401036:   e8 55 fb ff ff  callq  400b90 
<__kmpc_omp_task_with_deps@plt>


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

This patch breaks compilation of previously working code.

I added the following to 
`openmp/libomptarget/test/offloading/offloading_success.c`:

  +// RUN: %libomptarget-compile-run-and-check-nvptx64-nvidia-cuda

which results in

  # command stderr:
  ptxas offloading_success-openmp-nvptx64-nvidia-cuda.s, line 173; error   : 
Call has wrong number of parameters
  ptxas fatal   : Ptx assembly aborted due to errors
  clang-12: error: ptxas command failed with exit code 255 (use -v to see 
invocation)

The file `offloading_success-openmp-nvptx64-nvidia-cuda.s` contains:

  .func  (.param .b32 func_retval0) __kmpc_kernel_parallel
  (
  .param .b64 __kmpc_kernel_parallel_param_0,
  .param .b32 __kmpc_kernel_parallel_param_1
  )
  ;

For the clang 11 release, we should either fix the codegen or revert this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim reopened this revision.
protze.joachim added a comment.
This revision is now accepted and ready to land.

I carefully made sure, that the freshly built clang was used to execute the 
test. I opened https://bugs.llvm.org/show_bug.cgi?id=46836 to track the issue 
and made it release blocker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D85934: Enable OpenMP offloading to VE and enable tests for offloading to VE

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added inline comments.



Comment at: openmp/libomptarget/test/lit.cfg:67
 append_dynamic_library_path('DYLD_LIBRARY_PATH', \
 config.omp_host_rtl_directory, ";")
 config.test_flags += " -Wl,-rpath," + config.library_dir

Unrelated to this patch, but why are different separators used for Darwin?



Comment at: openmp/libomptarget/test/lit.cfg:78-83
+if 've-unknown-linux-unknown' in config.libomptarget_system_targets and \
+config.omp_host_rtl_directory:
+config.test_flags = config.test_flags + " -Xopenmp-target \"-L" +\
+config.omp_host_rtl_directory + "\""
+append_dynamic_library_path('VE_LD_LIBRARY_PATH',
+config.omp_host_rtl_directory, ":")

I think, this should only be set for the specific device RUN-lines. I suggest 
to include the flags and export of the environment variable in the definition 
of `%libomptarget-compile-ve-unknown-linux-unknown`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85934

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

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

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added inline comments.



Comment at: clang/docs/OpenMPSupport.rst:165
++--+--+--++
+| OMPD | OMPD interfaces   
   | mostly done  ||
++--+--+--++

Hahnfeld wrote:
> jdoerfert wrote:
> > Hahnfeld wrote:
> > > This is not correct, at least it's not yet upstream.
> > Is there anything upstreamed? What should I put for status and 
> > revisions/reviews?
> I don't think there's a review yet. @protze.joachim ?
There is no review yet. Implementation is available in 
https://github.com/OpenMPToolsInterface/LLVM-openmp/tree/ompd-tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64375



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

[PATCH] D65880: [Driver] Move LIBRARY_PATH before user inputs

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

I still see some clang-specific and system link directories listed in the 
linker line before the directories from `LIBRARY_PATH`:

  $ LIBRARY_PATH=test1 /usr/local/clang/bin/clang -Ltest2 -v hello.c
   "/usr/bin/ld" .../crtbegin.o -Ltest2 
-L/usr/lib/gcc/x86_64-redhat-linux/4.8.5 
-L/usr/lib/gcc/x86_64-redhat-linux/4.8.5/../../../../lib64 -L/lib/../lib64 
-L/usr/lib/../lib64 -L/usr/lib/gcc/x86_64-redhat-linux/4.8.5/../../.. 
-L/usr/local/clang/bin/../lib -L/lib -L/usr/lib -Ltest1

I think they are inserted by `ToolChain.AddFilePathLibArgs` in Gnu.cpp. Is this 
the intended ordering? My expectation would be

  $ LIBRARY_PATH=test1 /usr/local/clang/bin/clang -Ltest2 -v hello.c
   "/usr/bin/ld" .../crtbegin.o -Ltest2 -Ltest1 
-L/usr/lib/gcc/x86_64-redhat-linux/4.8.5 
-L/usr/lib/gcc/x86_64-redhat-linux/4.8.5/../../../../lib64 -L/lib/../lib64 
-L/usr/lib/../lib64 -L/usr/lib/gcc/x86_64-redhat-linux/4.8.5/../../.. 
-L/usr/local/clang/bin/../lib -L/lib -L/usr/lib

@hfinkel any opinion?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65880

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

[PATCH] D97085: [OpenMP] libomp: implement OpenMP 5.1 inoutset task dependence type

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added inline comments.



Comment at: openmp/runtime/src/kmp_taskdeps.cpp:344-346
+// link node as successor of all nodes in the prev_set if any
+npredecessors +=
+__kmp_depnode_link_successor(gtid, thread, task, node, prev_set);

AndreyChurbanov wrote:
> protze.joachim wrote:
> > If I understand this code right, this has O(n^2) complexity for two sets of 
> > size n?
> > 
> > Consider:
> > ```
> > for (int i=0; i > #pragma omp task depend(in:A)
> > work(A,i);
> > }
> > for (int i=0; i > #pragma omp task depend(inoutset:A)
> > work(A,i);
> > }
> > ```
> > All n tasks in the first loop would be predecessor for each of the tasks in 
> > the second loop. This will also result in n^2 releases/notifications.
> > 
> > 
> > I'd suggest to model the dependencies like:
> > ```
> > for (int i=0; i > #pragma omp task depend(in:A)
> > work(A,i);
> > }
> > #pragma omp taskwait depend(inout:A) nowait
> > for (int i=0; i > #pragma omp task depend(inoutset:A)
> > work(A,i);
> > }
> > ```
> > I.e., add a dummy dependency node between the two sets, which has n 
> > predecessors and n successors. You probably understand the depnode code 
> > better, than I do, but I think you would only need to add some code in line 
> > 357, where `last_set` is moved to `prev_set`.
> > This dummy node would reduce linking and releasing complexity to O(n).
> This could be a separate research project.  Because such change may cause 
> performance regressions on some real codes, so it needs thorough 
> investigation.  I mean insertion of an auxiliary dummy node into dependency 
> graph is definitely an overhead, which may or may not be overridden by 
> reduction in number of edges in the graph.  Or it can be made optional 
> optimization under an external control, if there is a significant gain in 
> some particular case.
I don't suggest to insert the auxiliary node in the general case. Just for the 
case that you see a transition of `in` -> `inoutset` or vice versa.

With current task dependencies, you always have 1 node notifying n nodes (`out` 
-> `in`) or n nodes notifying one node (`in` -> `out`). You can see this as an 
amortized O(1) operation per task in the graph.

Here you introduce a code pattern, where n nodes each will need to notify m 
other nodes. This leads to an O(n) operation per task. I'm really worried about 
the complexity of this pattern.
If there is no use case with large n, I don't understand, why `inoutset` was 
introduced in the first place.


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

https://reviews.llvm.org/D97085

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

[PATCH] D97085: [OpenMP] libomp: implement OpenMP 5.1 inoutset task dependence type

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added inline comments.



Comment at: openmp/runtime/src/kmp_taskdeps.cpp:344-346
+// link node as successor of all nodes in the prev_set if any
+npredecessors +=
+__kmp_depnode_link_successor(gtid, thread, task, node, prev_set);

AndreyChurbanov wrote:
> protze.joachim wrote:
> > AndreyChurbanov wrote:
> > > protze.joachim wrote:
> > > > If I understand this code right, this has O(n^2) complexity for two 
> > > > sets of size n?
> > > > 
> > > > Consider:
> > > > ```
> > > > for (int i=0; i > > > #pragma omp task depend(in:A)
> > > > work(A,i);
> > > > }
> > > > for (int i=0; i > > > #pragma omp task depend(inoutset:A)
> > > > work(A,i);
> > > > }
> > > > ```
> > > > All n tasks in the first loop would be predecessor for each of the 
> > > > tasks in the second loop. This will also result in n^2 
> > > > releases/notifications.
> > > > 
> > > > 
> > > > I'd suggest to model the dependencies like:
> > > > ```
> > > > for (int i=0; i > > > #pragma omp task depend(in:A)
> > > > work(A,i);
> > > > }
> > > > #pragma omp taskwait depend(inout:A) nowait
> > > > for (int i=0; i > > > #pragma omp task depend(inoutset:A)
> > > > work(A,i);
> > > > }
> > > > ```
> > > > I.e., add a dummy dependency node between the two sets, which has n 
> > > > predecessors and n successors. You probably understand the depnode code 
> > > > better, than I do, but I think you would only need to add some code in 
> > > > line 357, where `last_set` is moved to `prev_set`.
> > > > This dummy node would reduce linking and releasing complexity to O(n).
> > > This could be a separate research project.  Because such change may cause 
> > > performance regressions on some real codes, so it needs thorough 
> > > investigation.  I mean insertion of an auxiliary dummy node into 
> > > dependency graph is definitely an overhead, which may or may not be 
> > > overridden by reduction in number of edges in the graph.  Or it can be 
> > > made optional optimization under an external control, if there is a 
> > > significant gain in some particular case.
> > I don't suggest to insert the auxiliary node in the general case. Just for 
> > the case that you see a transition of `in` -> `inoutset` or vice versa.
> > 
> > With current task dependencies, you always have 1 node notifying n nodes 
> > (`out` -> `in`) or n nodes notifying one node (`in` -> `out`). You can see 
> > this as an amortized O(1) operation per task in the graph.
> > 
> > Here you introduce a code pattern, where n nodes each will need to notify m 
> > other nodes. This leads to an O(n) operation per task. I'm really worried 
> > about the complexity of this pattern.
> > If there is no use case with large n, I don't understand, why `inoutset` 
> > was introduced in the first place.
> > I don't suggest to insert the auxiliary node in the general case. Just for 
> > the case that you see a transition of `in` -> `inoutset` or vice versa.
> > 
> > With current task dependencies, you always have 1 node notifying n nodes 
> > (`out` -> `in`) or n nodes notifying one node (`in` -> `out`). You can see 
> > this as an amortized O(1) operation per task in the graph.
> > 
> > Here you introduce a code pattern, where n nodes each will need to notify m 
> > other nodes. This leads to an O(n) operation per task. I'm really worried 
> > about the complexity of this pattern.
> No, I don't introduce the pattern, as it already worked for sets of tasks 
> with `in` dependency following set of tasks with `mutexinoutset` dependency 
> or vice versa.
> 
> > If there is no use case with large n, I don't understand, why `inoutset` 
> > was introduced in the first place.
> I didn't like that `inoutset` was introduced as the clone of "in" in order to 
> separate two (big) sets of mutually independent tasks, but this has already 
> been added to specification, so we have to implement it. Of cause your 
> suggestion can dramatically speed up dependency processing of some trivial 
> case with two huge sets of tasks one wit `in` dependency another with 
> `inoutset`; but it slightly changes the semantics of user's code, and 
> actually can slowdown other cases.  I would let users do such optimizations.  
> Or compiler can do this once it sees big sets of tasks those don't have 
> barrier-like "taskwait nowait".  For user this is one line of code, for 
> compiler this is dozen lines of code, for runtime library this is pretty big 
> change which does not worth efforts to implement, I think.  Especially given 
> that such "optimization" will definitely slowdown some cases.  E.g. small set 
> of tasks with `in` dependency can be followed single task with `inoutset` in 
> a loop. Why should we slowdown this case?  Library has no idea of the 
> structure of user's code.
> 
> In general, I don't like the idea when runtime library tries to optimize 
> user's code.  Especially along with other changes in the same patch.
> 
> No, I don't introduce 

[PATCH] D94745: [OpenMP][deviceRTLs] Build the deviceRTLs with OpenMP instead of target dependent language

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

For me this patch breaks building llvm. Before this patch, I successfully built 
llvm using a llvm/10 installation on the system. What is probably special with 
our llvm installation is that we by default use libc++ rather than libstdc++.

  FAILED: 
projects/openmp/libomptarget/deviceRTLs/nvptx/loop.cu-cuda_110-sm_60.bc 
  cd BUILD/projects/openmp/libomptarget/deviceRTLs/nvptx && 
/home/pj416018/sw/UTIL/ccache/bin/clang -S -x c++ -target nvptx64 -Xclang 
-emit-llvm-bc -Xclang -aux-triple -Xclang x86_64-unknown-linux-gnu -fopenmp 
-fopenmp-cuda-mode -Xclang -fopenmp-is-device -D__CUDACC__ 
-I${llvm-SOURCE}/openmp/libomptarget/deviceRTLs 
-I${llvm-SOURCE}/openmp/libomptarget/deviceRTLs/nvptx/src 
-DOMPTARGET_NVPTX_DEBUG=0 -Xclang -target-cpu -Xclang sm_60 -D__CUDA_ARCH__=600 
-Xclang -target-feature -Xclang +ptx70 -DCUDA_VERSION=11000 
${llvm-SOURCE}/openmp/libomptarget/deviceRTLs/common/src/loop.cu -o 
loop.cu-cuda_110-sm_60.bc
  In file included from 
${llvm-SOURCE}/openmp/libomptarget/deviceRTLs/common/src/loop.cu:16:
  In file included from 
${llvm-SOURCE}/openmp/libomptarget/deviceRTLs/common/omptarget.h:18:
  In file included from 
${llvm-SOURCE}/openmp/libomptarget/deviceRTLs/common/debug.h:31:
  In file included from 
${llvm-SOURCE}/openmp/libomptarget/deviceRTLs/common/device_environment.h:16:
  In file included from 
${llvm-SOURCE}/openmp/libomptarget/deviceRTLs/nvptx/src/target_impl.h:18:
  In file included from 
${llvm-INSTALL}/10.0.0/bin/../include/c++/v1/stdlib.h:100:
  In file included from ${llvm-INSTALL}/10.0.0/bin/../include/c++/v1/math.h:312:
  ${llvm-INSTALL}/10.0.0/bin/../include/c++/v1/limits:406:89: error: host 
requires 128 bit size 'std::__1::__libcpp_numeric_limits::type' (aka 'long double') type support, but device 'nvptx64' does not 
support it
  _LIBCPP_INLINE_VISIBILITY static _LIBCPP_CONSTEXPR type lowest() 
_NOEXCEPT {return -max();}

  ^

My cmake call looks like:

  cmake -GNinja -DCMAKE_BUILD_TYPE=Release \
-DCMAKE_INSTALL_PREFIX=$INSTALLDIR \
-DLLVM_ENABLE_LIBCXX=ON \
-DCLANG_DEFAULT_CXX_STDLIB=libc++ \
-DCLANG_OPENMP_NVPTX_DEFAULT_ARCH=sm_70 \
-DLIBOMPTARGET_ENABLE_DEBUG=on \
-DLIBOMPTARGET_NVPTX_ENABLE_BCLIB=true \
-DLIBOMPTARGET_NVPTX_COMPUTE_CAPABILITIES=35,60,70 \

-DLLVM_ENABLE_PROJECTS="clang;compiler-rt;libcxxabi;libcxx;libunwind;openmp" \
-DCMAKE_BUILD_WITH_INSTALL_RPATH=ON \
$LLVM_SOURCE

I also tried to build using my newest installed llvm build 
(7dd198852b4db52ae22242dfeda4eccda83aa8b2 
):

  In file included from 
${llvm-SOURCE}/openmp/libomptarget/deviceRTLs/nvptx/src/target_impl.cu:14:
  In file included from 
${llvm-SOURCE}/openmp/libomptarget/deviceRTLs/nvptx/src/target_impl.h:18:
  ${llvm-INSTALL}/bin/../include/c++/v1/stdlib.h:128:10: error: 
'__builtin_fabsl' requires 128 bit size 'long double' type support, but device 
'nvptx64' does not support it
return __builtin_fabsl(__lcpp_x);
   ^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94745

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

[PATCH] D94745: [OpenMP][deviceRTLs] Build the deviceRTLs with OpenMP instead of target dependent language

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

In D94745#2539454 , @JonChesterfield 
wrote:

> I think there's a bug report about this. Sycl (iirc) introduced a change that 
> caught invalid things with types that were previously ignored. @jdoerfert is 
> on point I think.
>
> `-DLLVM_ENABLE_PROJECTS="clang;compiler-rt;libcxxabi;libcxx;libunwind;openmp" 
> `
> ^ That works, if the compiler in question can build things like an nvptx 
> devicertl, which essentially means if it's a (sometimes very) recent clang. A 
> generally easier path is
> `-DLLVM_ENABLE_RUNTIMES="openmp"`
> as that will build clang first, then use that clang to build openmp.
>
> Won't help in this particular instance - if I understand correctly, it's a 
> misfire from using glibc headers on the nvptx subsystem - though that 
> stdlib.h looks like it shipped as part of libc++.
>
> edit: I am too slow...

I tried @jdoerfert 's patches, but they are not even necessary to address my 
building issue. Just delaying the build of OpenMP by setting 
`-DLLVM_ENABLE_RUNTIMES="openmp"` helped.
From my perspective, we should error out if people try to build OpenMP as a 
project rather than runtime and print a error message about what to change.

In any case, a stand-alone build of OpenMP still fails with any of the older 
clang compilers. Should we disable building of libomptarget, if an old clang is 
used? CMake diagnostic could suggest to use in-tree build for libomptarget.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94745

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

[PATCH] D94745: [OpenMP][deviceRTLs] Build the deviceRTLs with OpenMP instead of target dependent language

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

Sorry, I meant to disable building of the cuda device-runtime, or whatever is 
breaking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94745

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

[PATCH] D94735: CGDebugInfo CreatedLimitedType: Drop file/line for RecordType with invalid location

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added subscribers: jdoerfert, protze.joachim.
protze.joachim added a comment.

@jdoerfert  
This commit introduced the compile errors when OpenMP code is compiled with 
debug information. The symptom in llvm tests are the failing libarcher tests 
(which seem to be the only OpenMP test to be compiled with `-g -fopenmp`)
I have no idea how this patch is related to OpenMP codegen.

If anyone wants to have a look at a failing harbormaster example: 
https://reviews.llvm.org/harbormaster/unit/view/323409/

To reproduce the error manually:

  llvm-project/build/./bin/clang 
llvm-project/openmp/tools/archer/tests/races/task-two.c -I 
llvm-project/build/projects/openmp/runtime/src -g -fopenmp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94735

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

[PATCH] D94735: CGDebugInfo CreatedLimitedType: Drop file/line for RecordType with invalid location

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

Reverting this commit fixes the compiler errors. 
This issue is tracked in https://bugs.llvm.org/show_bug.cgi?id=48953 , which is 
marked as blocking the 12 release.

Without any knowledge about the compiler backend or the Debug codegen: might it 
be possible, that this change impacts debug info for any #-line? Also `#pragma 
omp` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94735

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

[PATCH] D103885: [clang] Suppress warnings for tautological comparison in generated macro code

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim created this revision.
protze.joachim added reviewers: jdoerfert, aaron.ballman, hokein.
protze.joachim added projects: clang, clang-tools-extra.
Herald added subscribers: usaxena95, kadircet, arphaman, hiraditya.
protze.joachim requested review of this revision.
Herald added subscribers: cfe-commits, llvm-commits.
Herald added a project: LLVM.

The warnings for some of the generated and included files can quickly fill 
pages and might hide serious issues. The macro purposely performs tautological 
comparison to filter those cases. Filtering the warning for the respective code 
regions seems a reasonable approach.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103885

Files:
  clang-tools-extra/clangd/CompileCommands.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  llvm/lib/Frontend/OpenMP/OMPContext.cpp


Index: llvm/lib/Frontend/OpenMP/OMPContext.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPContext.cpp
+++ llvm/lib/Frontend/OpenMP/OMPContext.cpp
@@ -58,6 +58,10 @@
 break;
   }
 
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wtautological-compare"
+  // The generated code is expected to generate tautological comparison
+
   // Add the appropriate device architecture trait based on the triple.
 #define OMP_TRAIT_PROPERTY(Enum, TraitSetEnum, TraitSelectorEnum, Str) 
\
   if (TraitSelector::TraitSelectorEnum == TraitSelector::device_arch) {
\
@@ -68,6 +72,7 @@
   ActiveTraits.set(unsigned(TraitProperty::Enum)); 
\
   }
 #include "llvm/Frontend/OpenMP/OMPKinds.def"
+#pragma clang diagnostic pop
 
   // TODO: What exactly do we want to see as device ISA trait?
   //   The discussion on the list did not seem to have come to an agreed
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -6227,12 +6227,17 @@
 &ResultBuilder::IsType);
   Results.EnterNewScope();
 
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wtautological-compare"
+  // The generated code is expected to generate tautological comparison
+
   // Add the names of overloadable operators. Note that OO_Conditional is not
   // actually overloadable.
 #define OVERLOADED_OPERATOR(Name, Spelling, Token, Unary, Binary, MemberOnly)  
\
   if (OO_##Name != OO_Conditional) 
\
 Results.AddResult(Result(Spelling));
 #include "clang/Basic/OperatorKinds.def"
+#pragma clang diagnostic pop
 
   // Add any type names visible from the current scope
   Results.allowNestedNameSpecifiers();
Index: clang-tools-extra/clangd/CompileCommands.cpp
===
--- clang-tools-extra/clangd/CompileCommands.cpp
+++ clang-tools-extra/clangd/CompileCommands.cpp
@@ -352,6 +352,10 @@
 };
 // Also grab prefixes for each option, these are not fully exposed.
 const char *const *Prefixes[DriverID::LastOption] = {nullptr};
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wtautological-compare"
+// the generated code is expected to generate tautological comparison
 #define PREFIX(NAME, VALUE) static const char *const NAME[] = VALUE;
 #define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  
\
HELP, METAVAR, VALUES)  
\
@@ -361,6 +365,7 @@
 #include "clang/Driver/Options.inc"
 #undef OPTION
 #undef PREFIX
+#pragma clang diagnostic pop
 
 auto Result = std::make_unique();
 // Iterate over distinct options (represented by the canonical alias).


Index: llvm/lib/Frontend/OpenMP/OMPContext.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPContext.cpp
+++ llvm/lib/Frontend/OpenMP/OMPContext.cpp
@@ -58,6 +58,10 @@
 break;
   }
 
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wtautological-compare"
+  // The generated code is expected to generate tautological comparison
+
   // Add the appropriate device architecture trait based on the triple.
 #define OMP_TRAIT_PROPERTY(Enum, TraitSetEnum, TraitSelectorEnum, Str) \
   if (TraitSelector::TraitSelectorEnum == TraitSelector::device_arch) {\
@@ -68,6 +72,7 @@
   ActiveTraits.set(unsigned(TraitProperty::Enum)); \
   }
 #include "llvm/Frontend/OpenMP/OMPKinds.def"
+#pragma clang diagnostic pop
 
   // TODO: What exactly do we want to see as device ISA trait?
   //   The discussion on the list did not seem to have come to an agreed
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -6227,12 +6227,17 @@

[PATCH] D103885: [clang] Suppress warnings for tautological comparison in generated macro code

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

I don't think, this needs ifdefs. As I understand, a compiler is supposed to 
ignore unknown pragmas.
I fully agree that the suppression will not be compatible with other compilers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103885

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

[PATCH] D97085: [OpenMP] libomp: implement OpenMP 5.1 inoutset task dependence type

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added inline comments.



Comment at: openmp/runtime/src/kmp_taskdeps.cpp:418
 if (dep_list[i].base_addr != 0) {
+  KMP_DEBUG_ASSERT(
+  dep_list[i].flag == KMP_DEP_IN || dep_list[i].flag == KMP_DEP_OUT ||

I hit this assertion, when compiling the tests with clang-11. Is this expected 
behavior? Does this patch break backwards compatibility, or should this 
assertion just not look at the higher bits, as they might be uninitialized?

Whey I change `dep_list[i].flag` to `(dep_list[i].flag&0xf)`, the assertion 
doesn't trigger.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97085

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

[PATCH] D99353: [driver] Make `clang` warn rather then error on `flang` options

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

Any chance that we get this into llvm/13?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99353

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

[PATCH] D99353: [driver] Make `clang` warn rather then error on `flang` options

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

The use case, where I see the current behavior as an issue is linking multiple 
object files compiled from different languages.
Many build systems will pass CFLAGS as well as FFLAGS to the linker command in 
such case. To automatically link the necessary language-specific runtime 
libraries, I see using the compiler for linking as common practice:

  $(FC) -c fortran-code.F90 $(FFLAGS)
  $(CXX) -c cpp-code.cpp $(CFLAGS)
  $(CC) -c c-code.c $(CFLAGS)
  $(CXX) fortran-code.o cpp-code.o c-code.o $(FFLAGS) $(CFLAGS) 
-l$(FORTRAN_RUNTIME)

To support such workflow, the compiler should not error out, at least when only 
used for linking.

This works with `CC=gcc`, `CXX=g++`, `FC=gfortran`, and 
`FORTRAN_RUNTIME=gfortran`.
This used to work with `CC=clang`, `CXX=clang++`, `FC=gfortran`, and 
`FORTRAN_RUNTIME=gfortran`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99353

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

[PATCH] D99353: [driver] Make `clang` warn rather then error on `flang` options

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

Since I see this as a regression over clang-12, I posted 
https://bugs.llvm.org/show_bug.cgi?id=51339 as a release blocking issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99353

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

To make debugging of failed tests easier, we could just explicitly include `env 
LD_LIBRARY_PATH=...` into each run line - by adding it to the `%libomp-run` 
substitution (and the libomptarget equivalent).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

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

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

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 for structured and unstructured 
map. If one of them is >0, the data is present. If both become 0, data is 
deleted.

I think, the `hold` modifier is not sufficient to replicate OpenACC behavior. 
Consider the following example:

  #pragma acc data copy(a)  // structured ref := 1
  {
  #pragma acc exit data delete(a) // dynamic ref := 0
  #pragma acc enter data copyin(a) // dynamic ref := 1
  } // structured ref := 0 // no copyout because dynamic ref >0

As I understand this will be translated to the following OpenMP:

  #pragma omp target data map(ompx_hold, tofrom:a)  // ref := 1
  {
  #pragma omp exit data map(delete:a) // ref := 0  // no action because of hold
  #pragma omp enter data map(to:a) // ref := 1
  } // ref := 0 // perform map from

I don't think, that trying to map the two openacc reference count to a single 
openmp reference count will work in general.


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

https://reviews.llvm.org/D106509

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

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

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

In D106509#2943316 , @grokos wrote:

> The next patch in this series (D106510 ) 
> modifies libomptarget and introduces a second reference count for ompx_hold. 
> There won't be a singe RefCount anymore. I will review that patch once this 
> one has been finalized.

Ok, thanks for the clarification. This change was not obvious from the 
description of the two patches. Makes sense then.


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

https://reviews.llvm.org/D106509

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

[PATCH] D91944: OpenMP 5.0 metadirective

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

In D91944#2458154 , @jdoerfert wrote:

> The problem is this patch can only resolve to a single directive during 
> parsing. Take
>
>   template
>   void foo() {
> #pragma omp metadirective when(user={condition(b)}) ...
>   }
>
> which is not resolvable at parsing time but it is a valid OpenMP 5.0 use of 
> the metadirective
> that needs to resolve at compile time.

It's unfortunate that this pattern is not supported by this patch. How 
difficult would it be to provide this support, or would the implementation be 
not different to dynamic conditions?
For this templated pattern, the optimizer would throw away the branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91944

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

[PATCH] D97085: [OpenMP] libomp: implement OpenMP 5.1 inoutset task dependence type

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

Can you add the inoutset case to the ompt code as well? omp-tools.h already 
defines `ompt_dependence_type_inoutset`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97085

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

[PATCH] D97085: [OpenMP] libomp: implement OpenMP 5.1 inoutset task dependence type

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added inline comments.



Comment at: openmp/runtime/src/kmp_taskdeps.cpp:344-346
+// link node as successor of all nodes in the prev_set if any
+npredecessors +=
+__kmp_depnode_link_successor(gtid, thread, task, node, prev_set);

If I understand this code right, this has O(n^2) complexity for two sets of 
size n?

Consider:
```
for (int i=0; ihttps://reviews.llvm.org/D97085/new/

https://reviews.llvm.org/D97085

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

[PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim reopened this revision.
protze.joachim added a comment.
This revision is now accepted and ready to land.

The review should not be closed, since the patch was reverted and should be 
recommitted (this time possibly with a reference to this review?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

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

[PATCH] D131830: [OpenMP] Clang Support for taskwait nowait clause

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

Why do we need __kmpc_omp_taskwait_51 ? Taskwait nowait only makes sense with 
dependencies. It should only result in additional nodes/edges in the task 
dependency graph without any code to execute. There is not taskwait (aka task 
barrier) in this case.

The taskwait construct has the restriction in the spec:

> The **nowait** clause may only appear on a **taskwait** directive if the 
> **depend** clause is present.


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

https://reviews.llvm.org/D131830

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

[PATCH] D117977: [cmake] Don't export `LLVM_TOOLS_INSTALL_DIR` anymore

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

I think in openmp/tools currently only libraries and header files get 
installed, but no binaries.

I suggest to define `OPENMP_TOOLS_INSTALL_BINDIR`, 
`OPENMP_TOOLS_INSTALL_LIBDIR`, and `OPENMP_TOOLS_INSTALL_INCLUDEDIR` and 
probably base these variables on OPENMP_TOOLS_INSTALL_DIR, if defined (e.g., 
for distro builds), or OPENMP_INSTALL_{BIN|LIB|INCLUDE}DIR as fallback.

openmp/tools/*/CMakeLists.txt should then use these variables instead of the 
various variables they use currently.




Comment at: openmp/tools/CMakeLists.txt:1-3
+set(OPENMP_TOOLS_INSTALL_DIR "${CMAKE_INSTALL_BINDIR}" CACHE PATH
+"Path for binary subdirectory (defaults to '${CMAKE_INSTALL_BINDIR}')")
+mark_as_advanced(OPENMP_TOOLS_INSTALL_DIR)

Is this variable used anywhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117977

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

[PATCH] D95976: [OpenMP] Simplify offloading parallel call codegen

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

Please update the test with a NFC commit.




Comment at: openmp/libomptarget/test/offloading/bug49779.cpp:1-5
+// RUN: %libomptarget-compilexx-run-and-check-aarch64-unknown-linux-gnu
+// RUN: %libomptarget-compilexx-run-and-check-powerpc64-ibm-linux-gnu
+// RUN: %libomptarget-compilexx-run-and-check-powerpc64le-ibm-linux-gnu
+// RUN: %libomptarget-compilexx-run-and-check-x86_64-pc-linux-gnu
+// RUN: %libomptarget-compilexx-run-and-check-nvptx64-nvidia-cuda

See D101326



Comment at: openmp/libomptarget/test/offloading/bug49779.cpp:29-36
+  assert(C >= 2 && C <= 6);
+
+  std::cout << "PASS\n";
+
+  return 0;
+}
+

Since the output goes to Filecheck anyways, I think we should avoid asserts, 
but let Filecheck test for expected results. 
The output for failing tests has more information with this approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95976

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:648
 
+void tools::addOpenMPRuntimeSpecificRPath(const ToolChain &TC,
+  const ArgList &Args,

JonChesterfield wrote:
> lebedev.ri wrote:
> > JonChesterfield wrote:
> > > Similar to other functions in this file, derived from aomp (by deleting 
> > > some stuff for finding a debug version of the library)
> > > 
> > > I can make my peace with runpath instead if people are keen to override 
> > > this with LD_LIBRARY_PATH. The rest of clang's toolchains wire things up 
> > > with rpath but that doesn't mean we have to make the same choice here.
> > I think it would be a shame if this would be the only thing
> > that *doesn't* support being overriden with `LD_LIBRARY_PATH`.
> > I'm not sure about `libomptarget`, but it i think it would be good to keep 
> > such possibility for `libomp`.
> The search order is 'rpath' then 'LD_LIBRARY_PATH' then 'runpath'. Presently 
> we set no default at all and require the user to set LD_LIBRARY_PATH or 
> manually link the right library. So using runpath here is backwards 
> compatible, in that all the scripts out there that use LD_LIBRARY_PATH will 
> continue to work. That may force our hand here.
Especially for debugging, I like the ability to exchange the OpenMP runtimes by 
adding the debugging build of the runtimes to LD_LIBRARY_PATH without needing 
to relink the application, so I'd also prefer runpath.


In the manpage of ld I found: 
> For a native ELF linker, the directories in "DT_RUNPATH" or "DT_RPATH" of a 
> shared library are searched for shared libraries needed by it. The "DT_RPATH" 
> entries are ignored if "DT_RUNPATH" entries exist.

Does this mean, that adding a runpath here will lead to ignoring the other 
rpath entries? Or does this only affect linking shared libraries and not 
linking an application?




Comment at: openmp/libomptarget/test/lit.cfg:182
+
+if config.cuda_path and config.cuda_path != 'CUDA_TOOLKIT_ROOT_DIR-NOTFOUND':
   config.substitutions.append(("%cuda_flags", "--cuda-path=" + 
config.cuda_path))

This would completely avoid the --cuda-path flag for non-nvptx tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D95460: [flang][driver] Add forced form flags and -ffixed-line-length

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

Before this patch, clang would happily ignore a `-ffree-form` flag. With this 
patch, none of `-Wno-error=unknown-argument`, `-Wno-unknown-argument` , 
`-Qunused-arguments` help to avoid clang from exiting with

  error: unknown argument: '-ffree-form'

Why can't clang ignore these flags as any other unknown flags?

As a background: in the build system I'm dealing with, I cannot avoid that 
fortran flags are passed to the linking command. As C++ and fortran is 
involved, I prefer using clang++ as the linking command and explicitly link the 
fortran runtime library (at the moment gfortran, but in the future probably the 
flang runtime library)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95460

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

[PATCH] D95460: [flang][driver] Add forced form flags and -ffixed-line-length

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

Hi Andrzej,

thanks for the detailed insights. I think I really misunderstood, what the -W 
flags can do here. I also was not up-to-date regarding the state of the flang 
implementation.

I just found, that compiling Spec OMP 2012 with clang-12 worked fine with my 
configuration, but compiling with current HEAD failed. Bisecting brought me to 
this patch.
What my build configuration basically does is to use gfortran to compile 
Fortran source to object files. Then I use clang/clang++ to link all the object 
files, basically:

  gfortran -c foo.f
  clang -lgfortran --gcc-toolchain=$(dirname $(dirname $(which gcc))) foo.o

In the build configuration, I can set FPORTABILITY flags for specific apps, but 
these flags are unfortunately passed to both the compile and the link step. 
Setting FLD to clang used to work, and now it broke.

One of my reasons for linking with clang is to make sure, that as much LLVM 
libraries as possible are linked. Especially, I want to pick up the LLVM 
version of the ThreadSanitizer runtime.

I tried to change my build configuration to use flang. After fixing some of the 
code (removing save keyword from variables, when having a global save), I could 
successfully compile one of the codes. Since flang under the hood uses gfortran 
for linking, this configuration picks up the GNU version of the ThreadSanitizer 
runtime. The GNU runtime is typically built as a dynamic library and comes with 
~30-40% performance penalty. So, even when compiling with flang, I would prefer 
to link using clang.

I have no idea about the compiler internals, but would it be possible to mark 
the flang flags as known to the compiler tool-chain, but not used in case of 
clang? Finally, this would result in a `-Wunused-command-line-argument` warning.

Best
Joachim


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95460

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

[PATCH] D99353: [driver] Make `clang` warn rather then error on `flang` options

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

Thank you for working on this! It works for me.
As you mentioned in D95460 , this makes the 
behavior more similar to gcc.

I tested with `-Werror`:

  $ flang -fopenmp test-f77.f -ffree-form -c
  $ clang -fopenmp test-f77.o -ffree-form -lgfortran -Werror && echo $?
  clang-13: warning: command line option ‘-ffree-form’ is only valid in Flang 
mode (i.e. for Fortran input)
  clang-13: error: argument unused during compilation: '-ffree-form' 
[-Werror,-Wunused-command-line-argument]

Since `-Werror` only raises the second warning, 
`-Wno-error=unused-command-line-argument` allows to successfully compile with 
`-Werror`.

  $ clang -fopenmp test-f77.o -ffree-form -lgfortran -Werror 
-Wno-error=unused-command-line-argument && echo $?
  clang-13: warning: command line option ‘-ffree-form’ is only valid in Flang 
mode (i.e. for Fortran input)
  clang-13: warning: argument unused during compilation: '-ffree-form' 
[-Wunused-command-line-argument]
  0

I also tested with `-ffixed-form` and `-ffixed-line-length-132`. The latter 
doesn't work, while `-ffixed-line-length=132` works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99353

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

[PATCH] D99353: [driver] Make `clang` warn rather then error on `flang` options

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

In D99353#2669046 , @awarzynski wrote:

> Btw, how important are these aliases for you?

I can work with `-ffixed-line-length=132` where needed. It's just not obvious 
from `flang --help` that this is an alias for `-ffixed-line-length-132` (or the 
other way around). I only learned that by looking at LLVM source.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99353

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

[PATCH] D100673: [OPENMP]Fix PR49698: OpenMP declare mapper causes segmentation fault.

[Joachim Protze via Phabricator via cfe-commits]

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

I tested the patch and still get wrong results (I modified the 
`declare_mapper_nested_default_mappers_complex_structure.cpp` test to use my 
verbose printf from bugzilla):

  Modified Data Hierarchy:
  Object C (0x623e50, 2) Contents:
  Object B (0x623e80, 2) Contents:
  Object A (0x6243f0) Contents:
  data1 = 6439680  data2 = 0
  Object A (0x6243f8) Contents:
  data1 = -784902216  data2 = 11062
  Object B (0x623e90, 2) Contents:
  Object A (0x6249e0) Contents:
  data1 = 6441856  data2 = 0
  Object A (0x6249e8) Contents:
  data1 = 6439904  data2 = 0
  Object C (0x623e60, 2) Contents:
  Object B (0x623ef0, 2) Contents:
  Object A (0x624b90) Contents:
  data1 = 6438736  data2 = 0
  Object A (0x624b98) Contents:
  data1 = 6441344  data2 = 0
  Object B (0x623f00, 2) Contents:
  Object A (0x624c60) Contents:
  data1 = 6444032  data2 = 0
  Object A (0x624c68) Contents:
  data1 = 6441856  data2 = 0

The values inside the target region look good, but things break when mapping 
back.

When I remove the `teams distribute for` and only leave `#pragma omp target`, I 
get a different result and the test mistakenly passes:

  Modified Data Hierarchy:
  Object C (0x622e40, 2) Contents:
  Object B (0x622e70, 2) Contents:
  Object A (0x6233e0) Contents:
  data1 = 6433120  data2 = 0
  Object A (0x6233e8) Contents:
  data1 = 11  data2 = 22
  Object B (0x622e80, 2) Contents:
  Object A (0x6239d0) Contents:
  data1 = 6435792  data2 = 0
  Object A (0x6239d8) Contents:
  data1 = 11  data2 = 22
  Object C (0x622e50, 2) Contents:
  Object B (0x622ee0, 2) Contents:
  Object A (0x623b80) Contents:
  data1 = 6437312  data2 = 0
  Object A (0x623b88) Contents:
  data1 = 11  data2 = 22
  Object B (0x622ef0, 2) Contents:
  Object A (0x623c50) Contents:
  data1 = 6437744  data2 = 0
  Object A (0x623c58) Contents:
  data1 = 11  data2 = 22
  Testing for correctness...
  outer[1].arr[1].arr[1].data2 = 22.

I'm currently only testing x86_64 offloading.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100673

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

[PATCH] D100673: [OPENMP]Fix PR49698: OpenMP declare mapper causes segmentation fault.

[Joachim Protze via Phabricator via cfe-commits]

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

The latest changes fix the issue. I tested with x86_64 and nvidia offloading.

The behavior and tests looks good to me, just one comment.




Comment at: 
openmp/libomptarget/test/mapping/declare_mapper_nested_default_mappers_complex_structure.cpp:123-126
+  assert(outer[1].arr[1].arr[0].data1 == 11 &&
+ outer[1].arr[1].arr[0].data2 == 22 &&
+ outer[1].arr[1].arr[1].data1 == 11 &&
+ outer[1].arr[1].arr[1].data2 == 22);

Do these asserts make sense with the assert in line 104?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100673

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

[PATCH] D91944: OpenMP 5.0 metadirective

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

I found two issues with this patch regarding the `default` clause:

- The spec does not require a default clause. I get `error: expected 
expression` if I omit a default clause. The error is gone if I add `default()`.
- The spec does not allow an empty `default()` clause, but rather expects 
`default(nothing)`or omission of the default clause. This patch accepts the 
empty default clause.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91944

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

In D101960#2960641 , @JonChesterfield 
wrote:

> Pasting `env LD_LIBRARY_PATH=` and `env LIBRARY_PATH` into the printed 
> commandline, as opposed to through magic, would solve most of my day to day 
> frustration here. libomp.so and libomptarget.so are in two different 
> directories, LD_LIBRARY_PATH turns out to be colon delimited. LIBRARY_PATH is 
> presently used to find the deviceRTL though I'd like to change that.

The lit config has platform-specific rules to build the environmental variables 
(including the use of the proper separators). I suggest to used these values to 
create the printed env expressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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