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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Thanks for the update.

In D61052#1483115 <https://reviews.llvm.org/D61052#1483115>, @aykevl wrote:

> If someone could suggest a good way to test this, that would be great. At the moment it has been tested by compiling for armv6m, armv7m, and armv7em and confirming that the add operations appear to work.


While it will be difficult to test for threading problems, it would be possible to test these functions in a single threaded context by adding test cases in compiler-rt/test/builtins/Unit. We'd need to be careful that these only ran when the functions are supported though. Getting these tests to run for native builds would be simple, cortex-m would be more difficult as we'd have to cross compile and run in an emulator.



================
Comment at: lib/builtins/arm/sync-ops.h:14
+ * These builtins are documented here:
+ * https://gcc.gnu.org/onlinedocs/gcc-8.3.0/gcc/_005f_005fsync-Builtins.html
  *
----------------
Probably want to use https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html as this will track the latest release of GCC.


================
Comment at: lib/builtins/arm/sync-ops.h:34
+        bx lr
+#else
 #define SYNC_OP_4(op) \
----------------
Strictly speaking I think we need more guards here, although this would have been a problem prior to this patch.
- ldrex and strex were introduced in ARM state in armv6 (that is processors prior to A, R, M profile split and not to be confused with armv6-m!)
- ldrex and strex were introduced in Thumb state in armv6t2 (arm-1156t2-s).
- Prior to that arm cpus had a SWP instruction that was used to implement an atomic swap, however has been removed in v8.
- disabling interrupts with cpsid is specific to M class.

I think fixing that is probably best done in another patch as I suspect very few people are trying to build compiler-rt for these older platforms.


================
Comment at: lib/builtins/arm/sync-ops.h:52
 
+#if __ARM_ARCH_PROFILE == 'M'
+#define SYNC_OP_8(op) \
----------------
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.   


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D61052