From 1af828e4ae7c27f86c249c4670b71c5f937ee82f Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 19 Jul 2024 10:18:19 +0900 Subject: [PATCH 1/3] [compiler] Maintain RPO and uniquely instruction ids when constructing scope terminals Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. [ghstack-poisoned] --- .../src/HIR/AssertTerminalBlocksExist.ts | 1 + .../src/HIR/BuildReactiveScopeTerminalsHIR.ts | 65 ++++++++++++++++--- .../ValidatePreservedManualMemoization.ts | 3 +- 3 files changed, 59 insertions(+), 10 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/AssertTerminalBlocksExist.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/AssertTerminalBlocksExist.ts index 1708f7beefe..e696fddbdeb 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/AssertTerminalBlocksExist.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/AssertTerminalBlocksExist.ts @@ -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, }, ); diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildReactiveScopeTerminalsHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildReactiveScopeTerminalsHIR.ts index bf12bfd86fa..30f340ccdc0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildReactiveScopeTerminalsHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildReactiveScopeTerminalsHIR.ts @@ -9,10 +9,22 @@ import { GotoVariant, HIRFunction, InstructionId, + makeInstructionId, + Place, ReactiveScope, ReactiveScopeTerminal, ScopeId, } from './HIR'; +import { + markInstructionIds, + markPredecessors, + reversePostorderBlocks, +} from './HIRBuilder'; +import { + eachInstructionLValue, + eachInstructionValueOperand, + eachTerminalOperand, +} from './visitors'; /** * This pass assumes that all program blocks are properly nested with respect to fallthroughs @@ -142,16 +154,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); @@ -162,6 +167,50 @@ 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 + */ + 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 = diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index bbf271c50e5..51a665105f3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -22,7 +22,7 @@ import { ScopeId, SourceLocation, } from '../HIR'; -import {printManualMemoDependency} from '../HIR/PrintHIR'; +import {printManualMemoDependency, printPlace} from '../HIR/PrintHIR'; import {eachInstructionValueOperand} from '../HIR/visitors'; import {collectMaybeMemoDependencies} from '../Inference/DropManualMemoization'; import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; @@ -470,7 +470,6 @@ class Visitor extends ReactiveFunctionVisitor { state.errors.push({ reason: 'React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value may be mutated later, which could cause the value to change unexpectedly', - description: null, severity: ErrorSeverity.CannotPreserveMemoization, loc: typeof instruction.loc !== 'symbol' ? instruction.loc : null, suggestions: null, From 346eefb764b5c3d08cfd6b0c6543a82f21691c0d Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 19 Jul 2024 10:20:38 +0900 Subject: [PATCH 2/3] Update on "[compiler] Maintain RPO and uniquely instruction ids when constructing scope terminals" Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. [ghstack-poisoned] --- .../src/HIR/BuildReactiveScopeTerminalsHIR.ts | 21 +++++++++---------- .../ValidatePreservedManualMemoization.ts | 2 +- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildReactiveScopeTerminalsHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildReactiveScopeTerminalsHIR.ts index 30f340ccdc0..6819c2c6c5b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildReactiveScopeTerminalsHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildReactiveScopeTerminalsHIR.ts @@ -10,7 +10,6 @@ import { HIRFunction, InstructionId, makeInstructionId, - Place, ReactiveScope, ReactiveScopeTerminal, ScopeId, @@ -20,11 +19,7 @@ import { markPredecessors, reversePostorderBlocks, } from './HIRBuilder'; -import { - eachInstructionLValue, - eachInstructionValueOperand, - eachTerminalOperand, -} from './visitors'; +import {eachInstructionLValue} from './visitors'; /** * This pass assumes that all program blocks are properly nested with respect to fallthroughs @@ -186,9 +181,11 @@ export function buildReactiveScopeTerminalsHIR(fn: HIRFunction): void { 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 + /* + * 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 @@ -202,8 +199,10 @@ export function buildReactiveScopeTerminalsHIR(fn: HIRFunction): void { } 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 + /* + * 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; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index 51a665105f3..ee7f923ef18 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -22,7 +22,7 @@ import { ScopeId, SourceLocation, } from '../HIR'; -import {printManualMemoDependency, printPlace} from '../HIR/PrintHIR'; +import {printManualMemoDependency} from '../HIR/PrintHIR'; import {eachInstructionValueOperand} from '../HIR/visitors'; import {collectMaybeMemoDependencies} from '../Inference/DropManualMemoization'; import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; From 4059ee3959d64a88491665cc94217dd479bd2acd Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 19 Jul 2024 10:22:04 +0900 Subject: [PATCH 3/3] Update on "[compiler] Maintain RPO and uniquely instruction ids when constructing scope terminals" Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. [ghstack-poisoned] --- .../src/Validation/ValidatePreservedManualMemoization.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index ee7f923ef18..bbf271c50e5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -470,6 +470,7 @@ class Visitor extends ReactiveFunctionVisitor { state.errors.push({ reason: 'React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value may be mutated later, which could cause the value to change unexpectedly', + description: null, severity: ErrorSeverity.CannotPreserveMemoization, loc: typeof instruction.loc !== 'symbol' ? instruction.loc : null, suggestions: null,