Turn off Store GVN with explaination

2 files changed, 32 insertions, 3 deletions

diff --git a/chapter11/src/main/java/com/seaofnodes/simple/node/StoreNode.java b/chapter11/src/main/java/com/seaofnodes/simple/node/StoreNode.java
index 026d5c50..024307bb 100644
--- a/chapter11/src/main/java/com/seaofnodes/simple/node/StoreNode.java
+++ b/chapter11/src/main/java/com/seaofnodes/simple/node/StoreNode.java
@@ -72,4 +72,12 @@ public class StoreNode extends MemOpNode {
         return false;
     }
 
+
+    // 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.
+    @Override boolean eq(Node n) { return this == n; }
 }
diff --git a/chapter11/src/test/java/com/seaofnodes/simple/Chapter10Test.java b/chapter11/src/test/java/com/seaofnodes/simple/Chapter10Test.java
index 9a5e4677..62c35b8e 100644
--- a/chapter11/src/test/java/com/seaofnodes/simple/Chapter10Test.java
+++ b/chapter11/src/test/java/com/seaofnodes/simple/Chapter10Test.java
@@ -15,6 +15,26 @@ 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 stop = new Parser(src).parse().iterate();
+        for( int arg : new int[]{0,1,2,3} ) {
+            long expected = (arg==1 || arg==2) ? 9 : 10;
+            var obj = (com.seaofnodes.simple.evaluator.Evaluator.Obj)
+                com.seaofnodes.simple.evaluator.Evaluator.evaluate(stop,arg);
+            assertEquals(expected,obj.fields()[0]);
+        }
+    }
+
     @Test public void testDeadReferenceReturn() {
         for( String body : new String[] {
             "return new S; return 0;",
@@ -290,7 +310,7 @@ while( i.x < i.len ) {
 return sum;
 """);
         StopNode stop = parser.parse().iterate();
-        assertEquals("return Phi(Loop16,0,(Phi(Loop,0,(Phi_$2+1))+Phi_sum));", stop.toString());
+        assertEquals("return Phi(Loop16,0,(Phi(Loop,0,(Phi_x+1))+Phi_sum));", stop.toString());
         assertEquals(10L,Evaluator.evaluate(stop,5));
     }
 
@@ -543,7 +563,7 @@ return 0;
         assertEquals(3L,Evaluator.evaluate(stop,3));
     }
 
-    @Test public void testDropStores() {
+    @Test public void testKeepBranchStores() {
         StopNode stop = new Parser("""
             struct S { int x; }
             S s = new S;
@@ -551,7 +571,8 @@ return 0;
             return s;
             """).parse().iterate();
         // Releasing the old aggregate exposes the overwritten initializing Store.
-        assertEquals(1,countMemoryNodes(stop,StoreNode.class,new BitSet()));
+        // Two branch Stores plus the initial zero Store.
+        assertEquals(3,countMemoryNodes(stop,StoreNode.class,new BitSet()));
         assertEquals(2L,((Evaluator.Obj)Evaluator.evaluate(stop,0)).fields()[0]);
         assertEquals(4L,((Evaluator.Obj)Evaluator.evaluate(stop,3)).fields()[0]);
     }