Remove unnecessary fences from GSI output (#7308)

GlobalStructInference only optimizes gets of immutable fields, so even
if those gets are seqcst, they cannot synchronize with any writes. We
were being overly conservative by emitting fences in place of optimized
seqcst gets before.

Author: tlively

src/passes/GlobalStructInference.cpp

@@ -462,18 +462,12 @@ struct GlobalStructInference : public Pass {
         // the early return above) so that only leaves 1 and 2.
         if (values.size() == 1) {
           // The case of 1 value is simple: trap if the ref is null, and
-          // otherwise return the value. We must also fence if the get was
-          // seqcst. No additional work is necessary for an acquire get because
-          // there cannot have been any writes to this immutable field that it
-          // would synchronize with.
-          Expression* replacement =
-            builder.makeDrop(builder.makeRefAs(RefAsNonNull, curr->ref));
-          if (curr->order == MemoryOrder::SeqCst) {
-            replacement =
-              builder.blockify(replacement, builder.makeAtomicFence());
-          }
-          replaceCurrent(
-            builder.blockify(replacement, getReadValue(values[0])));
+          // otherwise return the value. Since the field is immutable, there
+          // cannot have been any writes to it we must synchonize with, so we do
+          // not need a fence.
+          replaceCurrent(builder.makeSequence(
+            builder.makeDrop(builder.makeRefAs(RefAsNonNull, curr->ref)),
+            getReadValue(values[0])));
           return;
         }
         assert(values.size() == 2);
@@ -499,16 +493,10 @@ struct GlobalStructInference : public Pass {
         // of their execution matters (they may note globals for un-nesting).
         auto* left = getReadValue(values[0]);
         auto* right = getReadValue(values[1]);
-        // Note that we must trap on null, so add a ref.as_non_null here. We
-        // must also add a fence if this get is seqcst. As before, no extra work
-        // is necessary for an acquire get because there cannot be a write it
-        // synchronizes with.
+        // Note that we must trap on null, so add a ref.as_non_null here. As
+        // before, the get cannot have synchronized with anything.
         Expression* getGlobal =
           builder.makeGlobalGet(checkGlobal, wasm.getGlobal(checkGlobal)->type);
-        if (curr->order == MemoryOrder::SeqCst) {
-          getGlobal =
-            builder.makeSequence(builder.makeAtomicFence(), getGlobal);
-        }
         replaceCurrent(builder.makeSelect(
           builder.makeRefEq(builder.makeRefAs(RefAsNonNull, curr->ref),
                             getGlobal),

test/lit/passes/gsi.wast

@@ -2077,10 +2077,7 @@
   ;; CHECK-NEXT:     (ref.as_non_null
   ;; CHECK-NEXT:      (local.get $0)
   ;; CHECK-NEXT:     )
-  ;; CHECK-NEXT:     (block (result (ref $two))
-  ;; CHECK-NEXT:      (atomic.fence)
-  ;; CHECK-NEXT:      (global.get $two-a)
-  ;; CHECK-NEXT:     )
+  ;; CHECK-NEXT:     (global.get $two-a)
   ;; CHECK-NEXT:    )
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:  )
@@ -2099,8 +2096,7 @@
       )
     )
     (drop
-      ;; This requires a fence to maintain its effect on the global order of
-      ;; seqcst operations.
+      ;; Even though this is seqcst, it still can't synchronize with anything.
       (struct.atomic.get $two 0
         (local.get 0)
       )
@@ -2135,7 +2131,6 @@
   ;; CHECK-NEXT:      (local.get $0)
   ;; CHECK-NEXT:     )
   ;; CHECK-NEXT:    )
-  ;; CHECK-NEXT:    (atomic.fence)
   ;; CHECK-NEXT:    (i32.const 42)
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:  )
@@ -2154,8 +2149,7 @@
       )
     )
     (drop
-      ;; This requires a fence to maintain its effect on the global order of
-      ;; seqcst operations.
+      ;; Even though this is seqcst, it still can't synchronize with anything.
       (struct.atomic.get $two-same 0
         (local.get 0)
       )