Turn off Store GVN with explaination

3 files changed, 54 insertions, 2 deletions

diff --git a/chapter25/src/main/java/com/seaofnodes/simple/node/StoreNode.java b/chapter25/src/main/java/com/seaofnodes/simple/node/StoreNode.java
index 8a0bf8d4..cc958348 100644
--- a/chapter25/src/main/java/com/seaofnodes/simple/node/StoreNode.java
+++ b/chapter25/src/main/java/com/seaofnodes/simple/node/StoreNode.java
@@ -129,7 +129,7 @@ public class StoreNode extends MemOpNode {
         // peeps can lose the declaration to a flow-sensitive constant field
         // state.  Imported Stores already carry a serialized non-zero width.
         if( _size==0 && (_size= storageType()) != 0 ) {
-            unlock();           // Width participates in GVN identity.
+            unlock();           // Width participates in the cached hash.
             return init();
         }
 
@@ -323,6 +323,13 @@ public class StoreNode extends MemOpNode {
         return null;
     }
 
-    @Override public boolean eq(Node n) { return _size==((StoreNode)n)._size && super.eq(n); }
+    // Do not GVN Stores. Two equal Stores on two of three paths are the easy
+    // failure: sharing them makes GCM place the Store on all three paths.
+    // Sharing is safe only if every exit path goes through an original Store,
+    // or GCM splits the shared Store to keep it on those paths. Stores must
+    // sink, not run on extra paths. GVN does not check this; GCM does not split.
+    // Identity also disables generic Phi factoring of Stores.
+    // The closed-loop Store sink is a separate peep and still applies.
+    @Override public boolean eq(Node n) { return this == n; }
     @Override int hash() { return super.hash() ^ _size; }
 }
diff --git a/chapter25/src/test/java/com/seaofnodes/simple/Chapter10Test.java b/chapter25/src/test/java/com/seaofnodes/simple/Chapter10Test.java
index 924884ac..6892db0c 100644
--- a/chapter25/src/test/java/com/seaofnodes/simple/Chapter10Test.java
+++ b/chapter25/src/test/java/com/seaofnodes/simple/Chapter10Test.java
@@ -6,6 +6,24 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.fail;
 
 public class Chapter10Test {
+    @Test public void testConditionalStoreSharing() {
+        // Sharing the writes on two of three paths must not write on the third.
+        // Return the object so Load forwarding cannot hide the physical Store.
+        String src = """
+            struct S { int x; };
+            !S !a = new S;
+            a.x = 10;
+            if (arg == 1) a.x = 9;
+            else if (arg == 2) a.x = 9;
+            return a;
+            """;
+        var code = new CodeGen(src).driver(CodeGen.Phase.LocalSched);
+        for( int arg : new int[]{0,1,2,3} ) {
+            long expected = (arg==1 || arg==2) ? 9 : 10;
+            assertEquals("Test.S{x="+expected+"}",Eval2.eval(code,arg));
+        }
+    }
+
     @Test public void testDeadReferenceReturn() {
         for( String body : new String[] {
             "return new S; return 0;",
diff --git a/chapter25/src/test/java/com/seaofnodes/simple/Chapter17bTest.java b/chapter25/src/test/java/com/seaofnodes/simple/Chapter17bTest.java
index e85ce31d..08029c4b 100644
--- a/chapter25/src/test/java/com/seaofnodes/simple/Chapter17bTest.java
+++ b/chapter25/src/test/java/com/seaofnodes/simple/Chapter17bTest.java
@@ -8,6 +8,33 @@ import static org.junit.Assert.fail;
 
 public class Chapter17bTest {
 
+    @Test public void testSinkLoopStores() {
+        var code = new CodeGen("""
+            struct S { int x; };
+            !S !a = new S;
+            a.x = 10;
+            int i = 0;
+            while (i < arg) { a.x = i + 1; i++; }
+            return a;
+            """).driver(CodeGen.Phase.LocalSched);
+        int[] stores = {0};
+        code._stop.walk(n -> {
+            // Constructor zero Stores remain in separate private memory.
+            if( n instanceof com.seaofnodes.simple.node.StoreNode st && st._name.equals("x") &&
+                st.val() instanceof com.seaofnodes.simple.node.PhiNode ) {
+                assertEquals(0,st.cfg0().loopDepth()); // One final Store outside the loop.
+                stores[0]++;
+            }
+            return null;
+        });
+        assertEquals(1,stores[0]);
+        for( int arg : new int[]{-1,0,1,4} ) {
+            long expected = arg<=0 ? 10 : arg;
+            assertEquals("Test.S{x="+expected+"}",Eval2.eval(code,arg));
+        }
+    }
+
+
     @Ignore
     @Test
     public void testJig() {