Hi Heikki and Team, Thanks for your comments.
Here are some more details
> I still don't understand. We have Linux powerpc systems
> running happily in the buildfarm. They are happy with the
> current spinlock implementation. Why is this needed?
> What happens without it?
Not sure, by the time the below commits were made if there was a consideration
to use the gcc routines. My guess is that by using this PPC assembler code
would make the code independent of the compilers. Even the Linux ppc would use
the
same asm. Since gcc is available on AIX, I have replaced the asm changes with
the gcc routine __sync_lock_test_and_set() to set the lock.
We have the gcc(package) build on the AIX platform and as part of the testsuite
there are no issues wrt this routine. We tried to test with the sample test
program extracted from gcc testsuite. Also we discussed the same with our
compiler teams internally and they see no issues using this routine.
<attached sample test> gcc-12.4.0/libgomp/testsuite/libgomp.c/sections-1.c
--------
> > +#define TAS(lock) _check_lock((slock_t *) (lock), 0, 1)
> >
> > +#define S_UNLOCK(lock) _clear_lock((slock_t *) (lock), 0)
> >
> How is it different from __sync_lock_test_and_set()? Why is it needed?
> What is AIX kernel memory operation?
More Info: _check_lock routine description
https://www.ibm.com/docs/en/aix/7.1?topic=c-check-lock-subroutine
The _check_lock subroutine performs an atomic (uninterruptible) sequence of
operations. The compare_and_swap subroutine is similar, but does not issue
synchronization instructions and therefore is inappropriate for updating
lock
words.
This change need not be replaced with gcc routine as, these changes will be
compiled for the non-gcc platforms only. This piece of code would never be
compiled, as we are using only gcc to build.
I tried to replace the AIX asm (under__ppc__ macro) with the gcc routine
__sync_lock_test_and_set(), and all the buildfarm tests passed. Attached patch
and the buildfarm output. Please let me know your feedback.
----
> On a general note: it's your responsibility to explain all the changes
> in a way that others will understand and can verify. It is especially
> important for something critical and platform-specific like spinlocks,
> because others don't have easy access to the hardware to test these
> things independently. I also want to remind you that from the Postgres
> community point of view, you are introducing support for a new platform,
> AIX, not merely resurrecting the old stuff. Every change needs to be
> justified anew.
I do agree with you. To have a better understand on the previous changes,
I was going through the history of the file(s_lock.h) and see that multiple
changes that were made wrt the tas()routine specific to ppc/AIX. Below are
the commit levels.
I would kindly request, Tom Lane, to provide some insights on these changes.
commit e3b06a871b63b90d4a08560ce184bb33324410b8
commit 50938576d482cd36e52a60b5bb1b56026e63962a << added tas() for AIX
commit 7233aae50bea503369b0a4ef9a3b6a3864c96812
commit ceb4f5ea9c2c6c2bd44d4799ff4a62c40a038894 << added tas() for PPC
commit f9ba0a7fe56398e89fe349476d9e437c3197ea28
commit eb5e4c58d137c9258eff5e41b09cb5fe4ed6d64c
commit cd35d601b859d3a56632696b8d5293cbe547764b
commit 109867748259d286dd01fce17d5f895ce59c68d5
commit 5cfa8dd3007d7e953c6a03b0fa2215d97c581b0c
commit 631beeac3598a73dee2c2afa38fa2e734148031b
commit bc2a050d40976441cdb963ad829316c23e8df0aa
commit c41a1215f04912108068b909569551f42059db29
commit 50938576d482cd36e52a60b5bb1b56026e63962a
Please let me know if would like to try on the hardware, we have recently
setup a node in the OSU lab to try out.
Thanks,
Sriram.
0001-AIX-support-revert-the-changes-from-0b16bb8776bb8.v4.patch
Description: 0001-AIX-support-revert-the-changes-from-0b16bb8776bb8.v4.patch
buildfarm17-summary.log
Description: buildfarm17-summary.log
/* Test that all sections are touched. */
/* { dg-require-effective-target sync_int_long } */
#include <omp.h>
#include <string.h>
#include <assert.h>
#include "libgomp_g.h"
#define N 100
static int data[N];
static int NTHR;
static void clean_data (void)
{
memset (data, -1, sizeof (data));
}
static void test_data (void)
{
int i;
for (i = 0; i < N; ++i)
assert (data[i] != -1);
}
static void set_data (unsigned i, int val)
{
int old;
assert (i >= 1 && i <= N);
old = __sync_lock_test_and_set (data+i-1, val);
assert (old == -1);
}
static void f_1 (void *dummy)
{
int iam = omp_get_thread_num ();
unsigned long s;
for (s = GOMP_sections_start (N); s ; s = GOMP_sections_next ())
set_data (s, iam);
GOMP_sections_end ();
}
static void test_1 (void)
{
clean_data ();
GOMP_parallel_start (f_1, NULL, NTHR);
f_1 (NULL);
GOMP_parallel_end ();
test_data ();
}
static void f_2 (void *dummy)
{
int iam = omp_get_thread_num ();
unsigned s;
while ((s = GOMP_sections_next ()))
set_data (s, iam);
GOMP_sections_end_nowait ();
}
static void test_2 (void)
{
clean_data ();
GOMP_parallel_sections_start (f_2, NULL, NTHR, N);
f_2 (NULL);
GOMP_parallel_end ();
test_data ();
}
int main()
{
omp_set_dynamic (0);
NTHR = 4;
test_1 ();
test_2 ();
return 0;
}
