On 23/06/2020 5:51 pm, Jakub Jelinek wrote:
On Tue, Jun 23, 2020 at 06:44:26PM +0200, Thomas Schwinge wrote:
On 2020-06-15T21:28:12+0100, Kwok Cheung Yeung <k...@codesourcery.com> wrote:
This patch adds support on nvptx for __sync_val_compare_and_swap operations on
1- and 2-byte values.
Is this a thorough review that these are the only functions missing, or
did you just implement what you found missing for some test case you've
been looking into? Other architectures' similar libgcc files seem to be
defining more of such related functions.
It seems a bit unfortunate to have such a thing outlined in a separate
function, given we're talking about performance-critical code here? Even
more so for GCN, where there's no JIT compiler that can inline it later,
as it's the case for nvptx?
I think this should really be handled by the backend inline, like many other
targets do it when they only support 32-bit+ and not 8/16-bit atomics.
See e.g. sparc backend.
I can see both approaches being used - e.g. Sparc, Alpha, and RS6000 expand the
subword atomics into loops in the backend. However, there are various
linux-atomic.c files in the subdirectories of libgcc/config/ (e.g. for Arm,
PA-RISC, m68k etc.), which use a system-call to do the 32-bit compare-and-swap,
then express smaller operations in terms of that. TilePro, AMDGCN and RISC-V
have atomic.c files that do the subword compare-and-swap in terms of a larger
native compare-and-swap.
I don't know - is it worth doing this in the backend on this architecture? One
thing to keep in mind is that nvptx is a virtual instruction set, so the JIT
compiler would likely inline the libgcc library calls anyway.
Thanks
Kwok
