[PATCH] D91157: [AArch64] Out-of-line atomics (-moutline-atomics) implementation.

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/docs/Atomics.rst:625
+
+Libcalls: out-of-line atomics
+=============================
----------------
I think this section needs to be put on the end of the section on `__sync_*`. These functions are effectively an aarch64-specific version of the the `__sync` libcalls -- just with the addition of the memory ordering in the function name, instead of assuming seq_cst. All of the same commentary applies otherwise, and clearly distinguishing from the `__atomic_*` calls is important.

Maybe something like:
```
On AArch64, a variant of the __sync_* routines is used which contain the memory order as part of the function name. These routines may determine at runtime whether the single-instruction atomic operations which were introduced as part of AArch64 Large System Extensions "LSE" instruction set are available, or if it needs to fall back to an LL/SC loop.

The following helper functions are implemented in both [.....]
```


================
Comment at: llvm/include/llvm/IR/RuntimeLibcalls.def:547
 
+// Out-of-line atomics libcalls
+#define HLCALLS(A, N)                                                          \
----------------
Maybe just go ahead and define the libcalls up to size 16, even though aarch64 won't define or use the 16-byte functions, other than CAS. Can we come up with a better name for these libfuncs here? "ATOMIC_*" is an unfortunate prefix, since we already use it for the entirely-distinct set of functions above.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2170
+  SmallVector<SDValue, 4> Ops;
+  if (TLI.getLibcallName(LC)) {
+    Ops.append(Node->op_begin() + 2, Node->op_end());
----------------
t.p.northover wrote:
> I think this is a bit of an abuse of the `LibcallName` mechanism. A separate function in `TargetLowering` would probably be better.
I don't think that's odd or unusual -- we often condition libcall availability on getLibcallName != nullptr.

What does strike me here is the (pre-existing) code duplication between this function (DAGTypeLegalizer::ExapndAtomic) and SelectionDAGLegalize::ConvertNodeToLibcall. Not sure what's up with that...


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:451
+                                        MVT VT) {
+  struct OutlineAtomicsLibcalls {
+    Libcall LC[5][4];
----------------
What's the purpose of the struct?


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:473
+#undef LCALL5
+  switch (Opc) {
+  case ISD::ATOMIC_CMP_SWAP: {
----------------
If you moved this switch to the end, you can just have each clause be "return SwpLcalls[ModeN][ModelN];", instead of storing the address of the array.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15653
+    //   http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p0493r1.pdf
+    // (2) low level libgcc and compiler-rt support implemented by:
+    //   min/max outline atomics helpers
----------------
So, hold on -- AArch64 has umin/umax/smin/smax instructions, but libgcc and compiler-rt don't have helpers for those? That seems to be a remarkably unfortunate state of affairs.

Can you fix that, by implementing those functions in the compiler-rt patch, and submitting the same to libgcc?


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.h:476
+  bool outlineAtomics() const {
+    // Don't outline atomics if
+    // subtarget has LSE
----------------
t.p.northover wrote:
> I think something is a bit weird with how your clang-format handles comments. Here and earlier line lengths are about half as long as I'd expect.
I think it'd be clearer to have this simply "return OutlineAtomics;". The only usage that needs to change is  AArch64ISelLowering.cpp L663, and it'd be _clearer_ to have it explicitly say `if (!Subtarget->hasLSE() && Subtarget->outlineAtomics())`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91157