[PATCH] D149488: [RISCV] Use AMOSWAP for 32 and 64-bit atomic stores

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added a comment.

In D149488#4306677 <https://reviews.llvm.org/D149488#4306677>, @jrtc27 wrote:

> I thought this was being abandoned because it has worse performance. Certainly there are codegen regressions here that clearly indicate it being a bad idea.

The proposed change to the ISA, psABI, and the discussion on tech-unprivileged seem to indicate that this may still be profitable. Since this is only a potential optimization, I think it's fine to wait on this "optimization", or forgo it altogether.

And yes, for forced atomics, it indeed looks like there is a regression in codegen.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:15057
+  if (auto *SI = dyn_cast<StoreInst>(I))
+    return !canAmoSwapStoreInst(SI);
+  return false;
----------------
@jrtc27  Maybe we should forgo the optimization when forced atomics are used? As you've pointed out there seem to be some unintended interactions in that configuration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149488