Skip to content

Commit

Permalink
[wip][compiler] Stop rewriting IdentifierId in LeaveSSA
Browse files Browse the repository at this point in the history 
ghstack-source-id: 1b653587414b4103f33f415ab6574e8dbedd90b1
Pull Request resolved: #30573
  • Loading branch information
@josephsavona
josephsavona committed Aug 1, 2024
1 parent 3682b79 commit 5464ce4
Show file tree
Hide file tree
Showing 10 changed files with 277 additions and 79 deletions.
3 changes: 2 additions & 1 deletion compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ export function printInstructionValue(instrValue: ReactiveValue): string {
break;
}
case 'DeclareContext': {
value = `DeclareContext ${instrValue.lvalue.kind} ${printPlace(
value = `DeclareContext (${instrValue.lvalue.kind}) ${instrValue.lvalue.kind} ${printPlace(
instrValue.lvalue.place,
)}`;
break;
Expand Down Expand Up @@ -833,6 +833,7 @@ export function printPlace(place: Place): string {
}

export function printIdentifier(id: Identifier): string {
// return `${printName(id.name)}\$${id.id}${Number(id.declarationId) !== id.id ? `_d${id.declarationId}` : ''}${printScope(id.scope)}`;
return `${printName(id.name)}\$${id.id}${printScope(id.scope)}`;
}

Expand Down
23 changes: 12 additions & 11 deletions compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {Environment, EnvironmentConfig, ExternalFunction} from '../HIR';
import {
ArrayPattern,
BlockId,
DeclarationId,
GeneratedSource,
Identifier,
IdentifierId,
Expand Down Expand Up @@ -309,9 +310,9 @@ function codegenReactiveFunction(
): Result<CodegenFunction, CompilerError> {
for (const param of fn.params) {
if (param.kind === 'Identifier') {
cx.temp.set(param.identifier.id, null);
cx.temp.set(param.identifier.declarationId, null);
} else {
cx.temp.set(param.place.identifier.id, null);
cx.temp.set(param.place.identifier.declarationId, null);
}
}

Expand Down Expand Up @@ -392,7 +393,7 @@ class Context {
env: Environment;
fnName: string;
#nextCacheIndex: number = 0;
#declarations: Set<IdentifierId> = new Set();
#declarations: Set<DeclarationId> = new Set();
temp: Temporaries;
errors: CompilerError = new CompilerError();
objectMethods: Map<IdentifierId, ObjectMethod> = new Map();
Expand All @@ -418,11 +419,11 @@ class Context {
}

declare(identifier: Identifier): void {
this.#declarations.add(identifier.id);
this.#declarations.add(identifier.declarationId);
}

hasDeclared(identifier: Identifier): boolean {
return this.#declarations.has(identifier.id);
return this.#declarations.has(identifier.declarationId);
}

synthesizeName(name: string): ValidIdentifierName {
Expand Down Expand Up @@ -1147,7 +1148,7 @@ function codegenTerminal(
let catchParam = null;
if (terminal.handlerBinding !== null) {
catchParam = convertIdentifier(terminal.handlerBinding.identifier);
cx.temp.set(terminal.handlerBinding.identifier.id, null);
cx.temp.set(terminal.handlerBinding.identifier.declarationId, null);
}
return t.tryStatement(
codegenBlock(cx, terminal.block),
Expand Down Expand Up @@ -1205,7 +1206,7 @@ function codegenInstructionNullable(
kind !== InstructionKind.Reassign &&
place.identifier.name === null
) {
cx.temp.set(place.identifier.id, null);
cx.temp.set(place.identifier.declarationId, null);
}
const isDeclared = cx.hasDeclared(place.identifier);
hasReasign ||= isDeclared;
Expand Down Expand Up @@ -1261,7 +1262,7 @@ function codegenInstructionNullable(
);
if (instr.lvalue !== null) {
if (instr.value.kind !== 'StoreContext') {
cx.temp.set(instr.lvalue.identifier.id, expr);
cx.temp.set(instr.lvalue.identifier.declarationId, expr);
return null;
} else {
// Handle chained reassignments for context variables
Expand Down Expand Up @@ -1530,7 +1531,7 @@ function createCallExpression(
}
}

type Temporaries = Map<IdentifierId, t.Expression | t.JSXText | null>;
type Temporaries = Map<DeclarationId, t.Expression | t.JSXText | null>;

function codegenLabel(id: BlockId): string {
return `bb${id}`;
Expand All @@ -1549,7 +1550,7 @@ function codegenInstruction(
}
if (instr.lvalue.identifier.name === null) {
// temporary
cx.temp.set(instr.lvalue.identifier.id, value);
cx.temp.set(instr.lvalue.identifier.declarationId, value);
return t.emptyStatement();
} else {
const expressionValue = convertValueToExpression(value);
Expand Down Expand Up @@ -2498,7 +2499,7 @@ function codegenPlaceToExpression(cx: Context, place: Place): t.Expression {
}

function codegenPlace(cx: Context, place: Place): t.Expression | t.JSXText {
let tmp = cx.temp.get(place.identifier.id);
let tmp = cx.temp.get(place.identifier.declarationId);
if (tmp != null) {
return tmp;
}
Expand Down
55 changes: 43 additions & 12 deletions compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
import {CompilerError} from '../CompilerError';
import {GeneratedSource} from '../HIR';
import {
DeclarationId,
Identifier,
IdentifierId,
InstructionId,
Place,
PrunedReactiveScopeBlock,
Expand All @@ -24,7 +24,6 @@ import {ReactiveFunctionVisitor, visitReactiveFunction} from './visitors';

class Visitor extends ReactiveFunctionVisitor<State> {
override visitScope(scopeBlock: ReactiveScopeBlock, state: State): void {
this.traverseScope(scopeBlock, state);
for (const dep of scopeBlock.scope.dependencies) {
const {identifier} = dep;
if (identifier.name == null) {
Expand All @@ -43,21 +42,23 @@ class Visitor extends ReactiveFunctionVisitor<State> {
promoteIdentifier(declaration.identifier, state);
}
}
this.traverseScope(scopeBlock, state);
}

override visitPrunedScope(
scopeBlock: PrunedReactiveScopeBlock,
state: State,
): void {
this.traversePrunedScope(scopeBlock, state);
for (const [, declaration] of scopeBlock.scope.declarations) {
if (
declaration.identifier.name == null &&
state.pruned.get(declaration.identifier.id)?.usedOutsideScope === true
state.pruned.get(declaration.identifier.declarationId)
?.usedOutsideScope === true
) {
promoteIdentifier(declaration.identifier, state);
}
}
this.traversePrunedScope(scopeBlock, state);
}

override visitParam(place: Place, state: State): void {
Expand Down Expand Up @@ -93,11 +94,38 @@ class Visitor extends ReactiveFunctionVisitor<State> {
}
}

type JsxExpressionTags = Set<IdentifierId>;
class Visitor2 extends ReactiveFunctionVisitor<State> {
override visitPlace(_id: InstructionId, place: Place, state: State): void {
if (
place.identifier.name === null &&
state.promoted.has(place.identifier.declarationId)
) {
promoteIdentifier(place.identifier, state);
}
}
override visitLValue(
_id: InstructionId,
_lvalue: Place,
_state: State,
): void {
this.visitPlace(_id, _lvalue, _state);
}
override visitReactiveFunctionValue(
_id: InstructionId,
_dependencies: Array<Place>,
fn: ReactiveFunction,
state: State,
): void {
visitReactiveFunction(fn, this, state);
}
}

type JsxExpressionTags = Set<DeclarationId>;
type State = {
tags: JsxExpressionTags;
promoted: Set<DeclarationId>;
pruned: Map<
IdentifierId,
DeclarationId,
{activeScopes: Array<ScopeId>; usedOutsideScope: boolean}
>; // true if referenced within another scope, false if only accessed outside of scopes
};
Expand All @@ -108,9 +136,9 @@ class CollectPromotableTemporaries extends ReactiveFunctionVisitor<State> {
override visitPlace(_id: InstructionId, place: Place, state: State): void {
if (
this.activeScopes.length !== 0 &&
state.pruned.has(place.identifier.id)
state.pruned.has(place.identifier.declarationId)
) {
const prunedPlace = state.pruned.get(place.identifier.id)!;
const prunedPlace = state.pruned.get(place.identifier.declarationId)!;
if (prunedPlace.activeScopes.indexOf(this.activeScopes.at(-1)!) === -1) {
prunedPlace.usedOutsideScope = true;
}
Expand All @@ -124,16 +152,16 @@ class CollectPromotableTemporaries extends ReactiveFunctionVisitor<State> {
): void {
this.traverseValue(id, value, state);
if (value.kind === 'JsxExpression' && value.tag.kind === 'Identifier') {
state.tags.add(value.tag.identifier.id);
state.tags.add(value.tag.identifier.declarationId);
}
}

override visitPrunedScope(
scopeBlock: PrunedReactiveScopeBlock,
state: State,
): void {
for (const [id] of scopeBlock.scope.declarations) {
state.pruned.set(id, {
for (const [_id, decl] of scopeBlock.scope.declarations) {
state.pruned.set(decl.identifier.declarationId, {
activeScopes: [...this.activeScopes],
usedOutsideScope: false,
});
Expand All @@ -151,6 +179,7 @@ class CollectPromotableTemporaries extends ReactiveFunctionVisitor<State> {
export function promoteUsedTemporaries(fn: ReactiveFunction): void {
const state: State = {
tags: new Set(),
promoted: new Set(),
pruned: new Map(),
};
visitReactiveFunction(fn, new CollectPromotableTemporaries(), state);
Expand All @@ -161,6 +190,7 @@ export function promoteUsedTemporaries(fn: ReactiveFunction): void {
}
}
visitReactiveFunction(fn, new Visitor(), state);
visitReactiveFunction(fn, new Visitor2(), state);
}

function promoteIdentifier(identifier: Identifier, state: State): void {
Expand All @@ -171,9 +201,10 @@ function promoteIdentifier(identifier: Identifier, state: State): void {
loc: GeneratedSource,
suggestions: null,
});
if (state.tags.has(identifier.id)) {
if (state.tags.has(identifier.declarationId)) {
promoteTemporaryJsxTag(identifier);
} else {
promoteTemporary(identifier);
}
state.promoted.add(identifier.declarationId);
}
34 changes: 21 additions & 13 deletions ...ler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import {CompilerError} from '../CompilerError';
import {
BlockId,
DeclarationId,
GeneratedSource,
Identifier,
IdentifierId,
Expand Down Expand Up @@ -76,9 +77,9 @@ type TemporariesUsedOutsideDefiningScope = {
* tracks all relevant temporary declarations (currently LoadLocal and PropertyLoad)
* and the scope where they are defined
*/
declarations: Map<IdentifierId, ScopeId>;
declarations: Map<DeclarationId, ScopeId>;
// temporaries used outside of their defining scope
usedOutsideDeclaringScope: Set<IdentifierId>;
usedOutsideDeclaringScope: Set<DeclarationId>;
};
class FindPromotedTemporaries extends ReactiveFunctionVisitor<TemporariesUsedOutsideDefiningScope> {
scopes: Array<ScopeId> = [];
Expand Down Expand Up @@ -107,7 +108,10 @@ class FindPromotedTemporaries extends ReactiveFunctionVisitor<TemporariesUsedOut
case 'LoadLocal':
case 'LoadContext':
case 'PropertyLoad': {
state.declarations.set(instruction.lvalue.identifier.id, scope);
state.declarations.set(
instruction.lvalue.identifier.declarationId,
scope,
);
break;
}
default: {
Expand All @@ -121,18 +125,20 @@ class FindPromotedTemporaries extends ReactiveFunctionVisitor<TemporariesUsedOut
place: Place,
state: TemporariesUsedOutsideDefiningScope,
): void {
const declaringScope = state.declarations.get(place.identifier.id);
const declaringScope = state.declarations.get(
place.identifier.declarationId,
);
if (declaringScope === undefined) {
return;
}
if (this.scopes.indexOf(declaringScope) === -1) {
// Declaring scope is not active === used outside declaring scope
state.usedOutsideDeclaringScope.add(place.identifier.id);
state.usedOutsideDeclaringScope.add(place.identifier.declarationId);
}
}
}

type DeclMap = Map<IdentifierId, Decl>;
type DeclMap = Map<DeclarationId, Decl>;
type Decl = {
id: InstructionId;
scope: Stack<ScopeTraversalState>;
Expand Down Expand Up @@ -280,7 +286,7 @@ class PoisonState {
}

class Context {
#temporariesUsedOutsideScope: Set<IdentifierId>;
#temporariesUsedOutsideScope: Set<DeclarationId>;
#declarations: DeclMap = new Map();
#reassignments: Map<Identifier, Decl> = new Map();
// Reactive dependencies used in the current reactive scope.
Expand All @@ -307,7 +313,7 @@ class Context {
#scopes: Stack<ScopeTraversalState> = empty();
poisonState: PoisonState = new PoisonState(new Set(), new Set(), false);

constructor(temporariesUsedOutsideScope: Set<IdentifierId>) {
constructor(temporariesUsedOutsideScope: Set<DeclarationId>) {
this.#temporariesUsedOutsideScope = temporariesUsedOutsideScope;
}

Expand Down Expand Up @@ -377,7 +383,9 @@ class Context {
}

isUsedOutsideDeclaringScope(place: Place): boolean {
return this.#temporariesUsedOutsideScope.has(place.identifier.id);
return this.#temporariesUsedOutsideScope.has(
place.identifier.declarationId,
);
}

/*
Expand Down Expand Up @@ -440,8 +448,8 @@ class Context {
* on itself.
*/
declare(identifier: Identifier, decl: Decl): void {
if (!this.#declarations.has(identifier.id)) {
this.#declarations.set(identifier.id, decl);
if (!this.#declarations.has(identifier.declarationId)) {
this.#declarations.set(identifier.declarationId, decl);
}
this.#reassignments.set(identifier, decl);
}
Expand Down Expand Up @@ -533,7 +541,7 @@ class Context {
*/
const currentDeclaration =
this.#reassignments.get(identifier) ??
this.#declarations.get(identifier.id);
this.#declarations.get(identifier.declarationId);
const currentScope = this.currentScope.value?.value;
return (
currentScope != null &&
Expand Down Expand Up @@ -599,7 +607,7 @@ class Context {
* (all other decls e.g. `let x;` should be initialized in BuildHIR)
*/
const originalDeclaration = this.#declarations.get(
maybeDependency.identifier.id,
maybeDependency.identifier.declarationId,
);
if (
originalDeclaration !== undefined &&
Expand Down
Loading

0 comments on commit 5464ce4

Please sign in to comment.