Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[compiler] Maintain RPO and unique instruction ids when constructing scope terminals #30399

Merged
merged 5 commits into from
Jul 24, 2024
Merged

[compiler] Maintain RPO and unique instruction ids when constructing scope terminals #30399

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1af828e
[compiler] Maintain RPO and uniquely instruction ids when constructin…
josephsavona Jul 19, 2024
346eefb
Update on "[compiler] Maintain RPO and uniquely instruction ids when …
josephsavona Jul 19, 2024
4059ee3
Update on "[compiler] Maintain RPO and uniquely instruction ids when …
josephsavona Jul 19, 2024
45e889b
Update on "[compiler] Maintain RPO and unique instruction ids when co…
josephsavona Jul 19, 2024
9983ccf
rebase on "[compiler] Maintain RPO and unique instruction ids when co…
josephsavona Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/AssertTerminalBlocksExist.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export function assertTerminalPredsExist(fn: HIRFunction): void {
[...eachTerminalSuccessor(predBlock.terminal)].includes(block.id),
{
reason: 'Terminal successor does not reference correct predecessor',
description: `Block bb${block.id} has bb${predBlock.id} as a predecessor, but bb${predBlock.id}'s successors do not include bb${block.id}`,
loc: GeneratedSource,
},
);
Expand Down
64 changes: 56 additions & 8 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/BuildReactiveScopeTerminalsHIR.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,17 @@ import {
GotoVariant,
HIRFunction,
InstructionId,
makeInstructionId,
ReactiveScope,
ReactiveScopeTerminal,
ScopeId,
} from './HIR';
import {
markInstructionIds,
markPredecessors,
reversePostorderBlocks,
} from './HIRBuilder';
import {eachInstructionLValue} from './visitors';

/**
* This pass assumes that all program blocks are properly nested with respect to fallthroughs
Expand Down Expand Up @@ -142,16 +149,9 @@ export function buildReactiveScopeTerminalsHIR(fn: HIRFunction): void {

/**
* Step 3:
* Repoint preds and phis when they refer to a rewritten block.
* Repoint phis when they refer to a rewritten block.
*/
for (const [, block] of originalBlocks) {
for (const pred of block.preds) {
const newId = rewrittenFinalBlocks.get(pred);
if (newId != null) {
block.preds.delete(pred);
block.preds.add(newId);
}
}
for (const phi of block.phis) {
for (const [originalId, value] of phi.operands) {
const newId = rewrittenFinalBlocks.get(originalId);
Expand All @@ -162,6 +162,54 @@ export function buildReactiveScopeTerminalsHIR(fn: HIRFunction): void {
}
}
}

/**
* Step 4:
* Fixup the HIR to restore RPO, ensure correct predecessors, and
* renumber instructions. Note that the renumbering instructions
* invalidates scope and identifier ranges, so we fix them in the
* next step.
*/
reversePostorderBlocks(fn.body);
markPredecessors(fn.body);
markInstructionIds(fn.body);

/**
* Step 5:
* Fix scope and identifier ranges to account for renumbered instructions
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I don't think the range fixing logic covers all cases here. I wrote some validation in #30409, and we see quite a few cases in which identifiers have an attached mutable range that is neither (1) invalid e.g. end === start + 1 nor (2) a scope range.

There's roughly 3-4 classes:

  1. (fixed by [compiler][donotcommit] show differences between identifier + scope ranges #30409) when we merge scopes, we're currently not setting identifier.mutableRange to that of the scope.
  2. There are lvalues for which we end up pruning the scope, but keep their previous mutable range (see method call example).
  3. Handful of other times we end up assigning a mutable range but not a scope: see props aliasing as a context variable and mutable ref examples

Copy link
Contributor

@mofeiZ mofeiZ Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think currently, mutable ranges are referenced after this pass (see ValidateMemoizedEffectDeps and ValidatePreserveExistingManualMemo for examples).

Overall, I admit I'm a bit confused whether we need to ensure that identifier and scope ranges are always equivalent (i.e. pointing to the same object). My understanding is that this is unnecessary and we should instead deep copy when creating scopes in InferReactiveScopeVariables. Currently, our aligning and merging logic extends the mutableRange of some but not all identifiers

(cc @mvitousek as we were discussing a related topic yesterday)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was realizing after writing this that there were some other cases to cover. I think we need to just update all the passes that look at identifier or scope ranges (after this pass) to stop doing that and look at the scope’s effective range.

Alternatively I can do a more thorough tracking of how every scope needs to update its range before/after.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update all the passes that look at identifier or scope ranges (after this pass) to stop doing that

Is this possible to do even before this pass? I.e., is it semantically necessary that identifiers report their own mutable ranges at all? It feels like it would be more consistent to always refer to an identifier's scope's mutable range. As it stands I am uncertain what it even means when they don't align to be honest. (Happy to split this into a separate conversation!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's necessary for identifiers to report their ranges so that we can construct scopes. But yeah, after that i don't see why we'd ever need to use identifier ranges.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #30428 - happy to squash that into this PR though

for (const [, block] of fn.body.blocks) {
for (const instruction of block.instructions) {
for (const lvalue of eachInstructionLValue(instruction)) {
/*
* Any lvalues whose mutable range was a single instruction must have
* started at the current instruction, so update the range to match
* the instruction's new id
*/
if (
lvalue.identifier.mutableRange.end ===
lvalue.identifier.mutableRange.start + 1
) {
lvalue.identifier.mutableRange.start = instruction.id;
lvalue.identifier.mutableRange.end = makeInstructionId(
instruction.id + 1,
);
}
}
}
const terminal = block.terminal;
if (terminal.kind === 'scope' || terminal.kind === 'pruned-scope') {
/*
* Scope ranges should always align to start at the 'scope' terminal
* and end at the first instruction of the fallthrough block
*/
const fallthroughBlock = fn.body.blocks.get(terminal.fallthrough)!;
const firstId =
fallthroughBlock.instructions[0]?.id ?? fallthroughBlock.terminal.id;
terminal.scope.range.start = terminal.id;
terminal.scope.range.end = firstId;
}
}
}

type TerminalRewriteInfo =
Expand Down