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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits