[v5,00/11] RISC-V: Implement ISA Manual Table A.6 Mappings

  This patchset aims to make the RISCV atomics implementation stronger
than the recommended mapping present in table A.6 of the ISA manual.
https://github.com/riscv/riscv-isa-manual/blob/c7cf84547b3aefacab5463add1734c1602b67a49/src/memory.tex#L1083-L1157 

Context
---------
GCC defined RISC-V mappings [1] before the Memory Model task group
finalized their work and provided the ISA Manual Table A.6/A.7 mappings[2].

For at least a year now, we've known that the mappings were different,
but it wasn't clear if these unique mappings had correctness issues.

Andrea Parri found an issue with the GCC mappings, showing that
atomic_compare_exchange_weak_explicit(-,-,-,release,relaxed) mappings do
not enforce release ordering guarantees. (Meaning the GCC mappings have
a correctness issue).
  https://inbox.sourceware.org/gcc-patches/Y1GbJuhcBFpPGJQ0@andrea/ 

Why not A.6?
---------
We can update our mappings now, so the obvious choice would be to
implement Table A.6 (what LLVM implements/ISA manual recommends).

The reason why that isn't the best path forward for GCC is due to a
proposal by Hans Boehm to add L{d|w|b|h}.aq/rl and S{d|w|b|h}.aq/rl.

For context, there is discussion about fast-tracking the addition of
these instructions. The RISCV architectural review committee supports
adopting a "new and common atomics ABI for gcc and LLVM toochains ...
that assumes the addition of the preceding instructions”. That common
ABI is likely to be A.7.
  https://lists.riscv.org/g/tech-privileged/message/1284 

Transitioning from A.6 to A.7 will cause an ABI break. We can hedge
against that risk by emitting a conservative fence after SEQ_CST stores
to make the mapping compatible with both A.6 and A.7.

What does a mapping compatible with both A.6 & A.7 look like?
---------
It is exactly the same as Table A.6, but SEQ_CST stores have a trailing
fence rw,rw. It's strictly stronger than Table A.6.

Microbenchmark
---------
Hans Boehm helpfully wrote a microbenchmark [3] that uses ARM to give a
rough estimate for the performance benefits/penalties of the different
mappings. The microbenchmark is single threaded and almost-write-only.
This case seems unlikely but is useful for getting a rough idea of the
workload that would be impacted the most.

Testcases
-------
Control: A simple volatile store. This is most similar to a relaxed
store.
Release Store: This is most similar to Sw.rl (one of the instructions in
Hans' proposal).
Store with release fence: This is most similar to the mapping present in
Table A.6.
Store with two fences: This is most similar to the compatibility mapping
present in this patchset.

Machines
-------
Intel(R) Core(TM) i7-8650U (sanity check only): x86 TSO
Cortex A53 (Raspberry pi): ARM In order core
Cortex A55 (Pixel 6 Pro): ARM In order core
Cortex A76 (Pixel 6 Pro): ARM Out of order core
Cortex X1 (Pixel 6 Pro): ARM Out of order core

Microbenchmark Results [4]
--------
Units are nsecs per iteration.

Sanity check
Machine    	   CONTROL   REL_STORE   STORE_REL_FENCE   STORE_TWO_FENCE
-------    	   -------   ---------   ---------------   ---------------
Intel i7-8650U 1.34812   1.30038     1.2933            18.0474


Machine    	CONTROL   REL_STORE   STORE_REL_FENCE   STORE_TWO_FENCE
-------    	-------   ---------   ---------------   ---------------
Cortex A53 	7.15224   10.7282     7.15221           10.013
Cortex A55 	2.77965   8.89654     4.44787           7.78331
Cortex A76 	1.78021   1.86095     5.33088           8.88462
Cortex X1  	2.14252   2.14258     4.32982           7.05234

Reordered tests (using -r flag on microbenchmark)
Machine    	CONTROL   REL_STORE   STORE_REL_FENCE   STORE_TWO_FENCE
-------    	-------   ---------   ---------------   ---------------
Cortex A53 	7.15227   10.7282     7.16113           10.034
Cortex A55 	2.78024   8.89574     4.44844           7.78428
Cortex A76 	1.77686   1.81081     5.3301            8.88346
Cortex X1  	2.14254   2.14251     4.3273            7.05239

Benchmark Interpretation
--------
As expected, out of order machines are significantly faster with the
REL_STORE mappings. Unexpectedly, the in-order machines are
significantly slower with REL_STORE rather than REL_STORE_FENCE.

Most machines in the wild are expected to use Table A.7 once the
instructions are introduced. 
Incurring this added cost now will make it easier for compiled RISC-V
binaries to transition to the A.7 memory model mapping.

The performance benefits of moving to A.7 can be more clearly seen using
an almost-all-load microbenchmark (included on page 3 of Hans’
proposal). The code for that microbenchmark is attached below [5].
  https://lists.riscv.org/g/tech-unprivileged/attachment/382/0/load-acquire110422.pdf 
  https://lists.riscv.org/g/tech-unprivileged/topic/92916241 

Caveats
--------
This is a very synthetic microbenchmark that represents what is expected
to be a very unlikely workload. Nevertheless, it's helpful to see the
worst-case price we are paying for compatibility. 

“All times include an entire loop iteration, indirect dispatch and all.
The benchmark alternates tests, but does not lock CPU frequency. Since a
single core was in use, I expect this was running at basically full
speed. Any throttling affected everything more or less uniformly.”
- Hans Boehm

Patchset overview
--------
Patch 1 simplifies the memmodel to ignore MEMMODEL_SYNC_* cases (legacy
cases that aren't handled differently for RISC-V).
Patches 2-6 make the mappings strictly stronger.
Patches 7-9 weaken the mappings to be in line with table A.6 of the ISA
manual.
Patch 11 adds some basic conformance tests to ensure the implemented
mapping matches table A.6 with stronger SEQ_CST stores.

Conformance test cases notes
--------
The conformance tests in this patch are a good sanity check but do not
guarantee exactly following Table A.6. It checks that the right
instructions are emitted (ex. fence rw,r) but not the order of those
instructions.

LLVM mapping notes
--------
LLVM emits corresponding fences for atomic_signal_fence instructions.
This seems to be an oversight since AFAIK atomic_signal_fence acts as a
compiler directive. GCC does not emit any fences for atomic_signal_fence
instructions.

Future work
--------
There still remains some work to be done in this space after this
patchset fixes the correctness of the GCC mappings. 
* Look into explicitly handling subword loads/stores.
* Look into using AMOSWAP.rl for store words/doubles.
* L{b|h|w|d}.aq/rl & S{b|h|w|d}.aq/rl support once ratified.
* zTSO mappings.

Prior Patchsets
--------
Patchset v1:
  https://gcc.gnu.org/pipermail/gcc-patches/2022-April/592950.html 

Patchset v2:
  https://gcc.gnu.org/pipermail/gcc-patches/2023-April/615264.html 

Patchset v3:
  https://gcc.gnu.org/pipermail/gcc-patches/2023-April/615431.html 

Patchset v4:
  https://gcc.gnu.org/pipermail/gcc-patches/2023-April/615748.html 

Changelogs
--------
Changes for v2:
* Use memmodel_base rather than a custom simplify_memmodel function
  (Inspired by Christoph Muellner's patch 1/9)
* Move instruction styling change from [v1 5/7] to [v2 3/8] to reduce
  [v2 6/8]'s complexity
* Eliminated %K flag for atomic store introduced in v1 in favor of
  if/else
* Rebase/test

Changes for v3:
* Use a trailing fence for atomic stores to be compatible with table A.7
* Emit an optimized fence r,rw following a SEQ_CST load
* Consolidate tests in [PATCH v3 10/10]
* Add tests for basic A.6 conformance

Changes for v4:
* Update cover letter to cover more of the reasoning behind moving to a
  compatibility mapping
* Improve conformance testcases patch assertions and add new
  compare-exchange testcases

Changes for v5:
* Update cover letter to cover more context and reasoning behind moving
  to a compatibility mapping
* Rebase to include the subword-atomic cases introduced here:
  https://gcc.gnu.org/pipermail/gcc-patches/2023-April/616080.html
* Add basic amo-add subword atomic testcases
* Reformat changelogs
* Fix misc. whitespace issues

[1] GCC port with mappings merged 06 Feb 2017
  https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=09cae7507d9e88f2b05cf3a9404bf181e65ccbac

[2] A.6 mappings added to ISA manual 12 Dec 2017
https://github.com/riscv/riscv-isa-manual/commit/9da1a115bcc4fe327f35acceb851d4850d12e9fa 

[3] Hans Boehm almost-all-store Microbenchmark:
// Copyright 2023 Google LLC.
// SPDX-License-Identifier: Apache-2.0

#include <atomic>
#include <iostream>
#include <time.h>

static constexpr int INNER_ITERS = 10'000'000;
static constexpr int OUTER_ITERS = 20;
static constexpr int N_TESTS = 4;

volatile int the_volatile(17);
std::atomic<int> the_atomic(17);

void test1(int i) {
  the_volatile = i;
}

void test2(int i) {
  the_atomic.store(i, std::memory_order_release);
}

void test3(int i) {
  atomic_thread_fence(std::memory_order_release);
  the_atomic.store(i, std::memory_order_relaxed);
}

void test4(int i) {
  atomic_thread_fence(std::memory_order_release);
  the_atomic.store(i, std::memory_order_relaxed);
  atomic_thread_fence(std::memory_order_seq_cst);
}

typedef void (*int_func)(int);

uint64_t getnanos() {
  struct timespec result;
  if (clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &result) != 0) {
	std::cerr << "clock_gettime() failed\n";
	exit(1);
  }
  return (uint64_t)result.tv_nsec + 1'000'000'000 * (uint64_t)result.tv_sec;
}

int_func tests[N_TESTS] = { test1, test2, test3, test4 };
const char *test_names[N_TESTS] =
	{ "control", "release store", "store with release fence", "store with two fences" };
uint64_t total_time[N_TESTS] = { 0 };

int main(int argc, char **argv) {
  struct timespec res;
  if (clock_getres(CLOCK_PROCESS_CPUTIME_ID, &res) != 0) {
	std::cerr << "clock_getres() failed\n";
	exit(1);
  } else {
	std::cout << "nsec resolution = " << res.tv_nsec << std::endl;
  }
  if (argc == 2 && argv[1][0] == 'r') {
	// Run tests in reverse order.
	for (int i = 0; i < N_TESTS / 2; ++i) {
  	std::swap(tests[i], tests[N_TESTS - 1 - i]);
  	std::swap(test_names[i], test_names[N_TESTS - 1 - i]);
	}
  }
  for (int i = 0; i < OUTER_ITERS; ++i) {
	// Alternate tests to minimize bias due to thermal throttling.
	for (int j = 0; j < N_TESTS; ++j) {
  	uint64_t start_time = getnanos();
  	for (int k = 1; k <= INNER_ITERS; ++k) {
    	tests[j](k); // Provides memory accesses between tests.
  	}
  	// Ignore first iteration for all tests. The first iteration of the first test is
  	// empirically slightly slower.
  	if (i != 0) {
    	total_time[j] += getnanos() - start_time;
  	}
  	if ((tests[j] == test1 ? the_volatile : the_atomic.load()) != INNER_ITERS) {
    	std::cerr << "result check failed, test = " << j << ", " << the_volatile << std::endl;
    	exit(1);
  	}
	}
  }
  for (int i = 0; i < N_TESTS; ++i) {
	double nsecs_per_iter = (double) total_time[i] / INNER_ITERS / (OUTER_ITERS - 1);
	std::cout << test_names[i] << " took " << nsecs_per_iter << " nseconds per iteration\n";
  }
  exit(0);
}

[4] Hans Boehm Raw Microbenchmark Results
Intel(R) Core(TM) i7-8650U (sanity check only):

hboehm@hboehm-glaptop0:~/tests$ ./a.out
nsec resolution = 1
control took 1.34812 nseconds per iteration
release store took 1.30038 nseconds per iteration
store with release fence took 1.2933 nseconds per iteration
store with two fences took 18.0474 nseconds per iteration

Cortex A53 (Raspberry pi)
hboehm@rpi3-20210823:~/tests$ ./a.out
nsec resolution = 1
control took 7.15224 nseconds per iteration
release store took 10.7282 nseconds per iteration
store with release fence took 7.15221 nseconds per iteration
store with two fences took 10.013 nseconds per iteration
hboehm@rpi3-20210823:~/tests$ ./a.out -r
nsec resolution = 1
control took 7.15227 nseconds per iteration
release store took 10.7282 nseconds per iteration
store with release fence took 7.16133 nseconds per iteration
store with two fences took 10.034 nseconds per iteration

Cortex A55 (Pixel 6 Pro)

raven:/data/tmp # taskset 0f ./release-timer
nsec resolution = 1
control took 2.77965 nseconds per iteration
release store took 8.89654 nseconds per iteration
store with release fence took 4.44787 nseconds per iteration
store with two fences took 7.78331 nseconds per iteration
raven:/data/tmp # taskset 0f ./release-timer -r                                                                 	 
nsec resolution = 1
control took 2.78024 nseconds per iteration
release store took 8.89574 nseconds per iteration
store with release fence took 4.44844 nseconds per iteration
store with two fences took 7.78428 nseconds per iteration

Cortex A76 (Pixel 6 Pro)
raven:/data/tmp # taskset 30 ./release-timer -r                                                                 	 
nsec resolution = 1
control took 1.77686 nseconds per iteration
release store took 1.81081 nseconds per iteration
store with release fence took 5.3301 nseconds per iteration
store with two fences took 8.88346 nseconds per iteration
raven:/data/tmp # taskset 30 ./release-timer                                                                   	 
nsec resolution = 1
control took 1.78021 nseconds per iteration
release store took 1.86095 nseconds per iteration
store with release fence took 5.33088 nseconds per iteration
store with two fences took 8.88462 nseconds per iteration

Cortex X1 (Pixel 6 Pro)
raven:/data/tmp # taskset c0 ./release-timer                                                                   	 
nsec resolution = 1
control took 2.14252 nseconds per iteration
release store took 2.14258 nseconds per iteration
store with release fence took 4.32982 nseconds per iteration
store with two fences took 7.05234 nseconds per iteration
raven:/data/tmp # taskset c0 ./release-timer -r                                                                 	 
nsec resolution = 1
control took 2.14254 nseconds per iteration
release store took 2.14251 nseconds per iteration
store with release fence took 4.3273 nseconds per iteration
store with two fences took 7.05239 nseconds per iteration

[5] Hans Boehm almost-all-load Microbenchmark:
// Copyright 2023 Google LLC.
// SPDX-License-Identifier: Apache-2.0

#include <atomic>
#include <iostream>
#include <time.h>

static constexpr int INNER_ITERS = 10'000'000;
static constexpr int OUTER_ITERS = 20;
static constexpr int N_TESTS = 4;

volatile int the_volatile(17);
std::atomic<int> the_atomic(17);

int test1() {
  return the_volatile;
}

int test2() {
  return the_atomic.load(std::memory_order_acquire);
}

int test3() {
  int result = the_atomic.load(std::memory_order_relaxed);
  atomic_thread_fence(std::memory_order_acquire);
  return result;
}

int test4() {
  atomic_thread_fence(std::memory_order_seq_cst);
  int result = the_atomic.load(std::memory_order_relaxed);
  atomic_thread_fence(std::memory_order_acquire);
  return result;
}

typedef int (*int_func)();

uint64_t getnanos() {
  struct timespec result;
  if (clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &result) != 0) {
	std::cerr << "clock_gettime() failed\n";
	exit(1);
  }
  return (uint64_t)result.tv_nsec + 1'000'000'000 * (uint64_t)result.tv_sec;
}

int_func tests[N_TESTS] = { test1, test2, test3, test4 };
const char *test_names[N_TESTS] =
	{ "control", "acquire load", "load with acquire fence", "load with two fences" };
uint64_t total_time[N_TESTS] = { 0 };

uint sum, last_sum = 0;

int main(int argc, char **argv) {
  struct timespec res;
  if (clock_getres(CLOCK_PROCESS_CPUTIME_ID, &res) != 0) {
	std::cerr << "clock_getres() failed\n";
	exit(1);
  } else {
	std::cout << "nsec resolution = " << res.tv_nsec << std::endl;
  }
  if (argc == 2 && argv[1][0] == 'r') {
	// Run tests in reverse order.
	for (int i = 0; i < N_TESTS / 2; ++i) {
  	std::swap(tests[i], tests[N_TESTS - 1 - i]);
  	std::swap(test_names[i], test_names[N_TESTS - 1 - i]);
	}
  }
  for (int i = 0; i < OUTER_ITERS; ++i) {
	// Alternate tests to minimize bias due to thermal throttling.
	for (int j = 0; j < N_TESTS; ++j) {
  	sum = 0;
  	uint64_t start_time = getnanos();
  	for (int k = 0; k < INNER_ITERS; ++k) {
    	sum += tests[j](); // Provides memory accesses between tests.
  	}
  	// Ignore first iteration for all tests. The first iteration of the first test is
  	// empirically slightly slower.
  	if (i != 0) {
    	total_time[j] += getnanos() - start_time;
  	}
  	if (sum == 0 || last_sum != 0 && sum != last_sum) {
    	std::cerr << "result check failed";
    	exit(1);
  	}
  	last_sum = sum;
	}
  }
  for (int i = 0; i < N_TESTS; ++i) {
	double nsecs_per_iter = (double) total_time[i] / INNER_ITERS / (OUTER_ITERS - 1);
	std::cout << test_names[i] << " took " << nsecs_per_iter << " nseconds per iteration\n";
  }
  exit(0);
}

Patrick O'Neill (11):
  RISC-V: Eliminate SYNC memory models
  RISC-V: Enforce Libatomic LR/SC SEQ_CST
  RISC-V: Enforce subword atomic LR/SC SEQ_CST
  RISC-V: Enforce atomic compare_exchange SEQ_CST
  RISC-V: Add AMO release bits
  RISC-V: Strengthen atomic stores
  RISC-V: Eliminate AMO op fences
  RISC-V: Weaken LR/SC pairs
  RISC-V: Weaken mem_thread_fence
  RISC-V: Weaken atomic loads
  RISC-V: Table A.6 conformance tests

 gcc/config/riscv/riscv-protos.h               |   3 +
 gcc/config/riscv/riscv.cc                     |  66 ++++--
 gcc/config/riscv/sync.md                      | 194 ++++++++++++------
 .../riscv/amo-table-a-6-amo-add-1.c           |   8 +
 .../riscv/amo-table-a-6-amo-add-2.c           |   8 +
 .../riscv/amo-table-a-6-amo-add-3.c           |   8 +
 .../riscv/amo-table-a-6-amo-add-4.c           |   8 +
 .../riscv/amo-table-a-6-amo-add-5.c           |   8 +
 .../riscv/amo-table-a-6-compare-exchange-1.c  |  10 +
 .../riscv/amo-table-a-6-compare-exchange-2.c  |  10 +
 .../riscv/amo-table-a-6-compare-exchange-3.c  |  10 +
 .../riscv/amo-table-a-6-compare-exchange-4.c  |  10 +
 .../riscv/amo-table-a-6-compare-exchange-5.c  |  10 +
 .../riscv/amo-table-a-6-compare-exchange-6.c  |  11 +
 .../riscv/amo-table-a-6-compare-exchange-7.c  |  10 +
 .../gcc.target/riscv/amo-table-a-6-fence-1.c  |   8 +
 .../gcc.target/riscv/amo-table-a-6-fence-2.c  |  10 +
 .../gcc.target/riscv/amo-table-a-6-fence-3.c  |  10 +
 .../gcc.target/riscv/amo-table-a-6-fence-4.c  |  10 +
 .../gcc.target/riscv/amo-table-a-6-fence-5.c  |  10 +
 .../gcc.target/riscv/amo-table-a-6-load-1.c   |   9 +
 .../gcc.target/riscv/amo-table-a-6-load-2.c   |  11 +
 .../gcc.target/riscv/amo-table-a-6-load-3.c   |  11 +
 .../gcc.target/riscv/amo-table-a-6-store-1.c  |   9 +
 .../gcc.target/riscv/amo-table-a-6-store-2.c  |  11 +
 .../riscv/amo-table-a-6-store-compat-3.c      |  11 +
 .../riscv/amo-table-a-6-subword-amo-add-1.c   |   9 +
 .../riscv/amo-table-a-6-subword-amo-add-2.c   |   9 +
 .../riscv/amo-table-a-6-subword-amo-add-3.c   |   9 +
 .../riscv/amo-table-a-6-subword-amo-add-4.c   |   9 +
 .../riscv/amo-table-a-6-subword-amo-add-5.c   |   9 +
 gcc/testsuite/gcc.target/riscv/pr89835.c      |   9 +
 libgcc/config/riscv/atomic.c                  |   4 +-
 33 files changed, 467 insertions(+), 75 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-amo-add-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-amo-add-2.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-amo-add-3.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-amo-add-4.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-amo-add-5.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-compare-exchange-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-compare-exchange-2.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-compare-exchange-3.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-compare-exchange-4.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-compare-exchange-5.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-compare-exchange-6.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-compare-exchange-7.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-fence-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-fence-2.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-fence-3.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-fence-4.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-fence-5.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-load-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-load-2.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-load-3.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-store-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-store-2.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-store-compat-3.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-subword-amo-add-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-subword-amo-add-2.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-subword-amo-add-3.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-subword-amo-add-4.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-subword-amo-add-5.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/pr89835.c
  

On Thu, Apr 27, 2023 at 09:22:50AM -0700, Patrick O'Neill wrote:
> This patchset aims to make the RISCV atomics implementation stronger
> than the recommended mapping present in table A.6 of the ISA manual.
> https://github.com/riscv/riscv-isa-manual/blob/c7cf84547b3aefacab5463add1734c1602b67a49/src/memory.tex#L1083-L1157 
> 
> Context
> ---------
> GCC defined RISC-V mappings [1] before the Memory Model task group
> finalized their work and provided the ISA Manual Table A.6/A.7 mappings[2].
> 
> For at least a year now, we've known that the mappings were different,
> but it wasn't clear if these unique mappings had correctness issues.
> 
> Andrea Parri found an issue with the GCC mappings, showing that
> atomic_compare_exchange_weak_explicit(-,-,-,release,relaxed) mappings do
> not enforce release ordering guarantees. (Meaning the GCC mappings have
> a correctness issue).
>   https://inbox.sourceware.org/gcc-patches/Y1GbJuhcBFpPGJQ0@andrea/ 
> 
> Why not A.6?
> ---------
> We can update our mappings now, so the obvious choice would be to
> implement Table A.6 (what LLVM implements/ISA manual recommends).
> 
> The reason why that isn't the best path forward for GCC is due to a
> proposal by Hans Boehm to add L{d|w|b|h}.aq/rl and S{d|w|b|h}.aq/rl.
> 
> For context, there is discussion about fast-tracking the addition of
> these instructions. The RISCV architectural review committee supports
> adopting a "new and common atomics ABI for gcc and LLVM toochains ...
> that assumes the addition of the preceding instructions”. That common
> ABI is likely to be A.7.
>   https://lists.riscv.org/g/tech-privileged/message/1284 
> 
> Transitioning from A.6 to A.7 will cause an ABI break. We can hedge
> against that risk by emitting a conservative fence after SEQ_CST stores
> to make the mapping compatible with both A.6 and A.7.
> 
> What does a mapping compatible with both A.6 & A.7 look like?
> ---------
> It is exactly the same as Table A.6, but SEQ_CST stores have a trailing
> fence rw,rw. It's strictly stronger than Table A.6.
> 
> Microbenchmark
> ---------
> Hans Boehm helpfully wrote a microbenchmark [3] that uses ARM to give a
> rough estimate for the performance benefits/penalties of the different
> mappings. The microbenchmark is single threaded and almost-write-only.
> This case seems unlikely but is useful for getting a rough idea of the
> workload that would be impacted the most.
> 
> Testcases
> -------
> Control: A simple volatile store. This is most similar to a relaxed
> store.
> Release Store: This is most similar to Sw.rl (one of the instructions in
> Hans' proposal).
> Store with release fence: This is most similar to the mapping present in
> Table A.6.
> Store with two fences: This is most similar to the compatibility mapping
> present in this patchset.
> 
> Machines
> -------
> Intel(R) Core(TM) i7-8650U (sanity check only): x86 TSO
> Cortex A53 (Raspberry pi): ARM In order core
> Cortex A55 (Pixel 6 Pro): ARM In order core
> Cortex A76 (Pixel 6 Pro): ARM Out of order core
> Cortex X1 (Pixel 6 Pro): ARM Out of order core
> 
> Microbenchmark Results [4]
> --------
> Units are nsecs per iteration.
> 
> Sanity check
> Machine    	   CONTROL   REL_STORE   STORE_REL_FENCE   STORE_TWO_FENCE
> -------    	   -------   ---------   ---------------   ---------------
> Intel i7-8650U 1.34812   1.30038     1.2933            18.0474
> 
> 
> Machine    	CONTROL   REL_STORE   STORE_REL_FENCE   STORE_TWO_FENCE
> -------    	-------   ---------   ---------------   ---------------
> Cortex A53 	7.15224   10.7282     7.15221           10.013
> Cortex A55 	2.77965   8.89654     4.44787           7.78331
> Cortex A76 	1.78021   1.86095     5.33088           8.88462
> Cortex X1  	2.14252   2.14258     4.32982           7.05234
> 
> Reordered tests (using -r flag on microbenchmark)
> Machine    	CONTROL   REL_STORE   STORE_REL_FENCE   STORE_TWO_FENCE
> -------    	-------   ---------   ---------------   ---------------
> Cortex A53 	7.15227   10.7282     7.16113           10.034
> Cortex A55 	2.78024   8.89574     4.44844           7.78428
> Cortex A76 	1.77686   1.81081     5.3301            8.88346
> Cortex X1  	2.14254   2.14251     4.3273            7.05239
> 
> Benchmark Interpretation
> --------
> As expected, out of order machines are significantly faster with the
> REL_STORE mappings. Unexpectedly, the in-order machines are
> significantly slower with REL_STORE rather than REL_STORE_FENCE.
> 
> Most machines in the wild are expected to use Table A.7 once the
> instructions are introduced. 
> Incurring this added cost now will make it easier for compiled RISC-V
> binaries to transition to the A.7 memory model mapping.
> 
> The performance benefits of moving to A.7 can be more clearly seen using
> an almost-all-load microbenchmark (included on page 3 of Hans’
> proposal). The code for that microbenchmark is attached below [5].
>   https://lists.riscv.org/g/tech-unprivileged/attachment/382/0/load-acquire110422.pdf 
>   https://lists.riscv.org/g/tech-unprivileged/topic/92916241 
> 
> Caveats
> --------
> This is a very synthetic microbenchmark that represents what is expected
> to be a very unlikely workload. Nevertheless, it's helpful to see the
> worst-case price we are paying for compatibility. 
> 
> “All times include an entire loop iteration, indirect dispatch and all.
> The benchmark alternates tests, but does not lock CPU frequency. Since a
> single core was in use, I expect this was running at basically full
> speed. Any throttling affected everything more or less uniformly.”
> - Hans Boehm
> 
> Patchset overview
> --------
> Patch 1 simplifies the memmodel to ignore MEMMODEL_SYNC_* cases (legacy
> cases that aren't handled differently for RISC-V).
> Patches 2-6 make the mappings strictly stronger.
> Patches 7-9 weaken the mappings to be in line with table A.6 of the ISA
> manual.
> Patch 11 adds some basic conformance tests to ensure the implemented
> mapping matches table A.6 with stronger SEQ_CST stores.
> 
> Conformance test cases notes
> --------
> The conformance tests in this patch are a good sanity check but do not
> guarantee exactly following Table A.6. It checks that the right
> instructions are emitted (ex. fence rw,r) but not the order of those
> instructions.
> 
> LLVM mapping notes
> --------
> LLVM emits corresponding fences for atomic_signal_fence instructions.
> This seems to be an oversight since AFAIK atomic_signal_fence acts as a
> compiler directive. GCC does not emit any fences for atomic_signal_fence
> instructions.
> 
> Future work
> --------
> There still remains some work to be done in this space after this
> patchset fixes the correctness of the GCC mappings. 
> * Look into explicitly handling subword loads/stores.
> * Look into using AMOSWAP.rl for store words/doubles.
> * L{b|h|w|d}.aq/rl & S{b|h|w|d}.aq/rl support once ratified.
> * zTSO mappings.
> 
> Prior Patchsets
> --------
> Patchset v1:
>   https://gcc.gnu.org/pipermail/gcc-patches/2022-April/592950.html 
> 
> Patchset v2:
>   https://gcc.gnu.org/pipermail/gcc-patches/2023-April/615264.html 
> 
> Patchset v3:
>   https://gcc.gnu.org/pipermail/gcc-patches/2023-April/615431.html 
> 
> Patchset v4:
>   https://gcc.gnu.org/pipermail/gcc-patches/2023-April/615748.html 
> 
> Changelogs
> --------
> Changes for v2:
> * Use memmodel_base rather than a custom simplify_memmodel function
>   (Inspired by Christoph Muellner's patch 1/9)
> * Move instruction styling change from [v1 5/7] to [v2 3/8] to reduce
>   [v2 6/8]'s complexity
> * Eliminated %K flag for atomic store introduced in v1 in favor of
>   if/else
> * Rebase/test
> 
> Changes for v3:
> * Use a trailing fence for atomic stores to be compatible with table A.7
> * Emit an optimized fence r,rw following a SEQ_CST load
> * Consolidate tests in [PATCH v3 10/10]
> * Add tests for basic A.6 conformance
> 
> Changes for v4:
> * Update cover letter to cover more of the reasoning behind moving to a
>   compatibility mapping
> * Improve conformance testcases patch assertions and add new
>   compare-exchange testcases
> 
> Changes for v5:
> * Update cover letter to cover more context and reasoning behind moving
>   to a compatibility mapping
> * Rebase to include the subword-atomic cases introduced here:
>   https://gcc.gnu.org/pipermail/gcc-patches/2023-April/616080.html
> * Add basic amo-add subword atomic testcases
> * Reformat changelogs
> * Fix misc. whitespace issues
> 
> [1] GCC port with mappings merged 06 Feb 2017
>   https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=09cae7507d9e88f2b05cf3a9404bf181e65ccbac
> 
> [2] A.6 mappings added to ISA manual 12 Dec 2017
> https://github.com/riscv/riscv-isa-manual/commit/9da1a115bcc4fe327f35acceb851d4850d12e9fa 
> 
> [3] Hans Boehm almost-all-store Microbenchmark:
> // Copyright 2023 Google LLC.
> // SPDX-License-Identifier: Apache-2.0
> 
> #include <atomic>
> #include <iostream>
> #include <time.h>
> 
> static constexpr int INNER_ITERS = 10'000'000;
> static constexpr int OUTER_ITERS = 20;
> static constexpr int N_TESTS = 4;
> 
> volatile int the_volatile(17);
> std::atomic<int> the_atomic(17);
> 
> void test1(int i) {
>   the_volatile = i;
> }
> 
> void test2(int i) {
>   the_atomic.store(i, std::memory_order_release);
> }
> 
> void test3(int i) {
>   atomic_thread_fence(std::memory_order_release);
>   the_atomic.store(i, std::memory_order_relaxed);
> }
> 
> void test4(int i) {
>   atomic_thread_fence(std::memory_order_release);
>   the_atomic.store(i, std::memory_order_relaxed);
>   atomic_thread_fence(std::memory_order_seq_cst);
> }
> 
> typedef void (*int_func)(int);
> 
> uint64_t getnanos() {
>   struct timespec result;
>   if (clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &result) != 0) {
> 	std::cerr << "clock_gettime() failed\n";
> 	exit(1);
>   }
>   return (uint64_t)result.tv_nsec + 1'000'000'000 * (uint64_t)result.tv_sec;
> }
> 
> int_func tests[N_TESTS] = { test1, test2, test3, test4 };
> const char *test_names[N_TESTS] =
> 	{ "control", "release store", "store with release fence", "store with two fences" };
> uint64_t total_time[N_TESTS] = { 0 };
> 
> int main(int argc, char **argv) {
>   struct timespec res;
>   if (clock_getres(CLOCK_PROCESS_CPUTIME_ID, &res) != 0) {
> 	std::cerr << "clock_getres() failed\n";
> 	exit(1);
>   } else {
> 	std::cout << "nsec resolution = " << res.tv_nsec << std::endl;
>   }
>   if (argc == 2 && argv[1][0] == 'r') {
> 	// Run tests in reverse order.
> 	for (int i = 0; i < N_TESTS / 2; ++i) {
>   	std::swap(tests[i], tests[N_TESTS - 1 - i]);
>   	std::swap(test_names[i], test_names[N_TESTS - 1 - i]);
> 	}
>   }
>   for (int i = 0; i < OUTER_ITERS; ++i) {
> 	// Alternate tests to minimize bias due to thermal throttling.
> 	for (int j = 0; j < N_TESTS; ++j) {
>   	uint64_t start_time = getnanos();
>   	for (int k = 1; k <= INNER_ITERS; ++k) {
>     	tests[j](k); // Provides memory accesses between tests.
>   	}
>   	// Ignore first iteration for all tests. The first iteration of the first test is
>   	// empirically slightly slower.
>   	if (i != 0) {
>     	total_time[j] += getnanos() - start_time;
>   	}
>   	if ((tests[j] == test1 ? the_volatile : the_atomic.load()) != INNER_ITERS) {
>     	std::cerr << "result check failed, test = " << j << ", " << the_volatile << std::endl;
>     	exit(1);
>   	}
> 	}
>   }
>   for (int i = 0; i < N_TESTS; ++i) {
> 	double nsecs_per_iter = (double) total_time[i] / INNER_ITERS / (OUTER_ITERS - 1);
> 	std::cout << test_names[i] << " took " << nsecs_per_iter << " nseconds per iteration\n";
>   }
>   exit(0);
> }
> 
> [4] Hans Boehm Raw Microbenchmark Results
> Intel(R) Core(TM) i7-8650U (sanity check only):
> 
> hboehm@hboehm-glaptop0:~/tests$ ./a.out
> nsec resolution = 1
> control took 1.34812 nseconds per iteration
> release store took 1.30038 nseconds per iteration
> store with release fence took 1.2933 nseconds per iteration
> store with two fences took 18.0474 nseconds per iteration
> 
> Cortex A53 (Raspberry pi)
> hboehm@rpi3-20210823:~/tests$ ./a.out
> nsec resolution = 1
> control took 7.15224 nseconds per iteration
> release store took 10.7282 nseconds per iteration
> store with release fence took 7.15221 nseconds per iteration
> store with two fences took 10.013 nseconds per iteration
> hboehm@rpi3-20210823:~/tests$ ./a.out -r
> nsec resolution = 1
> control took 7.15227 nseconds per iteration
> release store took 10.7282 nseconds per iteration
> store with release fence took 7.16133 nseconds per iteration
> store with two fences took 10.034 nseconds per iteration
> 
> Cortex A55 (Pixel 6 Pro)
> 
> raven:/data/tmp # taskset 0f ./release-timer
> nsec resolution = 1
> control took 2.77965 nseconds per iteration
> release store took 8.89654 nseconds per iteration
> store with release fence took 4.44787 nseconds per iteration
> store with two fences took 7.78331 nseconds per iteration
> raven:/data/tmp # taskset 0f ./release-timer -r                                                                 	 
> nsec resolution = 1
> control took 2.78024 nseconds per iteration
> release store took 8.89574 nseconds per iteration
> store with release fence took 4.44844 nseconds per iteration
> store with two fences took 7.78428 nseconds per iteration
> 
> Cortex A76 (Pixel 6 Pro)
> raven:/data/tmp # taskset 30 ./release-timer -r                                                                 	 
> nsec resolution = 1
> control took 1.77686 nseconds per iteration
> release store took 1.81081 nseconds per iteration
> store with release fence took 5.3301 nseconds per iteration
> store with two fences took 8.88346 nseconds per iteration
> raven:/data/tmp # taskset 30 ./release-timer                                                                   	 
> nsec resolution = 1
> control took 1.78021 nseconds per iteration
> release store took 1.86095 nseconds per iteration
> store with release fence took 5.33088 nseconds per iteration
> store with two fences took 8.88462 nseconds per iteration
> 
> Cortex X1 (Pixel 6 Pro)
> raven:/data/tmp # taskset c0 ./release-timer                                                                   	 
> nsec resolution = 1
> control took 2.14252 nseconds per iteration
> release store took 2.14258 nseconds per iteration
> store with release fence took 4.32982 nseconds per iteration
> store with two fences took 7.05234 nseconds per iteration
> raven:/data/tmp # taskset c0 ./release-timer -r                                                                 	 
> nsec resolution = 1
> control took 2.14254 nseconds per iteration
> release store took 2.14251 nseconds per iteration
> store with release fence took 4.3273 nseconds per iteration
> store with two fences took 7.05239 nseconds per iteration
> 
> [5] Hans Boehm almost-all-load Microbenchmark:
> // Copyright 2023 Google LLC.
> // SPDX-License-Identifier: Apache-2.0
> 
> #include <atomic>
> #include <iostream>
> #include <time.h>
> 
> static constexpr int INNER_ITERS = 10'000'000;
> static constexpr int OUTER_ITERS = 20;
> static constexpr int N_TESTS = 4;
> 
> volatile int the_volatile(17);
> std::atomic<int> the_atomic(17);
> 
> int test1() {
>   return the_volatile;
> }
> 
> int test2() {
>   return the_atomic.load(std::memory_order_acquire);
> }
> 
> int test3() {
>   int result = the_atomic.load(std::memory_order_relaxed);
>   atomic_thread_fence(std::memory_order_acquire);
>   return result;
> }
> 
> int test4() {
>   atomic_thread_fence(std::memory_order_seq_cst);
>   int result = the_atomic.load(std::memory_order_relaxed);
>   atomic_thread_fence(std::memory_order_acquire);
>   return result;
> }
> 
> typedef int (*int_func)();
> 
> uint64_t getnanos() {
>   struct timespec result;
>   if (clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &result) != 0) {
> 	std::cerr << "clock_gettime() failed\n";
> 	exit(1);
>   }
>   return (uint64_t)result.tv_nsec + 1'000'000'000 * (uint64_t)result.tv_sec;
> }
> 
> int_func tests[N_TESTS] = { test1, test2, test3, test4 };
> const char *test_names[N_TESTS] =
> 	{ "control", "acquire load", "load with acquire fence", "load with two fences" };
> uint64_t total_time[N_TESTS] = { 0 };
> 
> uint sum, last_sum = 0;
> 
> int main(int argc, char **argv) {
>   struct timespec res;
>   if (clock_getres(CLOCK_PROCESS_CPUTIME_ID, &res) != 0) {
> 	std::cerr << "clock_getres() failed\n";
> 	exit(1);
>   } else {
> 	std::cout << "nsec resolution = " << res.tv_nsec << std::endl;
>   }
>   if (argc == 2 && argv[1][0] == 'r') {
> 	// Run tests in reverse order.
> 	for (int i = 0; i < N_TESTS / 2; ++i) {
>   	std::swap(tests[i], tests[N_TESTS - 1 - i]);
>   	std::swap(test_names[i], test_names[N_TESTS - 1 - i]);
> 	}
>   }
>   for (int i = 0; i < OUTER_ITERS; ++i) {
> 	// Alternate tests to minimize bias due to thermal throttling.
> 	for (int j = 0; j < N_TESTS; ++j) {
>   	sum = 0;
>   	uint64_t start_time = getnanos();
>   	for (int k = 0; k < INNER_ITERS; ++k) {
>     	sum += tests[j](); // Provides memory accesses between tests.
>   	}
>   	// Ignore first iteration for all tests. The first iteration of the first test is
>   	// empirically slightly slower.
>   	if (i != 0) {
>     	total_time[j] += getnanos() - start_time;
>   	}
>   	if (sum == 0 || last_sum != 0 && sum != last_sum) {
>     	std::cerr << "result check failed";
>     	exit(1);
>   	}
>   	last_sum = sum;
> 	}
>   }
>   for (int i = 0; i < N_TESTS; ++i) {
> 	double nsecs_per_iter = (double) total_time[i] / INNER_ITERS / (OUTER_ITERS - 1);
> 	std::cout << test_names[i] << " took " << nsecs_per_iter << " nseconds per iteration\n";
>   }
>   exit(0);
> }
> 
> Patrick O'Neill (11):
>   RISC-V: Eliminate SYNC memory models
>   RISC-V: Enforce Libatomic LR/SC SEQ_CST
>   RISC-V: Enforce subword atomic LR/SC SEQ_CST
>   RISC-V: Enforce atomic compare_exchange SEQ_CST
>   RISC-V: Add AMO release bits
>   RISC-V: Strengthen atomic stores
>   RISC-V: Eliminate AMO op fences
>   RISC-V: Weaken LR/SC pairs
>   RISC-V: Weaken mem_thread_fence
>   RISC-V: Weaken atomic loads
>   RISC-V: Table A.6 conformance tests
> 
>  gcc/config/riscv/riscv-protos.h               |   3 +
>  gcc/config/riscv/riscv.cc                     |  66 ++++--
>  gcc/config/riscv/sync.md                      | 194 ++++++++++++------
>  .../riscv/amo-table-a-6-amo-add-1.c           |   8 +
>  .../riscv/amo-table-a-6-amo-add-2.c           |   8 +
>  .../riscv/amo-table-a-6-amo-add-3.c           |   8 +
>  .../riscv/amo-table-a-6-amo-add-4.c           |   8 +
>  .../riscv/amo-table-a-6-amo-add-5.c           |   8 +
>  .../riscv/amo-table-a-6-compare-exchange-1.c  |  10 +
>  .../riscv/amo-table-a-6-compare-exchange-2.c  |  10 +
>  .../riscv/amo-table-a-6-compare-exchange-3.c  |  10 +
>  .../riscv/amo-table-a-6-compare-exchange-4.c  |  10 +
>  .../riscv/amo-table-a-6-compare-exchange-5.c  |  10 +
>  .../riscv/amo-table-a-6-compare-exchange-6.c  |  11 +
>  .../riscv/amo-table-a-6-compare-exchange-7.c  |  10 +
>  .../gcc.target/riscv/amo-table-a-6-fence-1.c  |   8 +
>  .../gcc.target/riscv/amo-table-a-6-fence-2.c  |  10 +
>  .../gcc.target/riscv/amo-table-a-6-fence-3.c  |  10 +
>  .../gcc.target/riscv/amo-table-a-6-fence-4.c  |  10 +
>  .../gcc.target/riscv/amo-table-a-6-fence-5.c  |  10 +
>  .../gcc.target/riscv/amo-table-a-6-load-1.c   |   9 +
>  .../gcc.target/riscv/amo-table-a-6-load-2.c   |  11 +
>  .../gcc.target/riscv/amo-table-a-6-load-3.c   |  11 +
>  .../gcc.target/riscv/amo-table-a-6-store-1.c  |   9 +
>  .../gcc.target/riscv/amo-table-a-6-store-2.c  |  11 +
>  .../riscv/amo-table-a-6-store-compat-3.c      |  11 +
>  .../riscv/amo-table-a-6-subword-amo-add-1.c   |   9 +
>  .../riscv/amo-table-a-6-subword-amo-add-2.c   |   9 +
>  .../riscv/amo-table-a-6-subword-amo-add-3.c   |   9 +
>  .../riscv/amo-table-a-6-subword-amo-add-4.c   |   9 +
>  .../riscv/amo-table-a-6-subword-amo-add-5.c   |   9 +
>  gcc/testsuite/gcc.target/riscv/pr89835.c      |   9 +
>  libgcc/config/riscv/atomic.c                  |   4 +-
>  33 files changed, 467 insertions(+), 75 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-amo-add-1.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-amo-add-2.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-amo-add-3.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-amo-add-4.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-amo-add-5.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-compare-exchange-1.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-compare-exchange-2.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-compare-exchange-3.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-compare-exchange-4.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-compare-exchange-5.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-compare-exchange-6.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-compare-exchange-7.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-fence-1.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-fence-2.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-fence-3.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-fence-4.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-fence-5.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-load-1.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-load-2.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-load-3.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-store-1.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-store-2.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-store-compat-3.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-subword-amo-add-1.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-subword-amo-add-2.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-subword-amo-add-3.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-subword-amo-add-4.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/amo-table-a-6-subword-amo-add-5.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/pr89835.c

These changes address and fix all the issues I reported/I'm aware of,
thank you!

Tested-by: Andrea Parri <andrea@rivosinc.com>

  Andrea
  

On 4/27/23 10:22, Patrick O'Neill wrote:
> This patchset aims to make the RISCV atomics implementation stronger
> than the recommended mapping present in table A.6 of the ISA manual.
> https://github.com/riscv/riscv-isa-manual/blob/c7cf84547b3aefacab5463add1734c1602b67a49/src/memory.tex#L1083-L1157
> 
> Context
> ---------
> GCC defined RISC-V mappings [1] before the Memory Model task group
> finalized their work and provided the ISA Manual Table A.6/A.7 mappings[2].
> 
> For at least a year now, we've known that the mappings were different,
> but it wasn't clear if these unique mappings had correctness issues.
> 
> Andrea Parri found an issue with the GCC mappings, showing that
> atomic_compare_exchange_weak_explicit(-,-,-,release,relaxed) mappings do
> not enforce release ordering guarantees. (Meaning the GCC mappings have
> a correctness issue).
>    https://inbox.sourceware.org/gcc-patches/Y1GbJuhcBFpPGJQ0@andrea/
Right.  I recall this discussion, but thanks for the back reference.

> 
> Why not A.6?
> ---------
> We can update our mappings now, so the obvious choice would be to
> implement Table A.6 (what LLVM implements/ISA manual recommends).
> 
> The reason why that isn't the best path forward for GCC is due to a
> proposal by Hans Boehm to add L{d|w|b|h}.aq/rl and S{d|w|b|h}.aq/rl.
> 
> For context, there is discussion about fast-tracking the addition of
> these instructions. The RISCV architectural review committee supports
> adopting a "new and common atomics ABI for gcc and LLVM toochains ...
> that assumes the addition of the preceding instructions”. That common
> ABI is likely to be A.7.
>    https://lists.riscv.org/g/tech-privileged/message/1284
> 
> Transitioning from A.6 to A.7 will cause an ABI break. We can hedge
> against that risk by emitting a conservative fence after SEQ_CST stores
> to make the mapping compatible with both A.6 and A.7.
So I like that we can have compatible sequences across A.6 and A.7.  Of 
course the concern is performance ;-)


> 
> What does a mapping compatible with both A.6 & A.7 look like?
> ---------
> It is exactly the same as Table A.6, but SEQ_CST stores have a trailing
> fence rw,rw. It's strictly stronger than Table A.6.
Right.  So my worry here is silicon that is either already available or 
coming online shortly.   Those implementations simply aren't going to be 
able to use the A.7 mapping, so they pay a penalty.  Does it make sense 
to have the compatibility fences conditional?



> 
> Benchmark Interpretation
> --------
> As expected, out of order machines are significantly faster with the
> REL_STORE mappings. Unexpectedly, the in-order machines are
> significantly slower with REL_STORE rather than REL_STORE_FENCE.
Yea, that's a bit of a surprise.

> 
> Most machines in the wild are expected to use Table A.7 once the
> instructions are introduced.
> Incurring this added cost now will make it easier for compiled RISC-V
> binaries to transition to the A.7 memory model mapping.
> 
> The performance benefits of moving to A.7 can be more clearly seen using
> an almost-all-load microbenchmark (included on page 3 of Hans’
> proposal). The code for that microbenchmark is attached below [5].
>    https://lists.riscv.org/g/tech-unprivileged/attachment/382/0/load-acquire110422.pdf
>    https://lists.riscv.org/g/tech-unprivileged/topic/92916241
Yea.  I'm not questioning the value of the new instructions that are on 
the horizon, just the value of trying to make everything A.7 compatible.


> 
> Conformance test cases notes
> --------
> The conformance tests in this patch are a good sanity check but do not
> guarantee exactly following Table A.6. It checks that the right
> instructions are emitted (ex. fence rw,r) but not the order of those
> instructions.
Note there is a way to check ordering as well.  You might look at the 
check-function-bodies approach.  I think there are some recent examples 
in the gcc risc-v specific tests.


> 
> LLVM mapping notes
> --------
> LLVM emits corresponding fences for atomic_signal_fence instructions.
> This seems to be an oversight since AFAIK atomic_signal_fence acts as a
> compiler directive. GCC does not emit any fences for atomic_signal_fence
> instructions.
This starts to touch on a larger concern.  Specifically I'd really like 
the two compilers to be compatible in terms of the code they generate 
for the various atomics.

What I worry about is code being written (by design or accident) that is 
dependent on the particular behavior of one compiler and then if that 
code gets built with the other compiler, and we end up different 
behavior.  Worse yet, if/when this happens, it's likely to be tough to 
expose, reproduce & debug.

Do you have any sense of where Clang/LLVM is going to go WRT providing 
an A.6 mapping that is compatible with A.7 by using the additional fences?


Jeff
  

On Fri, 28 Apr 2023 09:14:00 PDT (-0700), jeffreyalaw@gmail.com wrote:
>
>
> On 4/27/23 10:22, Patrick O'Neill wrote:
>> This patchset aims to make the RISCV atomics implementation stronger
>> than the recommended mapping present in table A.6 of the ISA manual.
>> https://github.com/riscv/riscv-isa-manual/blob/c7cf84547b3aefacab5463add1734c1602b67a49/src/memory.tex#L1083-L1157
>>
>> Context
>> ---------
>> GCC defined RISC-V mappings [1] before the Memory Model task group
>> finalized their work and provided the ISA Manual Table A.6/A.7 mappings[2].
>>
>> For at least a year now, we've known that the mappings were different,
>> but it wasn't clear if these unique mappings had correctness issues.
>>
>> Andrea Parri found an issue with the GCC mappings, showing that
>> atomic_compare_exchange_weak_explicit(-,-,-,release,relaxed) mappings do
>> not enforce release ordering guarantees. (Meaning the GCC mappings have
>> a correctness issue).
>>    https://inbox.sourceware.org/gcc-patches/Y1GbJuhcBFpPGJQ0@andrea/
> Right.  I recall this discussion, but thanks for the back reference.

Yep, and it's an important one: that's why we're calling the change a 
bug fix and dropping the current GCC mappings.  If we didn't have the 
bug we'd be talking about an ABI break, and since the GCC mappings 
predate the ISA mappings we'd likely need an additional compatibility 
mode.

So I guess we're lucky that we have a concurrency bug.  I think it's the 
first time I've said that ;)

>> Why not A.6?
>> ---------
>> We can update our mappings now, so the obvious choice would be to
>> implement Table A.6 (what LLVM implements/ISA manual recommends).
>>
>> The reason why that isn't the best path forward for GCC is due to a
>> proposal by Hans Boehm to add L{d|w|b|h}.aq/rl and S{d|w|b|h}.aq/rl.
>>
>> For context, there is discussion about fast-tracking the addition of
>> these instructions. The RISCV architectural review committee supports
>> adopting a "new and common atomics ABI for gcc and LLVM toochains ...
>> that assumes the addition of the preceding instructions”. That common
>> ABI is likely to be A.7.
>>    https://lists.riscv.org/g/tech-privileged/message/1284
>>
>> Transitioning from A.6 to A.7 will cause an ABI break. We can hedge
>> against that risk by emitting a conservative fence after SEQ_CST stores
>> to make the mapping compatible with both A.6 and A.7.
> So I like that we can have compatible sequences across A.6 and A.7.  Of
> course the concern is performance ;-)
>
>
>>
>> What does a mapping compatible with both A.6 & A.7 look like?
>> ---------
>> It is exactly the same as Table A.6, but SEQ_CST stores have a trailing
>> fence rw,rw. It's strictly stronger than Table A.6.
> Right.  So my worry here is silicon that is either already available or
> coming online shortly.   Those implementations simply aren't going to be
> able to use the A.7 mapping, so they pay a penalty.  Does it make sense
> to have the compatibility fences conditional?

IIRC this was discussed somewhere in some thread, but I think there's 
really three ABIs that could be implemented here (ignoring the current 
GCC mappings as they're broken):

* ABI compatible with the current mappings in the ISA manual (A.6).  
  This will presumably perform best on extant hardware, given that it's 
  what the words in the PDF say to do.
* ABI compatible with the proposed mappings for the ISA manual (A.7).  
  This may perform better on new hardware.
* ABI compatible with both A.6 and A.7.  This is likely slow on both new 
  and old hardware, but allows cross-linking.  If there's no performance 
  issues this would be the only mode we need, but that seems unlikely.

IMO those should be encoded somewhere in the ELF.  I'd just do it as two 
bits in the header, but last time I proposed header bits the psABI folks 
wanted to do something different.  I don't think where we encode this 
matters all that much, but if we're doing to treat these as real 
long-term ABIs we should have some way to encode that.

There's also the orthogonal axis of whether we use the new instructions.  
Those aren't in specs yet so I think we can hold off on them for a bit, 
but they're the whole point of doing the ABI break so we should at least 
think them over.  I think we're OK because we've just split out the ABI 
from the ISA here, but I'm not sure if I'm missing something.

Now that I wrote that, though, I remember talking to Patrick about it 
and we drew a bunch of stuff on the whiteboard and then got confused.  
So sorry if I'm just out of the loop here...

>
>
>
>>
>> Benchmark Interpretation
>> --------
>> As expected, out of order machines are significantly faster with the
>> REL_STORE mappings. Unexpectedly, the in-order machines are
>> significantly slower with REL_STORE rather than REL_STORE_FENCE.
> Yea, that's a bit of a surprise.
>
>>
>> Most machines in the wild are expected to use Table A.7 once the
>> instructions are introduced.
>> Incurring this added cost now will make it easier for compiled RISC-V
>> binaries to transition to the A.7 memory model mapping.
>>
>> The performance benefits of moving to A.7 can be more clearly seen using
>> an almost-all-load microbenchmark (included on page 3 of Hans’
>> proposal). The code for that microbenchmark is attached below [5].
>>    https://lists.riscv.org/g/tech-unprivileged/attachment/382/0/load-acquire110422.pdf
>>    https://lists.riscv.org/g/tech-unprivileged/topic/92916241
> Yea.  I'm not questioning the value of the new instructions that are on
> the horizon, just the value of trying to make everything A.7 compatible.
>
>
>>
>> Conformance test cases notes
>> --------
>> The conformance tests in this patch are a good sanity check but do not
>> guarantee exactly following Table A.6. It checks that the right
>> instructions are emitted (ex. fence rw,r) but not the order of those
>> instructions.
> Note there is a way to check ordering as well.  You might look at the
> check-function-bodies approach.  I think there are some recent examples
> in the gcc risc-v specific tests.
>
>
>>
>> LLVM mapping notes
>> --------
>> LLVM emits corresponding fences for atomic_signal_fence instructions.
>> This seems to be an oversight since AFAIK atomic_signal_fence acts as a
>> compiler directive. GCC does not emit any fences for atomic_signal_fence
>> instructions.
> This starts to touch on a larger concern.  Specifically I'd really like
> the two compilers to be compatible in terms of the code they generate
> for the various atomics.
>
> What I worry about is code being written (by design or accident) that is
> dependent on the particular behavior of one compiler and then if that
> code gets built with the other compiler, and we end up different
> behavior.  Worse yet, if/when this happens, it's likely to be tough to
> expose, reproduce & debug.
>
> Do you have any sense of where Clang/LLVM is going to go WRT providing
> an A.6 mapping that is compatible with A.7 by using the additional fences?
>
>
> Jeff
  

On 4/28/23 09:29, Palmer Dabbelt wrote:
> On Fri, 28 Apr 2023 09:14:00 PDT (-0700), jeffreyalaw@gmail.com wrote:
>>
>>
>> On 4/27/23 10:22, Patrick O'Neill wrote:
>>> This patchset aims to make the RISCV atomics implementation stronger
>>> than the recommended mapping present in table A.6 of the ISA manual.
>>> https://github.com/riscv/riscv-isa-manual/blob/c7cf84547b3aefacab5463add1734c1602b67a49/src/memory.tex#L1083-L1157 
>>>
>>>
>>> Context
>>> ---------
>>> GCC defined RISC-V mappings [1] before the Memory Model task group
>>> finalized their work and provided the ISA Manual Table A.6/A.7 
>>> mappings[2].
>>>
>>> For at least a year now, we've known that the mappings were different,
>>> but it wasn't clear if these unique mappings had correctness issues.
>>>
>>> Andrea Parri found an issue with the GCC mappings, showing that
>>> atomic_compare_exchange_weak_explicit(-,-,-,release,relaxed) 
>>> mappings do
>>> not enforce release ordering guarantees. (Meaning the GCC mappings have
>>> a correctness issue).
>>> https://inbox.sourceware.org/gcc-patches/Y1GbJuhcBFpPGJQ0@andrea/
>> Right.  I recall this discussion, but thanks for the back reference.
>
> Yep, and it's an important one: that's why we're calling the change a 
> bug fix and dropping the current GCC mappings.  If we didn't have the 
> bug we'd be talking about an ABI break, and since the GCC mappings 
> predate the ISA mappings we'd likely need an additional compatibility 
> mode.
>
> So I guess we're lucky that we have a concurrency bug.  I think it's 
> the first time I've said that ;)
>
>>> Why not A.6?
>>> ---------
>>> We can update our mappings now, so the obvious choice would be to
>>> implement Table A.6 (what LLVM implements/ISA manual recommends).
>>>
>>> The reason why that isn't the best path forward for GCC is due to a
>>> proposal by Hans Boehm to add L{d|w|b|h}.aq/rl and S{d|w|b|h}.aq/rl.
>>>
>>> For context, there is discussion about fast-tracking the addition of
>>> these instructions. The RISCV architectural review committee supports
>>> adopting a "new and common atomics ABI for gcc and LLVM toochains ...
>>> that assumes the addition of the preceding instructions”. That common
>>> ABI is likely to be A.7.
>>>    https://lists.riscv.org/g/tech-privileged/message/1284
>>>
>>> Transitioning from A.6 to A.7 will cause an ABI break. We can hedge
>>> against that risk by emitting a conservative fence after SEQ_CST stores
>>> to make the mapping compatible with both A.6 and A.7.
>> So I like that we can have compatible sequences across A.6 and A.7.  Of
>> course the concern is performance ;-)
>>
>>
>>>
>>> What does a mapping compatible with both A.6 & A.7 look like?
>>> ---------
>>> It is exactly the same as Table A.6, but SEQ_CST stores have a trailing
>>> fence rw,rw. It's strictly stronger than Table A.6.
>> Right.  So my worry here is silicon that is either already available or
>> coming online shortly.   Those implementations simply aren't going to be
>> able to use the A.7 mapping, so they pay a penalty.  Does it make sense
>> to have the compatibility fences conditional?
>
> IIRC this was discussed somewhere in some thread, but I think there's 
> really three ABIs that could be implemented here (ignoring the current 
> GCC mappings as they're broken):
>
> * ABI compatible with the current mappings in the ISA manual (A.6).  
>  This will presumably perform best on extant hardware, given that it's 
>  what the words in the PDF say to do.
> * ABI compatible with the proposed mappings for the ISA manual (A.7).  
>  This may perform better on new hardware.
> * ABI compatible with both A.6 and A.7.  This is likely slow on both 
> new  and old hardware, but allows cross-linking.  If there's no 
> performance  issues this would be the only mode we need, but that 
> seems unlikely.
>
> IMO those should be encoded somewhere in the ELF.  I'd just do it as 
> two bits in the header, but last time I proposed header bits the psABI 
> folks wanted to do something different.  I don't think where we encode 
> this matters all that much, but if we're doing to treat these as real 
> long-term ABIs we should have some way to encode that.
>
> There's also the orthogonal axis of whether we use the new 
> instructions.  Those aren't in specs yet so I think we can hold off on 
> them for a bit, but they're the whole point of doing the ABI break so 
> we should at least think them over.  I think we're OK because we've 
> just split out the ABI from the ISA here, but I'm not sure if I'm 
> missing something.
>
> Now that I wrote that, though, I remember talking to Patrick about it 
> and we drew a bunch of stuff on the whiteboard and then got confused.  
> So sorry if I'm just out of the loop here...
This looks up-to-date with how I understand it.
>>
>>>
>>> Benchmark Interpretation
>>> --------
>>> As expected, out of order machines are significantly faster with the
>>> REL_STORE mappings. Unexpectedly, the in-order machines are
>>> significantly slower with REL_STORE rather than REL_STORE_FENCE.
>> Yea, that's a bit of a surprise.
>>
>>>
>>> Most machines in the wild are expected to use Table A.7 once the
>>> instructions are introduced.
>>> Incurring this added cost now will make it easier for compiled RISC-V
>>> binaries to transition to the A.7 memory model mapping.
>>>
>>> The performance benefits of moving to A.7 can be more clearly seen 
>>> using
>>> an almost-all-load microbenchmark (included on page 3 of Hans’
>>> proposal). The code for that microbenchmark is attached below [5].
>>> https://lists.riscv.org/g/tech-unprivileged/attachment/382/0/load-acquire110422.pdf
>>>    https://lists.riscv.org/g/tech-unprivileged/topic/92916241
>> Yea.  I'm not questioning the value of the new instructions that are on
>> the horizon, just the value of trying to make everything A.7 compatible.
>>
>>
>>>
>>> Conformance test cases notes
>>> --------
>>> The conformance tests in this patch are a good sanity check but do not
>>> guarantee exactly following Table A.6. It checks that the right
>>> instructions are emitted (ex. fence rw,r) but not the order of those
>>> instructions.
>> Note there is a way to check ordering as well.  You might look at the
>> check-function-bodies approach.  I think there are some recent examples
>> in the gcc risc-v specific tests.
I'll check that out - thank you!
>>>
>>> LLVM mapping notes
>>> --------
>>> LLVM emits corresponding fences for atomic_signal_fence instructions.
>>> This seems to be an oversight since AFAIK atomic_signal_fence acts as a
>>> compiler directive. GCC does not emit any fences for 
>>> atomic_signal_fence
>>> instructions.
>> This starts to touch on a larger concern.  Specifically I'd really like
>> the two compilers to be compatible in terms of the code they generate
>> for the various atomics.
>>
>> What I worry about is code being written (by design or accident) that is
>> dependent on the particular behavior of one compiler and then if that
>> code gets built with the other compiler, and we end up different
>> behavior.  Worse yet, if/when this happens, it's likely to be tough to
>> expose, reproduce & debug.
Agreed.

I'll open an issue with LLVM and see what they have to say about this
particular behavior. Ideally we'd have perfectly compatible compilers
(for atomic ops) by the end of this :)

AFAICT GCC hasn't ever been emitting fences for these instructions.
(& This behavior isn't touched by the patchset).
>>
>> Do you have any sense of where Clang/LLVM is going to go WRT providing
>> an A.6 mapping that is compatible with A.7 by using the additional 
>> fences?
I don't - Based on how LLVM initially waited for A.6, I'd speculate that
they would wait for an official compatibility mapping to be added the
PSABI doc or ISA manual before implementing it.

I imagine there will be demand for an ABI-compatible mapping so the ABI
break has a transition plan, but the timeline for LLVM isn't clear to me.

Patrick
>>
>> Jeff
  

On 4/28/23 10:44, Patrick O'Neill wrote:
> On 4/28/23 09:29, Palmer Dabbelt wrote:
>> On Fri, 28 Apr 2023 09:14:00 PDT (-0700), jeffreyalaw@gmail.com wrote:
>>> On 4/27/23 10:22, Patrick O'Neill wrote:
...
>>>> LLVM mapping notes
>>>> --------
>>>> LLVM emits corresponding fences for atomic_signal_fence instructions.
>>>> This seems to be an oversight since AFAIK atomic_signal_fence acts 
>>>> as a
>>>> compiler directive. GCC does not emit any fences for 
>>>> atomic_signal_fence
>>>> instructions.
>>> This starts to touch on a larger concern.  Specifically I'd really like
>>> the two compilers to be compatible in terms of the code they generate
>>> for the various atomics.
>>>
>>> What I worry about is code being written (by design or accident) 
>>> that is
>>> dependent on the particular behavior of one compiler and then if that
>>> code gets built with the other compiler, and we end up different
>>> behavior.  Worse yet, if/when this happens, it's likely to be tough to
>>> expose, reproduce & debug.
> Agreed.
>
> I'll open an issue with LLVM and see what they have to say about this
> particular behavior. Ideally we'd have perfectly compatible compilers
> (for atomic ops) by the end of this :)
>
> AFAICT GCC hasn't ever been emitting fences for these instructions.
> (& This behavior isn't touched by the patchset).
I re-ran the set of tests I was using and couldn't replicate LLVM's
behavior that was noted here. I think I mixed had up atomic_thread_fence
with atomic_signal_fence at some point.

That was the only difference I could find during my testing of this
patchset (other than the strengthened SEQ_CST store), so I think
GCC/LLVM atomics will be fully compatible once this patchset is applied.
  

We're certainly pushing for the same ABI (A.6 + trailing fence on store) in
LLVM as well. I'm about to upload a pull request for the psABI document
that describes this version of the ABI, and a bit of the rationale for it.
I'll attach the current draft here.

I agree that compatibility is critical here, not just across llvm and gcc,
but also with other language implementations. That's part of the reason to
get this correct asap.

I believe that standardizing on A.6 + trailing fence on store, though
initially suboptimal, is by far the best bet to get us to an efficient ABI
in the long term. I expect the A.7 ABI to perform well. A.6, even without
the trailing store fence, has annoyingly expensive seq_cst loads, which I
would really like to get away from.

Hans







On Fri, Apr 28, 2023 at 10:44 AM Patrick O'Neill <patrick@rivosinc.com>
wrote:

> On 4/28/23 09:29, Palmer Dabbelt wrote:
> > On Fri, 28 Apr 2023 09:14:00 PDT (-0700), jeffreyalaw@gmail.com wrote:
> >>
> >>
> >> On 4/27/23 10:22, Patrick O'Neill wrote:
> >>> This patchset aims to make the RISCV atomics implementation stronger
> >>> than the recommended mapping present in table A.6 of the ISA manual.
> >>>
> https://github.com/riscv/riscv-isa-manual/blob/c7cf84547b3aefacab5463add1734c1602b67a49/src/memory.tex#L1083-L1157
> >>>
> >>>
> >>> Context
> >>> ---------
> >>> GCC defined RISC-V mappings [1] before the Memory Model task group
> >>> finalized their work and provided the ISA Manual Table A.6/A.7
> >>> mappings[2].
> >>>
> >>> For at least a year now, we've known that the mappings were different,
> >>> but it wasn't clear if these unique mappings had correctness issues.
> >>>
> >>> Andrea Parri found an issue with the GCC mappings, showing that
> >>> atomic_compare_exchange_weak_explicit(-,-,-,release,relaxed)
> >>> mappings do
> >>> not enforce release ordering guarantees. (Meaning the GCC mappings have
> >>> a correctness issue).
> >>> https://inbox.sourceware.org/gcc-patches/Y1GbJuhcBFpPGJQ0@andrea/
> >> Right.  I recall this discussion, but thanks for the back reference.
> >
> > Yep, and it's an important one: that's why we're calling the change a
> > bug fix and dropping the current GCC mappings.  If we didn't have the
> > bug we'd be talking about an ABI break, and since the GCC mappings
> > predate the ISA mappings we'd likely need an additional compatibility
> > mode.
> >
> > So I guess we're lucky that we have a concurrency bug.  I think it's
> > the first time I've said that ;)
> >
> >>> Why not A.6?
> >>> ---------
> >>> We can update our mappings now, so the obvious choice would be to
> >>> implement Table A.6 (what LLVM implements/ISA manual recommends).
> >>>
> >>> The reason why that isn't the best path forward for GCC is due to a
> >>> proposal by Hans Boehm to add L{d|w|b|h}.aq/rl and S{d|w|b|h}.aq/rl.
> >>>
> >>> For context, there is discussion about fast-tracking the addition of
> >>> these instructions. The RISCV architectural review committee supports
> >>> adopting a "new and common atomics ABI for gcc and LLVM toochains ...
> >>> that assumes the addition of the preceding instructions”. That common
> >>> ABI is likely to be A.7.
> >>>    https://lists.riscv.org/g/tech-privileged/message/1284
> >>>
> >>> Transitioning from A.6 to A.7 will cause an ABI break. We can hedge
> >>> against that risk by emitting a conservative fence after SEQ_CST stores
> >>> to make the mapping compatible with both A.6 and A.7.
> >> So I like that we can have compatible sequences across A.6 and A.7.  Of
> >> course the concern is performance ;-)
> >>
> >>
> >>>
> >>> What does a mapping compatible with both A.6 & A.7 look like?
> >>> ---------
> >>> It is exactly the same as Table A.6, but SEQ_CST stores have a trailing
> >>> fence rw,rw. It's strictly stronger than Table A.6.
> >> Right.  So my worry here is silicon that is either already available or
> >> coming online shortly.   Those implementations simply aren't going to be
> >> able to use the A.7 mapping, so they pay a penalty.  Does it make sense
> >> to have the compatibility fences conditional?
> >
> > IIRC this was discussed somewhere in some thread, but I think there's
> > really three ABIs that could be implemented here (ignoring the current
> > GCC mappings as they're broken):
> >
> > * ABI compatible with the current mappings in the ISA manual (A.6).
> >  This will presumably perform best on extant hardware, given that it's
> >  what the words in the PDF say to do.
> > * ABI compatible with the proposed mappings for the ISA manual (A.7).
> >  This may perform better on new hardware.
> > * ABI compatible with both A.6 and A.7.  This is likely slow on both
> > new  and old hardware, but allows cross-linking.  If there's no
> > performance  issues this would be the only mode we need, but that
> > seems unlikely.
> >
> > IMO those should be encoded somewhere in the ELF.  I'd just do it as
> > two bits in the header, but last time I proposed header bits the psABI
> > folks wanted to do something different.  I don't think where we encode
> > this matters all that much, but if we're doing to treat these as real
> > long-term ABIs we should have some way to encode that.
> >
> > There's also the orthogonal axis of whether we use the new
> > instructions.  Those aren't in specs yet so I think we can hold off on
> > them for a bit, but they're the whole point of doing the ABI break so
> > we should at least think them over.  I think we're OK because we've
> > just split out the ABI from the ISA here, but I'm not sure if I'm
> > missing something.
> >
> > Now that I wrote that, though, I remember talking to Patrick about it
> > and we drew a bunch of stuff on the whiteboard and then got confused.
> > So sorry if I'm just out of the loop here...
> This looks up-to-date with how I understand it.
> >>
> >>>
> >>> Benchmark Interpretation
> >>> --------
> >>> As expected, out of order machines are significantly faster with the
> >>> REL_STORE mappings. Unexpectedly, the in-order machines are
> >>> significantly slower with REL_STORE rather than REL_STORE_FENCE.
> >> Yea, that's a bit of a surprise.
> >>
> >>>
> >>> Most machines in the wild are expected to use Table A.7 once the
> >>> instructions are introduced.
> >>> Incurring this added cost now will make it easier for compiled RISC-V
> >>> binaries to transition to the A.7 memory model mapping.
> >>>
> >>> The performance benefits of moving to A.7 can be more clearly seen
> >>> using
> >>> an almost-all-load microbenchmark (included on page 3 of Hans’
> >>> proposal). The code for that microbenchmark is attached below [5].
> >>>
> https://lists.riscv.org/g/tech-unprivileged/attachment/382/0/load-acquire110422.pdf
> >>>    https://lists.riscv.org/g/tech-unprivileged/topic/92916241
> >> Yea.  I'm not questioning the value of the new instructions that are on
> >> the horizon, just the value of trying to make everything A.7 compatible.
> >>
> >>
> >>>
> >>> Conformance test cases notes
> >>> --------
> >>> The conformance tests in this patch are a good sanity check but do not
> >>> guarantee exactly following Table A.6. It checks that the right
> >>> instructions are emitted (ex. fence rw,r) but not the order of those
> >>> instructions.
> >> Note there is a way to check ordering as well.  You might look at the
> >> check-function-bodies approach.  I think there are some recent examples
> >> in the gcc risc-v specific tests.
> I'll check that out - thank you!
> >>>
> >>> LLVM mapping notes
> >>> --------
> >>> LLVM emits corresponding fences for atomic_signal_fence instructions.
> >>> This seems to be an oversight since AFAIK atomic_signal_fence acts as a
> >>> compiler directive. GCC does not emit any fences for
> >>> atomic_signal_fence
> >>> instructions.
> >> This starts to touch on a larger concern.  Specifically I'd really like
> >> the two compilers to be compatible in terms of the code they generate
> >> for the various atomics.
> >>
> >> What I worry about is code being written (by design or accident) that is
> >> dependent on the particular behavior of one compiler and then if that
> >> code gets built with the other compiler, and we end up different
> >> behavior.  Worse yet, if/when this happens, it's likely to be tough to
> >> expose, reproduce & debug.
> Agreed.
>
> I'll open an issue with LLVM and see what they have to say about this
> particular behavior. Ideally we'd have perfectly compatible compilers
> (for atomic ops) by the end of this :)
>
> AFAICT GCC hasn't ever been emitting fences for these instructions.
> (& This behavior isn't touched by the patchset).
> >>
> >> Do you have any sense of where Clang/LLVM is going to go WRT providing
> >> an A.6 mapping that is compatible with A.7 by using the additional
> >> fences?
> I don't - Based on how LLVM initially waited for A.6, I'd speculate that
> they would wait for an official compatibility mapping to be added the
> PSABI doc or ISA manual before implementing it.
>
> I imagine there will be demand for an ABI-compatible mapping so the ABI
> break has a transition plan, but the timeline for LLVM isn't clear to me.
>
> Patrick
> >>
> >> Jeff
>
  

On 4/28/23 12:45, Hans Boehm wrote:
> We're certainly pushing for the same ABI (A.6 + trailing fence on store) 
> in LLVM as well. I'm about to upload a pull request for the psABI 
> document that describes this version of the ABI, and a bit of the 
> rationale for it. I'll attach the current draft here.
> 
> I agree that compatibility is critical here, not just across llvm and 
> gcc, but also with other language implementations. That's part of the 
> reason to get this correct asap.
> 
> I believe that standardizing on A.6 + trailing fence on store, though 
> initially suboptimal, is by far the best bet to get us to an efficient 
> ABI in the long term. I expect the A.7 ABI to perform well. A.6, even 
> without the trailing store fence, has annoyingly expensive seq_cst 
> loads, which I would really like to get away from.
Thanks for the additional info.  This stuff is well outside my area of 
expertise, so having someone with your background to give key insights 
is definitely appreciated.

jeff