Atomics.load and Atomics.store need to be fully fenced
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Jun 2017 17:58:24 +0000 (17:58 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Jun 2017 17:58:24 +0000 (17:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=172844

Reviewed by Keith Miller.

Source/JavaScriptCore:

Implement fully fenced loads and stores in FTL using AtomicXchgAdd(0, ptr) for the load and
AtomicXchg(value, ptr) for the store.

DFG needed no changes because it implements all atomics using a CAS loop.

AtomicsObject.cpp now uses new Atomic<> API for fully fences loads and stores.

Prior to this change, we used half fences (acquire/release) for atomic loads and stores. This
is not correct according to my current understanding of the SAB memory model, which requires
that atomic operations are SC with respect to everything not just other atomics.

* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileAtomicsReadModifyWrite):
* ftl/FTLOutput.cpp:
(JSC::FTL::Output::atomicWeakCAS):
* ftl/FTLOutput.h:
* runtime/AtomicsObject.cpp:

Source/WTF:

Add loadFullyFenced and storeFullyFenced to Atomic<>.

* wtf/Atomics.h:
(WTF::Atomic::loadFullyFenced):
(WTF::Atomic::storeRelaxed):
(WTF::Atomic::storeFullyFenced):
(WTF::atomicLoadFullyFenced):
(WTF::atomicStoreFullyFenced):

Websites/webkit.org:

Update documentation to say that the canonical way to do fully fenced loads and stores is
AtomicXchgAdd(0, ptr) and AtomicXchg(value, ptr), respectively.

* docs/b3/intermediate-representation.html:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@217722 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Source/JavaScriptCore/runtime/AtomicsObject.cpp
Source/WTF/ChangeLog
Source/WTF/wtf/Atomics.h
Websites/webkit.org/ChangeLog
Websites/webkit.org/docs/b3/intermediate-representation.html

index 3656252..51e9ee9 100644 (file)
@@ -1,3 +1,28 @@
+2017-06-02  Filip Pizlo  <fpizlo@apple.com>
+
+        Atomics.load and Atomics.store need to be fully fenced
+        https://bugs.webkit.org/show_bug.cgi?id=172844
+
+        Reviewed by Keith Miller.
+        
+        Implement fully fenced loads and stores in FTL using AtomicXchgAdd(0, ptr) for the load and
+        AtomicXchg(value, ptr) for the store.
+        
+        DFG needed no changes because it implements all atomics using a CAS loop.
+        
+        AtomicsObject.cpp now uses new Atomic<> API for fully fences loads and stores.
+        
+        Prior to this change, we used half fences (acquire/release) for atomic loads and stores. This
+        is not correct according to my current understanding of the SAB memory model, which requires
+        that atomic operations are SC with respect to everything not just other atomics.
+
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileAtomicsReadModifyWrite):
+        * ftl/FTLOutput.cpp:
+        (JSC::FTL::Output::atomicWeakCAS):
+        * ftl/FTLOutput.h:
+        * runtime/AtomicsObject.cpp:
+
 2017-06-02  Ryan Haddad  <ryanhaddad@apple.com>
 
         Unreviewed, attempt to fix the iOS build after r217711.
index 9924b8f..bd04d57 100644 (file)
@@ -3027,7 +3027,7 @@ private:
             result = sanitizeResult(atomicValue);
             break;
         case AtomicsLoad:
-            atomicValue = loadFromIntTypedArray(pointer, type);
+            atomicValue = m_out.atomicXchgAdd(m_out.int32Zero, pointer, width);
             result = sanitizeResult(atomicValue);
             break;
         case AtomicsOr:
@@ -3035,7 +3035,7 @@ private:
             result = sanitizeResult(atomicValue);
             break;
         case AtomicsStore:
-            atomicValue = m_out.store(args[0], pointer, storeType(type));
+            atomicValue = m_out.atomicXchg(args[0], pointer, width);
             result = args[0];
             break;
         case AtomicsSub:
index df66223..7719075 100644 (file)
@@ -242,7 +242,7 @@ struct LoadFunc {
     template<typename T>
     JSValue operator()(T* ptr, const double*) const
     {
-        return jsNumber(WTF::atomicLoad(ptr));
+        return jsNumber(WTF::atomicLoadFullyFenced(ptr));
     }
 };
 
@@ -264,7 +264,7 @@ struct StoreFunc {
     {
         double valueAsInt = args[0];
         T valueAsT = static_cast<T>(toInt32(valueAsInt));
-        WTF::atomicStore(ptr, valueAsT);
+        WTF::atomicStoreFullyFenced(ptr, valueAsT);
         return jsNumber(valueAsInt);
     }
 };
index 560753e..6885095 100644 (file)
@@ -1,3 +1,19 @@
+2017-06-02  Filip Pizlo  <fpizlo@apple.com>
+
+        Atomics.load and Atomics.store need to be fully fenced
+        https://bugs.webkit.org/show_bug.cgi?id=172844
+
+        Reviewed by Keith Miller.
+        
+        Add loadFullyFenced and storeFullyFenced to Atomic<>.
+
+        * wtf/Atomics.h:
+        (WTF::Atomic::loadFullyFenced):
+        (WTF::Atomic::storeRelaxed):
+        (WTF::Atomic::storeFullyFenced):
+        (WTF::atomicLoadFullyFenced):
+        (WTF::atomicStoreFullyFenced):
+
 2017-06-01  Keith Miller  <keith_miller@apple.com>
 
         Undo rollout in r217638 with bug fix
index 5b26b2a..5dec3db 100644 (file)
@@ -61,8 +61,25 @@ struct Atomic {
     ALWAYS_INLINE T load(std::memory_order order = std::memory_order_seq_cst) const { return value.load(order); }
     
     ALWAYS_INLINE T loadRelaxed() const { return load(std::memory_order_relaxed); }
-
+    
+    // This is a load that simultaneously does a full fence - neither loads nor stores will move
+    // above or below it.
+    ALWAYS_INLINE T loadFullyFenced() const
+    {
+        Atomic<T>* ptr = const_cast<Atomic<T>*>(this);
+        return ptr->exchangeAdd(T());
+    }
+    
     ALWAYS_INLINE void store(T desired, std::memory_order order = std::memory_order_seq_cst) { value.store(desired, order); }
+    
+    ALWAYS_INLINE void storeRelaxed(T desired) { store(desired, std::memory_order_relaxed); }
+
+    // This is a store that simultaneously does a full fence - neither loads nor stores will move
+    // above or below it.
+    ALWAYS_INLINE void storeFullyFenced(T desired)
+    {
+        exchange(desired);
+    }
 
     ALWAYS_INLINE bool compareExchangeWeak(T expected, T desired, std::memory_order order = std::memory_order_seq_cst)
     {
@@ -81,6 +98,8 @@ struct Atomic {
         return value.compare_exchange_weak(expectedOrActual, desired, order_success, order_failure);
     }
 
+    // WARNING: This does not have strong fencing guarantees when it fails. For example, stores could
+    // sink below it in that case.
     ALWAYS_INLINE T compareExchangeStrong(T expected, T desired, std::memory_order order = std::memory_order_seq_cst)
     {
         T expectedOrActual = expected;
@@ -147,12 +166,24 @@ inline T atomicLoad(T* location, std::memory_order order = std::memory_order_seq
 }
 
 template<typename T>
+inline T atomicLoadFullyFenced(T* location)
+{
+    return bitwise_cast<Atomic<T>*>(location)->loadFullyFenced();
+}
+
+template<typename T>
 inline void atomicStore(T* location, T newValue, std::memory_order order = std::memory_order_seq_cst)
 {
     bitwise_cast<Atomic<T>*>(location)->store(newValue, order);
 }
 
 template<typename T>
+inline void atomicStoreFullyFenced(T* location, T newValue)
+{
+    bitwise_cast<Atomic<T>*>(location)->storeFullyFenced(newValue);
+}
+
+template<typename T>
 inline bool atomicCompareExchangeWeak(T* location, T expected, T newValue, std::memory_order order = std::memory_order_seq_cst)
 {
     return bitwise_cast<Atomic<T>*>(location)->compareExchangeWeak(expected, newValue, order);
index 78d9370..ad74d52 100644 (file)
@@ -1,3 +1,15 @@
+2017-06-02  Filip Pizlo  <fpizlo@apple.com>
+
+        Atomics.load and Atomics.store need to be fully fenced
+        https://bugs.webkit.org/show_bug.cgi?id=172844
+
+        Reviewed by Keith Miller.
+        
+        Update documentation to say that the canonical way to do fully fenced loads and stores is
+        AtomicXchgAdd(0, ptr) and AtomicXchg(value, ptr), respectively.
+
+        * docs/b3/intermediate-representation.html:
+
 2017-05-31  Jon Davis  <jond@apple.com>
 
         Reduce Safari Technology Preview Release Notes posts shown on homepage
index 047493e..1211536 100644 (file)
       precisely. The phantom write effects of a fenced load cannot write to anything that is read or
       written after the fence. The phantom read effects of a fenced store cannot read anything that
       is written after the fence.</p>
+    
+    <p>Applying a fence range to a load or store only makes that access a half-fence: load-with-fence
+      is an acquire fence that only prevents later operations from being hoisted and store-with-fence
+      is a release fence that only prevents earlier operations from being sunk. The canonical way to
+      express a fully fenced load (which is fenced in <i>both</i> directions) is to use
+      AtomicXchgAdd(0, ptr) with a non-empty fence range. The canonical way to express a fully fenced
+      store is to use AtomicXchg(value, ptr) with a non-empty fence range.</p>
 
     <h2>Opcodes</h2>