Skip to content

Commit 86e5dbd

Browse files
dibyendumajumdardibyendumajumdar
authored
Merge pull request #52 from CompilerProgramming/son21
Investigate and fix bug in SSA translation
2 parents bd293df + 2b02afb commit 86e5dbd

File tree

31 files changed

+2212
-259
lines changed

31 files changed

+2212
-259
lines changed

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,14 @@ public StringBuilder toDot(StringBuilder sb, boolean verbose) {
213213
sb.append("</TABLE>");
214214
return sb;
215215
}
216+
public StringBuilder listDomFrontiers(StringBuilder sb) {
217+
sb.append("L").append(this.bid).append(":");
218+
for (BasicBlock bb: dominationFrontier) {
219+
sb.append(" L").append(bb.bid);
220+
}
221+
sb.append("\n");
222+
return sb;
223+
}
216224
@Override
217225
public boolean equals(Object o) {
218226
if (this == o) return true;

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

Lines changed: 67 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -426,7 +426,7 @@ private boolean compileArrayStoreExpr(AST.ArrayStoreExpr arrayStoreExpr) {
426426
}
427427

428428
private boolean compileNewExpr(AST.NewExpr newExpr) {
429-
codeNew(newExpr.type);
429+
codeNew(newExpr.type,newExpr.len,newExpr.initValue);
430430
return false;
431431
}
432432

@@ -644,20 +644,44 @@ else if (indexed instanceof Operand.LoadFieldOperand loadFieldOperand)
644644
codeMove(value, indexed);
645645
}
646646

647-
private void codeNew(Type type) {
647+
private void codeNew(Type type, AST.Expr len, AST.Expr initVal) {
648648
if (type instanceof Type.TypeArray typeArray)
649-
codeNewArray(typeArray);
649+
codeNewArray(typeArray, len, initVal);
650650
else if (type instanceof Type.TypeStruct typeStruct)
651651
codeNewStruct(typeStruct);
652652
else
653653
throw new CompilerException("Unexpected type: " + type);
654654
}
655655

656-
private void codeNewArray(Type.TypeArray typeArray) {
656+
private void codeNewArray(Type.TypeArray typeArray, AST.Expr len, AST.Expr initVal) {
657657
var temp = createTemp(typeArray);
658+
Operand lenOperand = null;
659+
Operand initValOperand = null;
660+
if (len != null) {
661+
boolean indexed = compileExpr(len);
662+
if (indexed)
663+
codeIndexedLoad();
664+
if (initVal != null) {
665+
indexed = compileExpr(initVal);
666+
if (indexed)
667+
codeIndexedLoad();
668+
initValOperand = pop();
669+
}
670+
lenOperand = pop();
671+
}
672+
Instruction insn;
658673
var target = (Operand.RegisterOperand) issa.write(temp);
659-
var insn = new Instruction.NewArray(typeArray, target);
674+
if (lenOperand != null) {
675+
if (initValOperand != null)
676+
insn = new Instruction.NewArray(typeArray, target, issa.read(lenOperand), issa.read(initValOperand));
677+
else
678+
insn = new Instruction.NewArray(typeArray, target, issa.read(lenOperand));
679+
}
680+
else
681+
insn = new Instruction.NewArray(typeArray, target);
660682
issa.recordDef(target, insn);
683+
if (lenOperand != null) issa.recordUse(lenOperand,insn);
684+
if (initValOperand != null) issa.recordUse(initValOperand,insn);
661685
code(insn);
662686
}
663687

@@ -941,6 +965,32 @@ private Register addPhiOperands(Register variable, Instruction.Phi phi) {
941965
return tryRemovingPhi(phi);
942966
}
943967

968+
// The Phi's def is dead so we need to remove
969+
// all occurrences of this def from the memoized defs
970+
// per Basic Block
971+
private void clearDefs(Instruction.Phi phi) {
972+
// TODO rethink the data structure for currentDef
973+
var def = phi.value();
974+
var defs = currentDef.get(def.nonSSAId());
975+
// Make a list of block/reg that we need to delete
976+
var bbList = new ArrayList<BasicBlock>();
977+
var regList = new ArrayList<Register>();
978+
for (var entries : defs.entrySet()) {
979+
var bb = entries.getKey();
980+
var reg = entries.getValue();
981+
if (reg.equals(def)) {
982+
bbList.add(bb);
983+
regList.add(reg);
984+
}
985+
}
986+
// Now delete them
987+
for (int i = 0; i < bbList.size(); i++) {
988+
var bb = bbList.get(i);
989+
var reg = regList.get(i);
990+
defs.remove(bb, reg);
991+
}
992+
}
993+
944994
private Register tryRemovingPhi(Instruction.Phi phi) {
945995
Register same = null;
946996
// Check if phi has distinct inputs
@@ -967,6 +1017,9 @@ private Register tryRemovingPhi(Instruction.Phi phi) {
9671017
// remove all uses of phi to same and remove phi
9681018
replacePhiValueAndUsers(phi, same);
9691019
phi.block.deleteInstruction(phi);
1020+
// Since the phi is dead any references to its def
1021+
// must be removed; this is not mentioned in the paper
1022+
clearDefs(phi);
9701023
// try to recursively remove all phi users, which might have become trivial
9711024
for (var use: users) {
9721025
if (use instanceof Instruction.Phi phiuser)
@@ -979,25 +1032,28 @@ private Register tryRemovingPhi(Instruction.Phi phi) {
9791032
* Reroute all uses of phi to new value
9801033
*/
9811034
private void replacePhiValueAndUsers(Instruction.Phi phi, Register newValue) {
982-
var oldDefUseChain = ssaDefUses.get(phi.value());
1035+
var oldValue = phi.value();
1036+
var oldDefUseChain = ssaDefUses.get(oldValue);
9831037
var newDefUseChain = ssaDefUses.get(newValue);
9841038
if (newDefUseChain == null) {
985-
// Can be null because this may be existing def
986-
newDefUseChain = SSAEdges.addDef(ssaDefUses, newValue, phi);
1039+
throw new CompilerException("Expected error: undefined var " + newValue);
9871040
}
9881041
if (oldDefUseChain != null) {
9891042
for (Instruction instruction: oldDefUseChain.useList) {
1043+
boolean replaced;
9901044
if (instruction instanceof Instruction.Phi somePhi) {
991-
somePhi.replaceInput(phi.value(), newValue);
1045+
replaced = somePhi.replaceInput(oldValue, newValue);
9921046
}
9931047
else {
994-
instruction.replaceUse(phi.value(), newValue);
1048+
replaced = instruction.replaceUse(oldValue, newValue);
1049+
}
1050+
if (!replaced) {
1051+
throw new CompilerException("Discrepancy between var use list and var definition");
9951052
}
9961053
}
9971054
// Users of phi old value become users of the new value
9981055
newDefUseChain.useList.addAll(oldDefUseChain.useList);
9991056
oldDefUseChain.useList.clear();
1000-
// FIXME remove old def from def-use chains
10011057
}
10021058
}
10031059

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

Lines changed: 55 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,8 @@
11
package com.compilerprogramming.ezlang.compiler;
22

3-
import java.util.ArrayList;
43
import java.util.Comparator;
54
import java.util.HashSet;
65
import java.util.List;
7-
import java.util.function.Consumer;
86

97
/**
108
* The dominator tree construction algorithm is based on figure 9.24,
@@ -34,14 +32,15 @@ public DominatorTree(BasicBlock entry) {
3432
populateTree();
3533
setDepth();
3634
calculateDominanceFrontiers();
35+
//calculateDominanceFrontiersMethod2();
3736
}
3837

3938
private void calculateDominatorTree() {
4039
resetDomInfo();
4140
annotateBlocksWithRPO();
4241
sortBlocksByRPO();
4342

44-
// Set IDom entry for root to itself
43+
// Set IDom entry for root to itself (see note below)
4544
entry.idom = entry;
4645
boolean changed = true;
4746
while (changed) {
@@ -68,6 +67,11 @@ private void calculateDominatorTree() {
6867
}
6968
}
7069
}
70+
// There is a contradiction between what is described in the book
71+
// and the algo - as the book says the IDOM for root is undefined
72+
// But we set this to itself when calculating. So after calculations
73+
// are done, set this to null. This is technically more correct IMO.
74+
entry.idom = null;
7175
}
7276

7377
private void resetDomInfo() {
@@ -146,7 +150,7 @@ private BasicBlock findFirstPredecessorWithIdom(BasicBlock n) {
146150
private void populateTree() {
147151
for (BasicBlock block : blocks) {
148152
BasicBlock idom = block.idom;
149-
if (idom == block) // root
153+
if (idom == null) // root
150154
continue;
151155
// add edge from idom to n
152156
idom.dominatedChildren.add(block);
@@ -166,12 +170,13 @@ private void setDepth() {
166170
*/
167171
private void setDepth_(BasicBlock block) {
168172
BasicBlock idom = block.idom;
169-
if (idom != block) {
173+
if (idom != null) {
170174
assert idom.domDepth > 0;
171175
block.domDepth = idom.domDepth + 1;
172-
} else {
173-
assert idom.domDepth == 1;
174-
assert idom.domDepth == block.domDepth;
176+
}
177+
else {
178+
// root (entry) block's idom is null
179+
assert block.domDepth == 1;
175180
}
176181
for (BasicBlock child : block.dominatedChildren)
177182
setDepth_(child);
@@ -189,6 +194,8 @@ private void calculateDominanceFrontiers() {
189194
// while runner != doms[b]
190195
// add b to runner’s dominance frontier set
191196
// runner = doms[runner]
197+
for (BasicBlock b: blocks)
198+
b.dominationFrontier.clear(); // empty set
192199
for (BasicBlock b : blocks) {
193200
if (b.predecessors.size() >= 2) {
194201
for (BasicBlock p : b.predecessors) {
@@ -204,6 +211,37 @@ private void calculateDominanceFrontiers() {
204211
}
205212
}
206213

214+
// We have an alternative approach to calculating DOM Frontiers to
215+
// allow us to validate above
216+
private void calculateDominanceFrontiersMethod2()
217+
{
218+
for (BasicBlock b: blocks)
219+
b.dominationFrontier.clear(); // empty set
220+
computeDF(entry);
221+
}
222+
223+
// Implementation based on description in pg 440 of
224+
// Modern Compiler implementation in C
225+
// Appel
226+
private void computeDF(BasicBlock n) {
227+
var S = new HashSet<BasicBlock>();
228+
for (BasicBlock y: n.successors) {
229+
if (y.idom != n)
230+
S.add(y);
231+
}
232+
for (BasicBlock c: n.dominatedChildren) {
233+
computeDF(c);
234+
for (BasicBlock w: c.dominationFrontier) {
235+
// Note that the printed book has an error below
236+
// and errata gives the correct version
237+
if (!n.dominates(w) || n==w) {
238+
S.add(w);
239+
}
240+
}
241+
}
242+
n.dominationFrontier.addAll(S);
243+
}
244+
207245
public String generateDotOutput() {
208246
StringBuilder sb = new StringBuilder();
209247
sb.append("digraph DomTree {\n");
@@ -212,10 +250,18 @@ public String generateDotOutput() {
212250
}
213251
for (BasicBlock n : blocks) {
214252
BasicBlock idom = n.idom;
215-
if (idom == n) continue;
253+
if (idom == null) continue;
216254
sb.append(idom.uniqueName()).append("->").append(n.uniqueName()).append(";\n");
217255
}
218256
sb.append("}\n");
219257
return sb.toString();
220258
}
259+
260+
public String listDomFrontiers() {
261+
StringBuilder sb = new StringBuilder();
262+
for (BasicBlock n : blocks) {
263+
n.listDomFrontiers(sb);
264+
}
265+
return sb.toString();
266+
}
221267
}

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

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,17 @@ public EnterSSA(CompiledFunction bytecodeFunction, EnumSet<Options> options) {
4141
System.out.println("Pre SSA Dominator Tree");
4242
System.out.println(domTree.generateDotOutput());
4343
}
44+
if (options.contains(Options.DUMP_PRE_SSA_DOMFRONTIERS)) {
45+
System.out.println("Pre SSA Dominance Frontiers");
46+
System.out.println(domTree.listDomFrontiers());
47+
}
4448
this.blocks = domTree.blocks; // the blocks are ordered reverse post order
4549
findNonLocalNames();
4650
new Liveness(bytecodeFunction); // EWe require liveness info to construct pruned ssa
51+
if (options.contains(Options.DUMP_PRE_SSA_LIVENESS)) {
52+
System.out.println("Pre SSA Liveness");
53+
System.out.println(bytecodeFunction.toStr(new StringBuilder(), true));
54+
}
4755
insertPhis();
4856
renameVars();
4957
bytecodeFunction.isSSA = true;
@@ -177,8 +185,9 @@ void search(BasicBlock block) {
177185
}
178186
// Pop stacks for defs
179187
for (Instruction i: block.instructions) {
180-
if (i.definesVar()) {
181-
var reg = i.def();
188+
// Phis don't answer to definesVar() or def()
189+
if (i.definesVar() || i instanceof Instruction.Phi) {
190+
var reg = i instanceof Instruction.Phi phi ? phi.value() : i.def();
182191
stacks[reg.nonSSAId()].pop();
183192
}
184193
}

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

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,14 +124,28 @@ public NewArray(Type.TypeArray type, Operand.RegisterOperand destOperand) {
124124
super(I_NEW_ARRAY, destOperand);
125125
this.type = type;
126126
}
127+
public NewArray(Type.TypeArray type, Operand.RegisterOperand destOperand, Operand len) {
128+
super(I_NEW_ARRAY, destOperand, len);
129+
this.type = type;
130+
}
131+
public NewArray(Type.TypeArray type, Operand.RegisterOperand destOperand, Operand len, Operand initValue) {
132+
super(I_NEW_ARRAY, destOperand, len, initValue);
133+
this.type = type;
134+
}
135+
public Operand len() { return uses.length > 0 ? uses[0] : null; }
136+
public Operand initValue() { return uses.length > 1 ? uses[1] : null; }
127137
public Operand.RegisterOperand destOperand() { return def; }
128138
@Override
129139
public StringBuilder toStr(StringBuilder sb) {
130-
return sb.append(def)
140+
sb.append(def)
131141
.append(" = ")
132142
.append("New(")
133-
.append(type)
134-
.append(")");
143+
.append(type);
144+
if (len() != null)
145+
sb.append(", len=").append(len());
146+
if (initValue() != null)
147+
sb.append(", initValue=").append(initValue());
148+
return sb.append(")");
135149
}
136150
}
137151

@@ -417,15 +431,18 @@ public void addInput(Register register) {
417431
newUses[newUses.length-1] = new Operand.RegisterOperand(register);
418432
this.uses = newUses;
419433
}
420-
public void replaceInput(Register oldReg, Register newReg) {
434+
public boolean replaceInput(Register oldReg, Register newReg) {
435+
boolean replaced = false;
421436
for (int i = 0; i < numInputs(); i++) {
422437
if (isRegisterInput(i)) {
423438
Register in = inputAsRegister(i);
424439
if (in.equals(oldReg)) {
425440
replaceInput(i, newReg);
441+
replaced = true;
426442
}
427443
}
428444
}
445+
return replaced;
429446
}
430447
}
431448

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ public enum Options {
1010
REGALLOC,
1111
DUMP_INITIAL_IR,
1212
DUMP_PRE_SSA_DOMTREE,
13+
DUMP_PRE_SSA_DOMFRONTIERS,
14+
DUMP_PRE_SSA_LIVENESS,
1315
DUMP_SSA_IR,
1416
DUMP_SCCP_PREAPPLY,
1517
DUMP_SCCP_POSTAPPLY,

0 commit comments

Comments
 (0)