[PATCH] D131639: [OpenMP] Remove 'stdbool.h' from OpenMP header wrappers

[Ivan Rodriguez via Phabricator via cfe-commits]

ivanrodriguez3753 added a comment.

In D131639#3749633 , @jhuber6 wrote:

> I think it's perfectly reasonable to include system files as part of a 
> toolchain.

I think it comes down to a matter of inconveniencing the user versus the 
developer. We usually go with the latter. I don't particularly agree with your 
statement I quoted above, but I can't say I'm an expert in this area. I'll see 
what others have to say. I've already pulled this into our fork so it doesn't 
make a difference to us, but I did think it would be useful to get input from 
the community on whether or not we should pull this in to upstream.

Thanks for your input and responses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131639

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

[PATCH] D131639: [OpenMP] Remove 'stdbool.h' from OpenMP header wrappers

[Ivan Rodriguez via Phabricator via cfe-commits]

ivanrodriguez3753 added a comment.

This looks good to me, and I agree we should document what this is fixing. Any 
update on if/when this will land?

In my opinion, there's nothing broken about the user code (definitely 
contrived, though). They didn't ask for `stdbool.h` so there should not be a 
redeclaration error. This doesn't seem to be an issue when compiling for NVIDIA 
or host-only OpenMP, either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131639

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

[PATCH] D131639: [OpenMP] Remove 'stdbool.h' from OpenMP header wrappers

[Ivan Rodriguez via Phabricator via cfe-commits]

ivanrodriguez3753 added a comment.

The user didn't define any `__` or `_[A-Z]` identifiers, though?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131639

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

[PATCH] D131639: [OpenMP] Remove 'stdbool.h' from OpenMP header wrappers

[Ivan Rodriguez via Phabricator via cfe-commits]

ivanrodriguez3753 added a comment.

They are defining their own `bool`, which aliases to the built-in `_Bool` 
(which is reserved, as you noted with `_[A-Z]`). I thought `bool` was fair game 
unless they included `stdbool.h`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131639

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

[PATCH] D158559: [OpenMP] WIP: Attempt to fix clang frontend codegen issue

[Ivan Rodriguez via Phabricator via cfe-commits]

ivanrodriguez3753 created this revision.
ivanrodriguez3753 added a reviewer: OpenMP.
Herald added subscribers: pengfei, guansong, tpr, yaxunl.
Herald added a project: All.
ivanrodriguez3753 requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, jplehr, sstefan1.
Herald added a project: clang.

It seems that the OpenMP CodeGen is incorrectly generating a pointer for a size 
calculation on the combined entry of a partially mapped struct. Here is the 
reduced test case:

  scrubbed-user@scrubbed-server: cat reduced.cpp
  #include 
  #include 
  #include 
  
  #define N 1000
  
  struct T {
int dep_1[N];
int dep_2[N];
  };
  
  using namespace std;
  int main() {
#define SMALL 2
T t;
#pragma omp target map(tofrom: t.dep_1, t.dep_2[0:SMALL])
{
  for (int i = 0; i < SMALL; i++) {
t.dep_1[i] = 1;
t.dep_2[i] = 1;
  }
}
  
for (int i = 0; i < SMALL; i++) {
  assert(t.dep_1[i] == 1);
  assert(t.dep_2[i] == 1);
}
  }

Originally, we were mapping `t.dep_2[0:N]`, but I reduced to the smallest size 
that still breaks the runtime. We'll see why we need at least 2 in a second... 
Here is some output from the runtime library crashing

  scrubbed-user@scrubbed-server: 
/ptmp/scrubbed-user/llvm-project/build/bin/clang++ -I 
/ptmp/scrubbed-user/llvm-project/build/projects/openmp/runtime/src -L 
/ptmp/scrubbed-user/llvm-project/build/projects/openmp/libomptarget/ -fopenmp 
-fopenmp-targets=amdgcn-amd-amdhsa -Xopenmp-target=amdgcn-amd-amdhsa 
-march=gfx908 reduced.cpp -g
  scrubbed-user@scrubbed-server: LIBOMPTARGET_DEBUG=1 ./a.out # only including 
relevant output, run yourself for the full verbose debug messaging
  
  PluginInterface --> Entry point 0x maps to 
__omp_offloading_4e_6ccfb3ae_main_l16 (0x55b886d524d8)
  Libomptarget --> Entry  0: Base=0x7ffd9cac727c, Begin=0x7ffd9cac727c, 
Size=4004, Type=0x20, Name=unknown
  Libomptarget --> Entry  1: Base=0x7ffd9cac727c, Begin=0x7ffd9cac727c, 
Size=4000, Type=0x10003, Name=unknown
  Libomptarget --> Entry  2: Base=0x7ffd9cac727c, Begin=0x7ffd9cac821c, 
Size=8, Type=0x10003, Name=unknown
  
  a.out:237581 terminated with signal 6 at PC=7f409bf30c6b SP=7ffd9cac6a00.  
Backtrace:
  /lib64/libc.so.6(gsignal+0x10d)[0x7f409bf30c6b]
  /lib64/libc.so.6(abort+0x177)[0x7f409bf32305]
  
/ptmp/scrubbed-user/llvm-project/build/bin/../lib/libomptarget.so.18git(+0x7452c1)[0x7f409ca652c1]
  
/ptmp/scrubbed-user/llvm-project/build/bin/../lib/libomptarget.so.18git(+0x740fe6)[0x7f409ca60fe6]
  
/ptmp/scrubbed-user/llvm-project/build/bin/../lib/libomptarget.so.18git(__tgt_target_kernel+0xe5)[0x7f409ca60335]
  ./a.out(+0x3385)[0x55b8850d5385]
  /lib64/libc.so.6(__libc_start_main+0xef)[0x7f409bf1b24d]
  ./a.out(+0x312a)[0x55b8850d512a]

If my understanding is correct, the combined entry should have a size equal to 
the highest pointer minus the lowest pointer (in the most ideal scenario). I'm 
not sure if upstream clang uses a tight or loose bounding box for the combined 
entry, but in any case, it's wrong. It should be either 4008 or 8000, depending 
on whether we are being clever or not.

Running in GDB:

  scrubbed-user@scrubbed-server: gdb a.out
  (gdb) r
  Starting program: 
/cray/css/users/scrubbed-user/sandbox/test_cleanup/target_depend_lvalue_01/reduced/upstream_build/a.out
 
  Missing separate debuginfos, use: zypper install 
glibc-debuginfo-2.31-150300.46.1.x86_64
  [Thread debugging using libthread_db enabled]
  Using host libthread_db library "/lib64/libthread_db.so.1".
  [New Thread 0x7fffceb2c700 (LWP 247765)]
  [New Thread 0x7ffece1ff700 (LWP 247766)]
  [Thread 0x7ffece1ff700 (LWP 247766) exited]
  Libomptarget message: explicit extension not allowed: host address specified 
is 0x7fff786c (8 bytes), but device allocation maps to host at 
0x7fff68cc (4004 bytes)
  Libomptarget error: Call to getTargetPointer returned null pointer (device 
failure or illegal mapping).
  Libomptarget error: Call to targetDataBegin failed, abort target.
  Libomptarget error: Failed to process data before launching the kernel.
  Libomptarget error: Consult https://openmp.llvm.org/design/Runtimes.html for 
debugging options.
  reduced.cpp:16:3: Libomptarget fatal error 1: failure of target construct 
while offloading is mandatory
  
  Thread 1 "a.out" received signal SIGABRT, Aborted.
  0x762ccc6b in raise () from /lib64/libc.so.6
  Missing separate debuginfos, use: zypper install 
comgr5.5.0-debuginfo-2.5.0.50500-sles153.63.x86_64 
hip-runtime-amd5.5.0-debuginfo-5.5.30201.50500-sles153.63.x86_64 
hsa-rocr5.5.0-debuginfo-1.8.0.50500-sles153.63.x86_64 
libatomic1-debuginfo-12.2.1+git416-15.1.7.1.x86_64 
libdrm2-debuginfo-2.4.107-150400.1.8.x86_64 
libdrm_amdgpu1-debuginfo-2.4.107-150400.1.8.x86_64 
libefa1-debuginfo-38.1-150400.4.6.x86_64 
libelf1-debuginfo-0.185-150400.5.3.1.x86_64 
libfabric1-debug

[PATCH] D158559: [OpenMP] WIP: Attempt to fix clang frontend codegen issue

[Ivan Rodriguez via Phabricator via cfe-commits]

ivanrodriguez3753 added a comment.

In D158559#4608388 , @ABataev wrote:

> 1. Always provide full context in the patch.

Sure, would you mind mentioning what's missing? Or do you mean the stuff from 
debug output being shortened, as well as LLVM IR being only snippets?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158559

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

[PATCH] D158559: [OpenMP] WIP: Attempt to fix clang frontend codegen issue

[Ivan Rodriguez via Phabricator via cfe-commits]

ivanrodriguez3753 added a comment.

Also, should the underlying issue and test case be filed as an issue on Github? 
I wasn't sure since this revision includes the bug and a description


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158559

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