[PATCH] D130621: [RISCV] Add target feature to force-enable atomics

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Can you spell out in the review comment the difference between __sync and __atomic and why there's a strong preference for one over the other?  Links to details preferred, but enough context to send a reader in the right direction is appreciated.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:414
     setMinCmpXchgSizeInBits(32);
+  } else if (Subtarget.hasForcedAtomics()) {
+    setMaxAtomicSizeInBitsSupported(Subtarget.is64Bit() ? 64 : 32);
----------------
What MinCmpXchgSizeInBits do you want for the forced atomics?  

Once you answer that, this code can probably be rearranged to have a common if body for both A and forced atomics.

Also, use XLenVT


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:933
 
+  if (Subtarget.hasForcedAtomics()) {
+    // Set atomic rmw/cas operations to expand to force __sync libcalls.
----------------
I believe that at least a subset of these goes away if you set setMinCmpXchgSizeInBits properly.  Haven't definitely confirmed, but at glance at the code makes that look likely.  


================
Comment at: llvm/test/CodeGen/RISCV/forced-atomics32.ll:5
+
+define i8 @load8(ptr %p) nounwind {
+; NO-ATOMIC-LABEL: load8:
----------------
You need coverage here for non-seq_cst orderings.  You don't need to repeat it for all sizes, but please make sure to at least add tests for all the orderings at the native width,  


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

https://reviews.llvm.org/D130621