Merge #40 #47

40: interrupt: Optimize restore on AVR and MSP430 r=taiki-e a=taiki-e As we have already been doing for pre-v6 ARM, avoid unneeded branch and mask. https://github.com/taiki-e/portable-atomic/blob/d4b27473dd7d62a6d5a453e78b1629a1ce24d086/src/imp/interrupt/armv4t.rs#L31-L40 47: Add add/sub/and/or/xor methods that do not return previous value r=taiki-e a=taiki-e This adds `Atomic{I,U}*::{add,sub,and,or,xor}` and `AtomicBool::{and,or,xor}` methods. They are equivalent to the corresponding `fetch_*` methods, but do not return the previous value. They are intended for optimization on platforms that implement atomics using inline assembly, such as the MSP430. Currently, optimizations by these methods (add,sub,and,or,xor) are only guaranteed for MSP430; on x86, LLVM can optimize in most cases, so cases, where this would improve things, should be rare. See pftbest/msp430-atomic#7 for the context. cc `@cr1901` `@YuhanLiin` Co-authored-by: Taiki Endo <[email protected]>
taiki-e · Dec 9, 2022 · 6eac1c5 · 6eac1c5
3 parents f7bd827 + edbca26 + 1dbed76
commit 6eac1c5
Show file tree

Hide file tree

Showing 13 changed files with 973 additions and 36 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,10 @@ Note: In this file, do not use the hard wrap in the middle of a sentence for com
 
 ## [Unreleased]
 
+- Add `Atomic{I,U}*::{add,sub,and,or,xor}` and `AtomicBool::{and,or,xor}` methods.
+
+  They are equivalent to the corresponding `fetch_*` methods, but do not return the previous value. They are intended for optimization on platforms that implement atomics using inline assembly, such as the MSP430.
+
 - Various improvements to `portable_atomic_unsafe_assume_single_core` cfg. ([#44](https://github.com/taiki-e/portable-atomic/pull/44))
 
   - Support disabling FIQs on pre-v6 ARM under `portable_atomic_disable_fiq` cfg.

diff --git a/src/imp/atomic128/intrinsics.rs b/src/imp/atomic128/intrinsics.rs
@@ -415,6 +415,7 @@ macro_rules! atomic128 {
         // SAFETY: any data races are prevented by atomic intrinsics.
         unsafe impl Sync for $atomic_type {}
 
+        no_fetch_ops_impl!($atomic_type, $int_type);
         impl $atomic_type {
             #[inline]
             pub(crate) const fn new(v: $int_type) -> Self {

diff --git a/src/imp/atomic128/macros.rs b/src/imp/atomic128/macros.rs
@@ -10,6 +10,7 @@ macro_rules! atomic128 {
         // SAFETY: any data races are prevented by atomic intrinsics.
         unsafe impl Sync for $atomic_type {}
 
+        no_fetch_ops_impl!($atomic_type, $int_type);
         impl $atomic_type {
             #[inline]
             pub(crate) const fn new(v: $int_type) -> Self {
@@ -216,6 +217,7 @@ macro_rules! atomic128 {
         // SAFETY: any data races are prevented by atomic intrinsics.
         unsafe impl Sync for $atomic_type {}
 
+        no_fetch_ops_impl!($atomic_type, $int_type);
         impl $atomic_type {
             #[inline]
             pub(crate) const fn new(v: $int_type) -> Self {

diff --git a/src/imp/core_atomic.rs b/src/imp/core_atomic.rs
@@ -41,6 +41,9 @@ impl AtomicBool {
 }
 #[cfg_attr(portable_atomic_no_cfg_target_has_atomic, cfg(not(portable_atomic_no_atomic_cas)))]
 #[cfg_attr(not(portable_atomic_no_cfg_target_has_atomic), cfg(target_has_atomic = "ptr"))]
+no_fetch_ops_impl!(AtomicBool, bool);
+#[cfg_attr(portable_atomic_no_cfg_target_has_atomic, cfg(not(portable_atomic_no_atomic_cas)))]
+#[cfg_attr(not(portable_atomic_no_cfg_target_has_atomic), cfg(target_has_atomic = "ptr"))]
 impl AtomicBool {
     #[inline]
     #[cfg_attr(all(debug_assertions, not(portable_atomic_no_track_caller)), track_caller)]
@@ -171,6 +174,15 @@ macro_rules! atomic_int {
         pub(crate) struct $atomic_type {
             inner: core::sync::atomic::$atomic_type,
         }
+        #[cfg_attr(
+            portable_atomic_no_cfg_target_has_atomic,
+            cfg(not(portable_atomic_no_atomic_cas))
+        )]
+        #[cfg_attr(
+            not(portable_atomic_no_cfg_target_has_atomic),
+            cfg(target_has_atomic = "ptr")
+        )]
+        no_fetch_ops_impl!($atomic_type, $int_type);
         impl $atomic_type {
             #[inline]
             pub(crate) const fn new(v: $int_type) -> Self {

diff --git a/src/imp/fallback/imp.rs b/src/imp/fallback/imp.rs
@@ -130,6 +130,8 @@ macro_rules! atomic {
         // SAFETY: any data races are prevented by the lock and atomic operation.
         unsafe impl Sync for $atomic_type {}
 
+        #[cfg(any(test, not(portable_atomic_cmpxchg16b_dynamic)))]
+        no_fetch_ops_impl!($atomic_type, $int_type);
         impl $atomic_type {
             #[cfg(any(test, not(portable_atomic_cmpxchg16b_dynamic)))]
             #[inline]

diff --git a/src/imp/interrupt/armv4t.rs b/src/imp/interrupt/armv4t.rs
@@ -51,6 +51,8 @@ pub(super) fn disable() -> State {
 pub(super) unsafe fn restore(State(cpsr): State) {
     // SAFETY: the caller must guarantee that the state was retrieved by the previous `disable`,
     unsafe {
+        // This clobbers the entire CPSR. See msp430.rs to safety on this.
+        //
         // Do not use `nomem` and `readonly` because prevent preceding memory accesses from being reordered after interrupts are enabled.
         asm!("msr cpsr_c, {0}", in(reg) cpsr, options(nostack));
     }

diff --git a/src/imp/interrupt/avr.rs b/src/imp/interrupt/avr.rs
@@ -4,11 +4,11 @@
 use core::arch::asm;
 
 #[derive(Clone, Copy)]
-pub(super) struct WasEnabled(bool);
+pub(super) struct State(u8);
 
 /// Disables interrupts and returns the previous interrupt state.
 #[inline]
-pub(super) fn disable() -> WasEnabled {
+pub(super) fn disable() -> State {
     let sreg: u8;
     // SAFETY: reading the status register (SREG) and disabling interrupts are safe.
     // (see module-level comments of interrupt/mod.rs on the safety of using privileged instructions)
@@ -25,28 +25,25 @@ pub(super) fn disable() -> WasEnabled {
         );
         #[cfg(portable_atomic_no_asm)]
         {
-            llvm_asm!("in $0,0x3F" :"=r"(sreg) ::: "volatile");
+            llvm_asm!("in $0, 0x3F" : "=r"(sreg) ::: "volatile");
             llvm_asm!("cli" ::: "memory" : "volatile");
         }
     }
-    // I (Global Interrupt Enable) bit (1 << 7)
-    WasEnabled(sreg & 0x80 != 0)
+    State(sreg)
 }
 
 /// Restores the previous interrupt state.
 #[inline]
-pub(super) unsafe fn restore(WasEnabled(was_enabled): WasEnabled) {
-    if was_enabled {
-        // SAFETY: the caller must guarantee that the state was retrieved by the previous `disable`,
-        // and we've checked that interrupts were enabled before disabling interrupts.
-        unsafe {
-            // Do not use `nomem` and `readonly` because prevent preceding memory accesses from being reordered after interrupts are enabled.
-            // Do not use `preserves_flags` because SEI modifies the I bit of the status register (SREG).
-            // Refs: https://ww1.microchip.com/downloads/en/DeviceDoc/AVR-InstructionSet-Manual-DS40002198.pdf#page=127
-            #[cfg(not(portable_atomic_no_asm))]
-            asm!("sei", options(nostack));
-            #[cfg(portable_atomic_no_asm)]
-            llvm_asm!("sei" ::: "memory" : "volatile");
-        }
+pub(super) unsafe fn restore(State(sreg): State) {
+    // SAFETY: the caller must guarantee that the state was retrieved by the previous `disable`,
+    unsafe {
+        // This clobbers the entire status register. See msp430.rs to safety on this.
+        //
+        // Do not use `nomem` and `readonly` because prevent preceding memory accesses from being reordered after interrupts are enabled.
+        // Do not use `preserves_flags` because OUT modifies the status register (SREG).
+        #[cfg(not(portable_atomic_no_asm))]
+        asm!("out 0x3F, {0}", in(reg) sreg, options(nostack));
+        #[cfg(portable_atomic_no_asm)]
+        llvm_asm!("out 0x3F, $0" :: "r"(sreg) : "memory" : "volatile");
     }
 }
diff --git a/src/imp/interrupt/mod.rs b/src/imp/interrupt/mod.rs
@@ -23,7 +23,8 @@
 // [^avr2]: https://github.com/llvm/llvm-project/blob/llvmorg-15.0.0/llvm/test/CodeGen/AVR/atomics/load16.ll#L5
 
 // On some platforms, atomic load/store can be implemented in a more efficient
-// way than disabling interrupts.
+// way than disabling interrupts. On MSP430, some RMWs that do not return the
+// previous value can also be optimized.
 //
 // Note: On single-core systems, it is okay to use critical session-based
 // CAS together with atomic load/store. The load/store will not be
@@ -246,6 +247,33 @@ impl AtomicBool {
     }
 }
 
+#[cfg(not(target_arch = "msp430"))]
+no_fetch_ops_impl!(AtomicBool, bool);
+#[cfg(target_arch = "msp430")]
+impl AtomicBool {
+    #[inline]
+    pub(crate) fn and(&self, val: bool, order: Ordering) {
+        // SAFETY: Self and atomic::AtomicBool have the same layout,
+        unsafe {
+            (*(self as *const Self as *const atomic::AtomicBool)).and(val, order);
+        }
+    }
+    #[inline]
+    pub(crate) fn or(&self, val: bool, order: Ordering) {
+        // SAFETY: Self and atomic::AtomicBool have the same layout,
+        unsafe {
+            (*(self as *const Self as *const atomic::AtomicBool)).or(val, order);
+        }
+    }
+    #[inline]
+    pub(crate) fn xor(&self, val: bool, order: Ordering) {
+        // SAFETY: Self and atomic::AtomicBool have the same layout,
+        unsafe {
+            (*(self as *const Self as *const atomic::AtomicBool)).xor(val, order);
+        }
+    }
+}
+
 #[cfg_attr(target_pointer_width = "16", repr(C, align(2)))]
 #[cfg_attr(target_pointer_width = "32", repr(C, align(4)))]
 #[cfg_attr(target_pointer_width = "64", repr(C, align(8)))]
@@ -462,10 +490,52 @@ macro_rules! atomic_int {
                 }
             }
         }
+
+        #[cfg(not(target_arch = "msp430"))]
+        no_fetch_ops_impl!($atomic_type, $int_type);
+        #[cfg(target_arch = "msp430")]
+        impl $atomic_type {
+            #[inline]
+            pub(crate) fn add(&self, val: $int_type, order: Ordering) {
+                // SAFETY: Self and atomic::$atomic_type have the same layout,
+                unsafe {
+                    (*(self as *const Self as *const atomic::$atomic_type)).add(val, order);
+                }
+            }
+            #[inline]
+            pub(crate) fn sub(&self, val: $int_type, order: Ordering) {
+                // SAFETY: Self and atomic::$atomic_type have the same layout,
+                unsafe {
+                    (*(self as *const Self as *const atomic::$atomic_type)).sub(val, order);
+                }
+            }
+            #[inline]
+            pub(crate) fn and(&self, val: $int_type, order: Ordering) {
+                // SAFETY: Self and atomic::$atomic_type have the same layout,
+                unsafe {
+                    (*(self as *const Self as *const atomic::$atomic_type)).and(val, order);
+                }
+            }
+            #[inline]
+            pub(crate) fn or(&self, val: $int_type, order: Ordering) {
+                // SAFETY: Self and atomic::$atomic_type have the same layout,
+                unsafe {
+                    (*(self as *const Self as *const atomic::$atomic_type)).or(val, order);
+                }
+            }
+            #[inline]
+            pub(crate) fn xor(&self, val: $int_type, order: Ordering) {
+                // SAFETY: Self and atomic::$atomic_type have the same layout,
+                unsafe {
+                    (*(self as *const Self as *const atomic::$atomic_type)).xor(val, order);
+                }
+            }
+        }
     };
     (load_store_critical_session, $atomic_type:ident, $int_type:ident, $align:expr) => {
         atomic_int!(base, $atomic_type, $int_type, $align);
         atomic_int!(cas, $atomic_type, $int_type);
+        no_fetch_ops_impl!($atomic_type, $int_type);
         impl $atomic_type {
             #[inline]
             #[cfg_attr(all(debug_assertions, not(portable_atomic_no_track_caller)), track_caller)]

diff --git a/src/imp/interrupt/msp430.rs b/src/imp/interrupt/msp430.rs
@@ -1,16 +1,18 @@
 // Adapted from https://github.com/rust-embedded/msp430.
+//
+// See also src/imp/msp430.rs.
 
 #[cfg(not(portable_atomic_no_asm))]
 use core::arch::asm;
 
 pub(super) use super::super::msp430 as atomic;
 
 #[derive(Clone, Copy)]
-pub(super) struct WasEnabled(bool);
+pub(super) struct State(u16);
 
 /// Disables interrupts and returns the previous interrupt state.
 #[inline]
-pub(super) fn disable() -> WasEnabled {
+pub(super) fn disable() -> State {
     let r: u16;
     // SAFETY: reading the status register and disabling interrupts are safe.
     // (see module-level comments of interrupt/mod.rs on the safety of using privileged instructions)
@@ -31,24 +33,26 @@ pub(super) fn disable() -> WasEnabled {
             llvm_asm!("dint { nop" ::: "memory" : "volatile");
         }
     }
-    // GIE (global interrupt enable) bit (1 << 3)
-    WasEnabled(r & 0x8 != 0)
+    State(r)
 }
 
 /// Restores the previous interrupt state.
 #[inline]
-pub(super) unsafe fn restore(WasEnabled(was_enabled): WasEnabled) {
-    if was_enabled {
-        // SAFETY: the caller must guarantee that the state was retrieved by the previous `disable`,
-        // and we've checked that interrupts were enabled before disabling interrupts.
-        unsafe {
-            // Do not use `nomem` and `readonly` because prevent preceding memory accesses from being reordered after interrupts are enabled.
-            // Do not use `preserves_flags` because EINT modifies the GIE (global interrupt enable) bit of the status register.
-            // Refs: http://mspgcc.sourceforge.net/manual/x951.html
-            #[cfg(not(portable_atomic_no_asm))]
-            asm!("nop {{ eint {{ nop", options(nostack));
-            #[cfg(portable_atomic_no_asm)]
-            llvm_asm!("nop { eint { nop" ::: "memory" : "volatile");
-        }
+pub(super) unsafe fn restore(State(r): State) {
+    // SAFETY: the caller must guarantee that the state was retrieved by the previous `disable`,
+    unsafe {
+        // This clobbers the entire status register, but we never explicitly modify
+        // flags within a critical session, and the only flags that may be changed
+        // within a critical session are the arithmetic flags that are changed as
+        // a side effect of arithmetic operations, etc., which LLVM recognizes,
+        // so it is safe to clobber them here.
+        // See also the discussion at https://github.com/taiki-e/portable-atomic/pull/40.
+        //
+        // Do not use `nomem` and `readonly` because prevent preceding memory accesses from being reordered after interrupts are enabled.
+        // Do not use `preserves_flags` because MOV modifies the status register.
+        #[cfg(not(portable_atomic_no_asm))]
+        asm!("nop {{ mov {0}, R2 {{ nop", in(reg) r, options(nostack));
+        #[cfg(portable_atomic_no_asm)]
+        llvm_asm!("nop { mov $0, R2 { nop" :: "r"(r) : "memory" : "volatile");
     }
 }