Investigate and fix bug in SSA translation

optvm/src/main/java/com/compilerprogramming/ezlang/compiler/BasicBlock.java

@@ -213,6 +213,14 @@ public StringBuilder toDot(StringBuilder sb, boolean verbose) {
         sb.append("</TABLE>");
         return sb;
     }
+    public StringBuilder listDomFrontiers(StringBuilder sb) {
+        sb.append("L").append(this.bid).append(":");
+        for (BasicBlock bb: dominationFrontier) {
+            sb.append(" L").append(bb.bid);
+        }
+        sb.append("\n");
+        return sb;
+    }
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;

optvm/src/main/java/com/compilerprogramming/ezlang/compiler/CompiledFunction.java

@@ -426,7 +426,7 @@ private boolean compileArrayStoreExpr(AST.ArrayStoreExpr arrayStoreExpr) {
     }
 
     private boolean compileNewExpr(AST.NewExpr newExpr) {
-        codeNew(newExpr.type);
+        codeNew(newExpr.type,newExpr.len,newExpr.initValue);
         return false;
     }
 
@@ -644,20 +644,44 @@ else if (indexed instanceof Operand.LoadFieldOperand loadFieldOperand)
             codeMove(value, indexed);
     }
 
-    private void codeNew(Type type) {
+    private void codeNew(Type type, AST.Expr len, AST.Expr initVal) {
         if (type instanceof Type.TypeArray typeArray)
-            codeNewArray(typeArray);
+            codeNewArray(typeArray, len, initVal);
         else if (type instanceof Type.TypeStruct typeStruct)
             codeNewStruct(typeStruct);
         else
             throw new CompilerException("Unexpected type: " + type);
     }
 
-    private void codeNewArray(Type.TypeArray typeArray) {
+    private void codeNewArray(Type.TypeArray typeArray, AST.Expr len, AST.Expr initVal) {
         var temp = createTemp(typeArray);
+        Operand lenOperand = null;
+        Operand initValOperand = null;
+        if (len != null) {
+            boolean indexed = compileExpr(len);
+            if (indexed)
+                codeIndexedLoad();
+            if (initVal != null) {
+                indexed = compileExpr(initVal);
+                if (indexed)
+                    codeIndexedLoad();
+                initValOperand = pop();
+            }
+            lenOperand = pop();
+        }
+        Instruction insn;
         var target = (Operand.RegisterOperand) issa.write(temp);
-        var insn = new Instruction.NewArray(typeArray, target);
+        if (lenOperand != null) {
+            if (initValOperand != null)
+                insn = new Instruction.NewArray(typeArray, target, issa.read(lenOperand), issa.read(initValOperand));
+            else
+                insn = new Instruction.NewArray(typeArray, target, issa.read(lenOperand));
+        }
+        else
+            insn = new Instruction.NewArray(typeArray, target);
         issa.recordDef(target, insn);
+        if (lenOperand != null) issa.recordUse(lenOperand,insn);
+        if (initValOperand != null) issa.recordUse(initValOperand,insn);
         code(insn);
     }
 
@@ -941,6 +965,32 @@ private Register addPhiOperands(Register variable, Instruction.Phi phi) {
             return tryRemovingPhi(phi);
         }
 
+        // The Phi's def is dead so we need to remove
+        // all occurrences of this def from the memoized defs
+        // per Basic Block
+        private void clearDefs(Instruction.Phi phi) {
+            // TODO rethink the data structure for currentDef
+            var def = phi.value();
+            var defs = currentDef.get(def.nonSSAId());
+            // Make a list of block/reg that we need to delete
+            var bbList = new ArrayList<BasicBlock>();
+            var regList = new ArrayList<Register>();
+            for (var entries : defs.entrySet()) {
+                var bb = entries.getKey();
+                var reg = entries.getValue();
+                if (reg.equals(def)) {
+                    bbList.add(bb);
+                    regList.add(reg);
+                }
+            }
+            // Now delete them
+            for (int i = 0; i < bbList.size(); i++) {
+                var bb = bbList.get(i);
+                var reg = regList.get(i);
+                defs.remove(bb, reg);
+            }
+        }
+
         private Register tryRemovingPhi(Instruction.Phi phi) {
             Register same = null;
             // Check if phi has distinct inputs
@@ -967,6 +1017,9 @@ private Register tryRemovingPhi(Instruction.Phi phi) {
             // remove all uses of phi to same and remove phi
             replacePhiValueAndUsers(phi, same);
             phi.block.deleteInstruction(phi);
+            // Since the phi is dead any references to its def
+            // must be removed; this is not mentioned in the paper
+            clearDefs(phi);
             // try to recursively remove all phi users, which might have become trivial
             for (var use: users) {
                 if (use instanceof Instruction.Phi phiuser)
@@ -979,25 +1032,28 @@ private Register tryRemovingPhi(Instruction.Phi phi) {
          * Reroute all uses of phi to new value
          */
         private void replacePhiValueAndUsers(Instruction.Phi phi, Register newValue) {
-            var oldDefUseChain = ssaDefUses.get(phi.value());
+            var oldValue = phi.value();
+            var oldDefUseChain = ssaDefUses.get(oldValue);
             var newDefUseChain = ssaDefUses.get(newValue);
             if (newDefUseChain == null) {
-                // Can be null because this may be existing def
-                newDefUseChain = SSAEdges.addDef(ssaDefUses, newValue, phi);
+                throw new CompilerException("Expected error: undefined var " + newValue);
             }
             if (oldDefUseChain != null) {
                 for (Instruction instruction: oldDefUseChain.useList) {
+                    boolean replaced;
                     if (instruction instanceof Instruction.Phi somePhi) {
-                        somePhi.replaceInput(phi.value(), newValue);
+                        replaced = somePhi.replaceInput(oldValue, newValue);
                     }
                     else {
-                        instruction.replaceUse(phi.value(), newValue);
+                        replaced = instruction.replaceUse(oldValue, newValue);
+                    }
+                    if (!replaced) {
+                        throw new CompilerException("Discrepancy between var use list and var definition");
                     }
                 }
                 // Users of phi old value become users of the new value
                 newDefUseChain.useList.addAll(oldDefUseChain.useList);
                 oldDefUseChain.useList.clear();
-                // FIXME remove old def from def-use chains
             }
         }
 

optvm/src/main/java/com/compilerprogramming/ezlang/compiler/DominatorTree.java

@@ -1,10 +1,8 @@
 package com.compilerprogramming.ezlang.compiler;
 
-import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.HashSet;
 import java.util.List;
-import java.util.function.Consumer;
 
 /**
  * The dominator tree construction algorithm is based on figure 9.24,
@@ -34,14 +32,15 @@ public DominatorTree(BasicBlock entry) {
         populateTree();
         setDepth();
         calculateDominanceFrontiers();
+        //calculateDominanceFrontiersMethod2();
     }
 
     private void calculateDominatorTree() {
         resetDomInfo();
         annotateBlocksWithRPO();
         sortBlocksByRPO();
 
-        // Set IDom entry for root to itself
+        // Set IDom entry for root to itself (see note below)
         entry.idom = entry;
         boolean changed = true;
         while (changed) {
@@ -68,6 +67,11 @@ private void calculateDominatorTree() {
                 }
             }
         }
+        // There is a contradiction between what is described in the book
+        // and the algo - as the book says the IDOM for root is undefined
+        // But we set this to itself when calculating. So after calculations
+        // are done, set this to null. This is technically more correct IMO.
+        entry.idom = null;
     }
 
     private void resetDomInfo() {
@@ -146,7 +150,7 @@ private BasicBlock findFirstPredecessorWithIdom(BasicBlock n) {
     private void populateTree() {
         for (BasicBlock block : blocks) {
             BasicBlock idom = block.idom;
-            if (idom == block) // root
+            if (idom == null) // root
                 continue;
             // add edge from idom to n
             idom.dominatedChildren.add(block);
@@ -166,12 +170,13 @@ private void setDepth() {
      */
     private void setDepth_(BasicBlock block) {
         BasicBlock idom = block.idom;
-        if (idom != block) {
+        if (idom != null) {
             assert idom.domDepth > 0;
             block.domDepth = idom.domDepth + 1;
-        } else {
-            assert idom.domDepth == 1;
-            assert idom.domDepth == block.domDepth;
+        }
+        else {
+            // root (entry) block's idom is null
+            assert block.domDepth == 1;
         }
         for (BasicBlock child : block.dominatedChildren)
             setDepth_(child);
@@ -189,6 +194,8 @@ private void calculateDominanceFrontiers() {
         //          while runner != doms[b]
         //              add b to runner’s dominance frontier set
         //              runner = doms[runner]
+        for (BasicBlock b: blocks)
+            b.dominationFrontier.clear();   // empty set
         for (BasicBlock b : blocks) {
             if (b.predecessors.size() >= 2) {
                 for (BasicBlock p : b.predecessors) {
@@ -204,6 +211,37 @@ private void calculateDominanceFrontiers() {
         }
     }
 
+    // We have an alternative approach to calculating DOM Frontiers to
+    // allow us to validate above
+    private void calculateDominanceFrontiersMethod2()
+    {
+        for (BasicBlock b: blocks)
+            b.dominationFrontier.clear();   // empty set
+        computeDF(entry);
+    }
+
+    // Implementation based on description in pg 440 of
+    // Modern Compiler implementation in C
+    // Appel
+    private void computeDF(BasicBlock n) {
+        var S = new HashSet<BasicBlock>();
+        for (BasicBlock y: n.successors) {
+            if (y.idom != n)
+                S.add(y);
+        }
+        for (BasicBlock c: n.dominatedChildren) {
+            computeDF(c);
+            for (BasicBlock w: c.dominationFrontier) {
+                // Note that the printed book has an error below
+                // and errata gives the correct version
+                if (!n.dominates(w) || n==w) {
+                    S.add(w);
+                }
+            }
+        }
+        n.dominationFrontier.addAll(S);
+    }
+
     public String generateDotOutput() {
         StringBuilder sb = new StringBuilder();
         sb.append("digraph DomTree {\n");
@@ -212,10 +250,18 @@ public String generateDotOutput() {
         }
         for (BasicBlock n : blocks) {
             BasicBlock idom = n.idom;
-            if (idom == n) continue;
+            if (idom == null) continue;
             sb.append(idom.uniqueName()).append("->").append(n.uniqueName()).append(";\n");
         }
         sb.append("}\n");
         return sb.toString();
     }
+
+    public String listDomFrontiers() {
+        StringBuilder sb = new StringBuilder();
+        for (BasicBlock n : blocks) {
+            n.listDomFrontiers(sb);
+        }
+        return sb.toString();
+    }
 }

optvm/src/main/java/com/compilerprogramming/ezlang/compiler/EnterSSA.java

@@ -41,9 +41,17 @@ public EnterSSA(CompiledFunction bytecodeFunction, EnumSet<Options> options) {
             System.out.println("Pre SSA Dominator Tree");
             System.out.println(domTree.generateDotOutput());
         }
+        if (options.contains(Options.DUMP_PRE_SSA_DOMFRONTIERS)) {
+            System.out.println("Pre SSA Dominance Frontiers");
+            System.out.println(domTree.listDomFrontiers());
+        }
         this.blocks = domTree.blocks;   // the blocks are ordered reverse post order
         findNonLocalNames();
         new Liveness(bytecodeFunction); // EWe require liveness info to construct pruned ssa
+        if (options.contains(Options.DUMP_PRE_SSA_LIVENESS)) {
+            System.out.println("Pre SSA Liveness");
+            System.out.println(bytecodeFunction.toStr(new StringBuilder(), true));
+        }
         insertPhis();
         renameVars();
         bytecodeFunction.isSSA = true;
@@ -177,8 +185,9 @@ void search(BasicBlock block) {
         }
         // Pop stacks for defs
         for (Instruction i: block.instructions) {
-            if (i.definesVar()) {
-                var reg = i.def();
+            // Phis don't answer to definesVar() or def()
+            if (i.definesVar() || i instanceof Instruction.Phi) {
+                var reg = i instanceof Instruction.Phi phi ? phi.value() : i.def();
                 stacks[reg.nonSSAId()].pop();
             }
         }

optvm/src/main/java/com/compilerprogramming/ezlang/compiler/Instruction.java

@@ -124,14 +124,28 @@ public NewArray(Type.TypeArray type, Operand.RegisterOperand destOperand) {
             super(I_NEW_ARRAY, destOperand);
             this.type = type;
         }
+        public NewArray(Type.TypeArray type, Operand.RegisterOperand destOperand, Operand len) {
+            super(I_NEW_ARRAY, destOperand, len);
+            this.type = type;
+        }
+        public NewArray(Type.TypeArray type, Operand.RegisterOperand destOperand, Operand len, Operand initValue) {
+            super(I_NEW_ARRAY, destOperand, len, initValue);
+            this.type = type;
+        }
+        public Operand len() { return uses.length > 0 ? uses[0] : null; }
+        public Operand initValue() { return uses.length > 1 ? uses[1] : null; }
         public Operand.RegisterOperand destOperand() { return def; }
         @Override
         public StringBuilder toStr(StringBuilder sb) {
-            return sb.append(def)
+            sb.append(def)
                     .append(" = ")
                     .append("New(")
-                    .append(type)
-                    .append(")");
+                    .append(type);
+            if (len() != null)
+                sb.append(", len=").append(len());
+            if (initValue() != null)
+                sb.append(", initValue=").append(initValue());
+            return sb.append(")");
         }
     }
 
@@ -417,15 +431,18 @@ public void addInput(Register register) {
             newUses[newUses.length-1] = new Operand.RegisterOperand(register);
             this.uses = newUses;
         }
-        public void replaceInput(Register oldReg, Register newReg) {
+        public boolean replaceInput(Register oldReg, Register newReg) {
+            boolean replaced = false;
             for (int i = 0; i < numInputs(); i++) {
                 if (isRegisterInput(i)) {
                     Register in = inputAsRegister(i);
                     if (in.equals(oldReg)) {
                         replaceInput(i, newReg);
+                        replaced = true;
                     }
                 }
             }
+            return replaced;
         }
     }
 

optvm/src/main/java/com/compilerprogramming/ezlang/compiler/Options.java

@@ -10,6 +10,8 @@ public enum Options {
     REGALLOC,
     DUMP_INITIAL_IR,
     DUMP_PRE_SSA_DOMTREE,
+    DUMP_PRE_SSA_DOMFRONTIERS,
+    DUMP_PRE_SSA_LIVENESS,
     DUMP_SSA_IR,
     DUMP_SCCP_PREAPPLY,
     DUMP_SCCP_POSTAPPLY,