Value block reassignment uses StoreLocal
As part of removing Instruction.lvalue we need to ensure that it is only used to represent that instruction's value — the InstructionKind should always be Const. The one place where we violated this was for value blocks, specifically ConditionalExpression and LogicalExpression. For both of those, we generate a single temporary place to represent the expression result. Then the consequent and alternate branch ended in a `LoadLocal` that reassigned that temporary (in the lvalue) to the result of that branch. This PR changes to use StoreLocal instead, and updates the recently added validation pass to ensure that all identifiers that appear in an Instruction.lvalue are only ever assigned once.
Joe Savona committed
Mar 3, 2023 at 17:09 UTC
fe2d179a619012ae1cea44f3efd97c9974a6d5d5
4 files changed
+106
-19
compiler/forget/src/CompilerPipeline.ts
+2
@@ -58,6 +58,8 @@ export function* run(
58
mergeConsecutiveBlocks(hir);
59
yield log({ kind: "hir", name: "MergeConsecutiveBlocks", value: hir });
60
61
+ validateConsistentIdentifiers(hir);
62
+
63
enterSSA(hir);
64
yield log({ kind: "hir", name: "SSA", value: hir });
65
compiler/forget/src/HIR/BuildHIR.ts
+34
-9
@@ -1041,10 +1041,19 @@ function lowerExpression(
1041
1042
// Block for the consequent (if the test is truthy)
1043
const consequentBlock = builder.enter("value", (_blockId) => {
1044
+ const consequent = lowerExpressionToTemporary(
1045
+ builder,
1046
+ expr.get("consequent")
1047
+ );
1048
builder.push({
1049
id: makeInstructionId(0),
1046
- lvalue: { ...place },
1047
- value: lowerExpression(builder, expr.get("consequent")),
1050
+ lvalue: buildTemporaryPlace(builder, exprLoc),
1051
+ value: {
1052
+ kind: "StoreLocal",
1053
+ lvalue: { kind: InstructionKind.Const, place: { ...place } },
1054
+ value: consequent,
1055
+ loc: exprLoc,
1056
+ },
1057
loc: exprLoc,
1058
});
1059
return {
@@ -1056,10 +1065,19 @@ function lowerExpression(
1065
});
1066
// Block for the alternate (if the test is not truthy)
1067
const alternateBlock = builder.enter("value", (_blockId) => {
1068
+ const alternate = lowerExpressionToTemporary(
1069
+ builder,
1070
+ expr.get("alternate")
1071
+ );
1072
builder.push({
1073
id: makeInstructionId(0),
1061
- lvalue: { ...place },
1062
- value: lowerExpression(builder, expr.get("alternate")),
1074
+ lvalue: buildTemporaryPlace(builder, exprLoc),
1075
+ value: {
1076
+ kind: "StoreLocal",
1077
+ lvalue: { kind: InstructionKind.Const, place: { ...place } },
1078
+ value: alternate,
1079
+ loc: exprLoc,
1080
+ },
1081
loc: exprLoc,
1082
});
1083
return {
@@ -1106,10 +1124,11 @@ function lowerExpression(
1124
const consequent = builder.enter("value", () => {
1125
builder.push({
1126
id: makeInstructionId(0),
1109
- lvalue: { ...place },
1127
+ lvalue: buildTemporaryPlace(builder, leftPlace.loc),
1128
value: {
1111
- kind: "LoadLocal",
1112
- place: { ...leftPlace },
1129
+ kind: "StoreLocal",
1130
+ lvalue: { kind: InstructionKind.Const, place: { ...place } },
1131
+ value: { ...leftPlace },
1132
loc: leftPlace.loc,
1133
},
1134
loc: exprLoc,
@@ -1122,10 +1141,16 @@ function lowerExpression(
1141
};
1142
});
1143
const alternate = builder.enter("value", () => {
1144
+ const right = lowerExpressionToTemporary(builder, expr.get("right"));
1145
builder.push({
1146
id: makeInstructionId(0),
1127
- lvalue: { ...place },
1128
- value: lowerExpression(builder, expr.get("right")),
1147
+ lvalue: buildTemporaryPlace(builder, right.loc),
1148
+ value: {
1149
+ kind: "StoreLocal",
1150
+ lvalue: { kind: InstructionKind.Const, place: { ...place } },
1151
+ value: { ...right },
1152
+ loc: right.loc,
1153
+ },
1154
loc: exprLoc,
1155
});
1156
return {
compiler/forget/src/HIR/ValidateConsistentIdentifiers.ts
+11
@@ -13,6 +13,7 @@ import {
13
IdentifierId,
14
SourceLocation,
15
} from "./HIR";
16
+import { printPlace } from "./PrintHIR";
17
import { eachInstructionValueOperand, eachTerminalOperand } from "./visitors";
18
19
/**
@@ -21,6 +22,7 @@ import { eachInstructionValueOperand, eachTerminalOperand } from "./visitors";
22
*/
23
export function validateConsistentIdentifiers(fn: HIRFunction): void {
24
const identifiers: Identifiers = new Map();
25
+ const assignments: Set<IdentifierId> = new Set();
26
for (const [, block] of fn.body.blocks) {
27
for (const phi of block.phis) {
28
validate(identifiers, phi.id);
@@ -35,6 +37,15 @@ export function validateConsistentIdentifiers(fn: HIRFunction): void {
37
instr.lvalue.loc
38
);
39
}
40
+ if (assignments.has(instr.lvalue.identifier.id)) {
41
+ CompilerError.invariant(
42
+ `Expected lvalues to be assigned exactly once, found duplicate assignment of '${printPlace(
43
+ instr.lvalue
44
+ )}'`,
45
+ instr.lvalue.loc
46
+ );
47
+ }
48
+ assignments.add(instr.lvalue.identifier.id);
49
validate(identifiers, instr.lvalue.identifier, instr.lvalue.loc);
50
for (const operand of eachInstructionValueOperand(instr.value)) {
51
validate(identifiers, operand.identifier, operand.loc);
compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts
+59
-10
@@ -519,16 +519,9 @@ class Driver {
519
loc: SourceLocation
520
): { block: BlockId; value: ReactiveValue; place: Place; id: InstructionId } {
521
const defaultBlock = this.cx.ir.blocks.get(id)!;
522
- if (
523
- defaultBlock.terminal.kind === "goto" ||
524
- defaultBlock.terminal.kind === "branch"
525
- ) {
522
+ if (defaultBlock.terminal.kind === "branch") {
523
const instructions = defaultBlock.instructions;
524
if (instructions.length === 0) {
528
- invariant(
529
- defaultBlock.terminal.kind === "branch",
530
- "Expected instructions for non-branch terminal"
531
- );
525
return {
526
block: defaultBlock.id,
527
place: defaultBlock.terminal.test,
@@ -541,6 +534,11 @@ class Driver {
534
};
535
} else if (defaultBlock.instructions.length === 1) {
536
const instr = defaultBlock.instructions[0]!;
537
+ invariant(
538
+ instr.lvalue.identifier.id ===
539
+ defaultBlock.terminal.test.identifier.id,
540
+ "Expected branch block to end in an instruction that sets the test value"
541
+ );
542
return {
543
block: defaultBlock.id,
544
place: instr.lvalue!,
@@ -558,7 +556,58 @@ class Driver {
556
};
557
return {
558
block: defaultBlock.id,
561
- place: instr.lvalue!,
559
+ place: defaultBlock.terminal.test,
560
+ value: sequence,
561
+ id: defaultBlock.terminal.id,
562
+ };
563
+ }
564
+ } else if (defaultBlock.terminal.kind === "goto") {
565
+ const instructions = defaultBlock.instructions;
566
+ if (instructions.length === 0) {
567
+ invariant(
568
+ false,
569
+ "Expected goto value block to have at least one instruction"
570
+ );
571
+ } else if (defaultBlock.instructions.length === 1) {
572
+ const instr = defaultBlock.instructions[0]!;
573
+ let place: Place = instr.lvalue!;
574
+ let value: ReactiveValue = instr.value;
575
+ if (instr.value.kind === "StoreLocal") {
576
+ place = instr.value.lvalue.place;
577
+ value = {
578
+ kind: "LoadLocal",
579
+ place: instr.value.value,
580
+ loc: instr.value.value.loc,
581
+ };
582
+ }
583
+ return {
584
+ block: defaultBlock.id,
585
+ place,
586
+ value,
587
+ id: instr.id,
588
+ };
589
+ } else {
590
+ const instr = defaultBlock.instructions.at(-1)!;
591
+ let place: Place = instr.lvalue!;
592
+ let value: ReactiveValue = instr.value;
593
+ if (instr.value.kind === "StoreLocal") {
594
+ place = instr.value.lvalue.place;
595
+ value = {
596
+ kind: "LoadLocal",
597
+ place: instr.value.value,
598
+ loc: instr.value.value.loc,
599
+ };
600
+ }
601
+ const sequence: ReactiveSequenceValue = {
602
+ kind: "SequenceExpression",
603
+ instructions: defaultBlock.instructions.slice(0, -1),
604
+ id: instr.id,
605
+ value,
606
+ loc: loc,
607
+ };
608
+ return {
609
+ block: defaultBlock.id,
610
+ place,
611
value: sequence,
612
id: instr.id,
613
};
@@ -675,7 +724,7 @@ class Driver {
724
};
725
invariant(
726
consequent.place.identifier === alternate.place.identifier,
678
- "Expected the consquent and alternate of a ternary to store a value to the same place"
727
+ "Expected the consequent and alternate of a ternary to store a value to the same place"
728
);
729
return {
730
place: { ...consequent.place },