[PATCH] D149486: [RISCV] Strengthen atomic ordering for sequentially consistent stores

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

In D149486#4385719 <https://reviews.llvm.org/D149486#4385719>, @paulkirth wrote:

> Put the trailing fence behind an `experimental` flag.
>
> I'm not sure this is really the best way to go about this. Most of the
> "experimental" flags for RISC-V are defined in the tablegen files, but those
> are mostly extensions whose entire implementation is experimental, which
> doesn't seem to fit well with something that is a fairly minor transform.
>
> Right now the flag also won't be exposed to clang very well. We could add a
> module flag and try to plumb this through clang's tablegen, but that also feels
> pretty wrong.
>
> If possible I'd appreciate some guidance on the best way to acheive this from
> somone with more familiarty with how this should to be done in the RISC-V
> backend.

I'd definitely welcome input from others, but IMHO the way you've done this is fine. I agree that "experimental" is perhaps not the right concept here given it's not an ABI break and is more of an alternate lowering (which of course does have the benefit of compatibility with a future planned atomics ABI). With that in mind,`-enable-seq-cst-fence` would probably be fine.

In terms of plumbing to Clang properly etc, I think the main thought was to find a way to get this landed without being stuck in review limbo, where incremental follow-up patches can then follow (potentially even just flipping this to the default and leaving the old codegen as an option).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149486