Skip to content

Commit

Permalink
Update on "[compiler] Optimize instruction reordering"
Browse files Browse the repository at this point in the history
Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain.

This PR includes two changes:
* First, allow `LoadLocal` to be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly once
* Uses a more optimal reordering for statement blocks, while keeping the existing approach for expression blocks.

In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization.

This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, then try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this.

So for normal blocks, we now emit terminal operands first. This will invariably cause some of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes.

I think the complexity cost of two strategies is worth the benefit, as evidenced by the reduced memo slots in the fixtures.

[ghstack-poisoned]
  • Loading branch information
josephsavona committed Jun 21, 2024
2 parents 89f74eb + 1215cc7 commit 7fc622a
Showing 1 changed file with 14 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -98,18 +98,18 @@ type Node = {

// Inclusive start and end
type References = {
accessedRanges: AccessedRanges;
singleUseIdentifiers: SingleUseIdentifiers;
lastAssignments: LastAssignments;
};
type LastAssignments = Map<string, InstructionId>;
type AccessedRanges = Map<IdentifierId, Range>;
type SingleUseIdentifiers = Set<IdentifierId>;
type Range = { start: InstructionId; end: InstructionId };

Check failure on line 106 in compiler/packages/babel-plugin-react-compiler/src/Optimization/InstructionReordering.ts

View workflow job for this annotation

GitHub Actions / Lint babel-plugin-react-compiler

'Range' is defined but never used. Allowed unused vars must match /^_/u
enum ReferenceKind {
Read,
Write,
}
function findReferencedRangeOfTemporaries(fn: HIRFunction): References {
const accessedRanges: AccessedRanges = new Map();
const singleUseIdentifiers = new Map<IdentifierId, number>();
const lastAssignments: LastAssignments = new Map();
function reference(
instr: InstructionId,
Expand All @@ -134,15 +134,8 @@ function findReferencedRangeOfTemporaries(fn: HIRFunction): References {
}
return;
} else if (kind === ReferenceKind.Read) {
const range = getOrInsertWith(
accessedRanges,
place.identifier.id,
() => ({
start: instr,
end: instr,
})
);
range.end = instr;
const previousCount = singleUseIdentifiers.get(place.identifier.id) ?? 0;
singleUseIdentifiers.set(place.identifier.id, previousCount + 1);
}
}
for (const [, block] of fn.body.blocks) {
Expand All @@ -158,7 +151,14 @@ function findReferencedRangeOfTemporaries(fn: HIRFunction): References {
reference(block.terminal.id, operand, ReferenceKind.Read);
}
}
return { accessedRanges, lastAssignments };
return {
singleUseIdentifiers: new Set(
[...singleUseIdentifiers]
.filter(([, count]) => count === 1)
.map(([id]) => id)
),
lastAssignments,
};
}

function reorderBlock(
Expand Down Expand Up @@ -485,12 +485,10 @@ function getReorderability(
const name = instr.value.place.identifier.name;
if (name !== null && name.kind === "named") {
const lastAssignment = references.lastAssignments.get(name.value);
const range = references.accessedRanges.get(instr.lvalue.identifier.id);
if (
lastAssignment !== undefined &&
lastAssignment < instr.id &&
range !== undefined &&
range.end === range.start // this LoadLocal is used exactly once
references.singleUseIdentifiers.has(instr.lvalue.identifier.id)
) {
return Reorderability.Reorderable;
}
Expand Down

0 comments on commit 7fc622a

Please sign in to comment.