On 6/15/20 10:28 PM, Kwok Cheung Yeung wrote: > Hello > > This patch adds support on nvptx for __sync_val_compare_and_swap > operations on 1- and 2-byte values. The implementation is a straight > copy of the version for AMD GCN.
Actually it also adds support for __sync_bool_compare_and_swap, be sure to mention this in the commit log. > I have added a new libgomp test that exercises the new operation. I have > also verified that the new code does not cause any regressions on the > nvptx offloading tests, and that the new test passes with both nvptx and > amdgcn as offload targets. > AFAICT, that excercises __sync_val_compare_and_swap, but not __sync_bool_compare_and_swap. You've covered offloading, but now consider the nvptx standalone target. I was surprised to find only one generic test-case exercising char/short __sync_val_compare_and_swap: ... $ find gcc/testsuite/ -type f | xargs grep -l sync_char_short | xargs grep sync_val gcc/testsuite/gcc.dg/pr49696.c: __sync_val_compare_and_swap (x, 1, 0); ... and that's not a run test. So, I think gcc needs a copy of (some of) the gcc/testsuite/gcc.dg/ia64-sync-*.c tests for effective target sync_char_short. However, since this patch only adds partial support, we cannot enable sync_char_short for nvptx yet. So, if you stick to partial support, you should add a char/short copy of ia64-sync-3.c to gcc.target/nvptx (which ideally could be an include of a generic test-case that is active for sync_char_short only, with mention that it can be removed once sync_char_short is enabled for nvptx). > Okay for master and OG10? > I looked at the implementation, and it looks ok to me, though I think we need to make explicit in a comment what the assumptions are: - that we have read and write access to the entire word, and - that the word is not volatile. As for the discussion about libgcc vs backend implementation, I think it's good to have both. In particular, a backend implementation might be able to take advantage of the address space and issue an atom.shared.cas. But since the current patch fixes something, I don't want to hold it until a backend implementation is done. So, config/nvptx part okay for master, provided: - nvptx test-case added - nvptx standalone target testing done - assumption comment added to implementation. This bit can be committed as a separate patch, without the oacc test-case. As for the oacc test-case, you could add the __int128 bit, perhaps along the lines of how things are done in libgomp/testsuite/libgomp.c++/target-8.C ? [ FWIW, it would be nice if the choice between critical section and atomic builtin was postponed till after the host/offload compiler split. Then the nvptx compiler would just fall back on the critical section, and the test-case would have worked out of the box, and there would be no need for the target to pretend to support unsupported atomic operators. Conversely, we could upgrade the omp variables from char/short to int somehow to make sure the assumptions for using non-native subword __sync_val_compare_and_swap are met. ] Thanks, - Tom > Kwok > > > nvptx_subword.patch > > commit 7c3a9c23ba9f5b8fe953aa5492ae75617f2444a3 > Author: Kwok Cheung Yeung <k...@codesourcery.com> > Date: Mon Jun 15 12:34:55 2020 -0700 > > nvptx: Add support for subword compare-and-swap > > 2020-06-15 Kwok Cheung Yeung <k...@codesourcery.com> > > libgcc/ > * config/nvptx/atomic.c: New. > * config/nvptx/t-nvptx (LIB2ADD): Add atomic.c. > > libgomp/ > * testsuite/libgomp.c-c++-common/reduction-16.c: New. > > diff --git a/libgcc/config/nvptx/atomic.c b/libgcc/config/nvptx/atomic.c > new file mode 100644 > index 0000000..4becbd2 > --- /dev/null > +++ b/libgcc/config/nvptx/atomic.c > @@ -0,0 +1,59 @@ > +/* NVPTX atomic operations > + Copyright (C) 2020 Free Software Foundation, Inc. > + Contributed by Mentor Graphics. > + > + This file is free software; you can redistribute it and/or modify it > + under the terms of the GNU General Public License as published by the > + Free Software Foundation; either version 3, or (at your option) any > + later version. > + > + This file is distributed in the hope that it will be useful, but > + WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + General Public License for more details. > + > + Under Section 7 of GPL version 3, you are granted additional > + permissions described in the GCC Runtime Library Exception, version > + 3.1, as published by the Free Software Foundation. > + > + You should have received a copy of the GNU General Public License and > + a copy of the GCC Runtime Library Exception along with this program; > + see the files COPYING3 and COPYING.RUNTIME respectively. If not, see > + <http://www.gnu.org/licenses/>. */ > + > +#include <stdbool.h> > + > +#define __SYNC_SUBWORD_COMPARE_AND_SWAP(TYPE, SIZE) \ > + \ > +TYPE \ > +__sync_val_compare_and_swap_##SIZE (TYPE *ptr, TYPE oldval, TYPE newval) > \ > +{ \ > + unsigned int *wordptr = (unsigned int *)((__UINTPTR_TYPE__ ) ptr & ~3UL); > \ > + int shift = ((__UINTPTR_TYPE__ ) ptr & 3UL) * 8; \ > + unsigned int valmask = (1 << (SIZE * 8)) - 1; > \ > + unsigned int wordmask = ~(valmask << shift); > \ > + unsigned int oldword = *wordptr; \ > + for (;;) \ > + { > \ > + TYPE prevval = (oldword >> shift) & valmask; \ > + if (__builtin_expect (prevval != oldval, 0)) \ > + return prevval; \ > + unsigned int newword = oldword & wordmask; \ > + newword |= ((unsigned int) newval) << shift; \ > + unsigned int prevword \ > + = __sync_val_compare_and_swap_4 (wordptr, oldword, newword); \ > + if (__builtin_expect (prevword == oldword, 1)) \ > + return oldval; \ > + oldword = prevword; \ > + } > \ > +} \ > + \ > +bool \ > +__sync_bool_compare_and_swap_##SIZE (TYPE *ptr, TYPE oldval, TYPE newval) > \ > +{ \ > + return __sync_val_compare_and_swap_##SIZE (ptr, oldval, newval) == oldval; > \ > +} > + > +__SYNC_SUBWORD_COMPARE_AND_SWAP (unsigned char, 1) > +__SYNC_SUBWORD_COMPARE_AND_SWAP (unsigned short, 2) > + > diff --git a/libgcc/config/nvptx/t-nvptx b/libgcc/config/nvptx/t-nvptx > index c4d20c9..ede0bf0 100644 > --- a/libgcc/config/nvptx/t-nvptx > +++ b/libgcc/config/nvptx/t-nvptx > @@ -1,5 +1,6 @@ > LIB2ADD=$(srcdir)/config/nvptx/reduction.c \ > - $(srcdir)/config/nvptx/mgomp.c > + $(srcdir)/config/nvptx/mgomp.c \ > + $(srcdir)/config/nvptx/atomic.c > > LIB2ADDEH= > LIB2FUNCS_EXCLUDE=__main > diff --git a/libgomp/testsuite/libgomp.c-c++-common/reduction-16.c > b/libgomp/testsuite/libgomp.c-c++-common/reduction-16.c > new file mode 100644 > index 0000000..951e522 > --- /dev/null > +++ b/libgomp/testsuite/libgomp.c-c++-common/reduction-16.c > @@ -0,0 +1,46 @@ > +/* { dg-do run } */ > + > +#include <stdlib.h> > + > +#define N 512 > + > +#define GENERATE_TEST(T) \ > +int test_##T (void) \ > +{ \ > + T a[N], res = 0; \ > + \ > + for (int i = 0; i < N; ++i) \ > + a[i] = i & 1; \ > + \ > +_Pragma("omp target teams distribute reduction(||:res) > defaultmap(tofrom:scalar)") \ > + for (int i = 0; i < N; ++i) \ > + res = res || a[i]; \ > + \ > + /* res should be non-zero. */\ > + if (!res) \ > + return 1; \ > + \ > +_Pragma("omp target teams distribute reduction(&&:res) > defaultmap(tofrom:scalar)") \ > + for (int i = 0; i < N; ++i) \ > + res = res && a[i]; \ > + \ > + /* res should be zero. */ \ > + return res; \ > +} > + > +GENERATE_TEST(char) > +GENERATE_TEST(short) > +GENERATE_TEST(int) > +GENERATE_TEST(long) > + > +int main(void) > +{ > + if (test_char ()) > + abort (); > + if (test_short ()) > + abort (); > + if (test_int ()) > + abort (); > + if (test_long ()) > + abort (); > +} >
