[PATCH] D61052: [compiler-rt][builtins] Implement some fetch-and-x operations for Cortex-M

[Ayke via Phabricator via llvm-commits]

aykevl marked 3 inline comments as done.
aykevl added inline comments.


================
Comment at: lib/builtins/arm/sync-ops.h:52
 
+#if __ARM_ARCH_PROFILE == 'M'
+#define SYNC_OP_8(op) \
----------------
peter.smith wrote:
> As an alternative, which I've seen used on some other toolchains, is to just not define the _8 variants on Cortex-M. These are not widely used and it might be better for a user to get an undefined symbol error so that they could choose to implement their program differently or implement their own _8 variant?
> 
> One of the main advantages of ldrex, strex is that the exclusive monitor that they set on the address can be read by external peripherals, disabling interrupts may not help in this case so it may be best to not use an interrupt disabled version here.   
Perhaps you're right. Especially in low-latency applications, disabling interrupts in library calls that were inserted by the compiler is probably unexpected. I've checked with arm-none-eabi-gcc version 5.4.1 (as packaged in Debian stretch), and it is not included in the provided libc.
So maybe compiler-rt should not implement this? This would mean:
* armv6m provides no atomic operations at all, as the required instructions are missing. They should be implemented by the application in some appropriate way. I think this should be documented.
* armv7m and up provide atomic operations for 32-bit values only.

Or perhaps better: we should teach LLVM to emit the proper instruction sequences inline (for armv7m and up) instead of calling a library function.

Some related reading: https://gcc.gnu.org/wiki/Atomic/GCCMM/LIbrary

> One of the main advantages of ldrex, strex is that the exclusive monitor that they set on the address can be read by external peripherals, disabling interrupts may not help in this case so it may be best to not use an interrupt disabled version here.

The way I read the docs, the exclusive monitor bit can only be reset during this sequence if an exception occurs. In other words, a peripheral might update the memory location making the operation non-atomic (but I don't see a way to make it atomic). Thus it sounds to me like (for example) __sync_fetch_and_add_4 can only be used on normal memory, not memory in the peripheral address space. Interrupts disabled or not won't make a difference.
Source:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0552a/BABHCIHB.html


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D61052