[compiler] Represent phis with places rather than identifiers

Summary: The fact that phis are identifiers rather than places is unfortunate in a few cases. In some later analyses, we might wish to know whether a phi is reactive, but we don't have an easy way to do that currently. Most of the changes here is just replacing phi.id with phi.place.identifier and such. Interesting bits are EnterSSA (several functions now take places rather than identifiers, and InferReactivePlaces now needs to mark places as reactive explicitly. [ghstack-poisoned]
facebook · Oct 10, 2024 · f8c0c3e · f8c0c3e
1 parent 131ae81
commit f8c0c3e
Show file tree

Hide file tree

Showing 20 changed files with 103 additions and 99 deletions.
diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/AssertConsistentIdentifiers.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/AssertConsistentIdentifiers.ts
@@ -29,9 +29,9 @@ export function assertConsistentIdentifiers(fn: HIRFunction): void {
   const assignments: Set<IdentifierId> = new Set();
   for (const [, block] of fn.body.blocks) {
     for (const phi of block.phis) {
-      validate(identifiers, phi.id);
+      validate(identifiers, phi.place.identifier);
       for (const [, operand] of phi.operands) {
-        validate(identifiers, operand);
+        validate(identifiers, operand.identifier);
       }
     }
     for (const instr of block.instructions) {

diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/AssertValidMutableRanges.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/AssertValidMutableRanges.ts
@@ -20,9 +20,9 @@ import {
 export function assertValidMutableRanges(fn: HIRFunction): void {
   for (const [, block] of fn.body.blocks) {
     for (const phi of block.phis) {
-      visitIdentifier(phi.id);
+      visitIdentifier(phi.place.identifier);
       for (const [, operand] of phi.operands) {
-        visitIdentifier(operand);
+        visitIdentifier(operand.identifier);
       }
     }
     for (const instr of block.instructions) {

diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts
@@ -761,8 +761,8 @@ function _staticInvariantInstructionValueHasLocation(
 
 export type Phi = {
   kind: 'Phi';
-  id: Identifier;
-  operands: Map<BlockId, Identifier>;
+  place: Place;
+  operands: Map<BlockId, Place>;
 };
 
 /**

diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/MergeConsecutiveBlocks.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/MergeConsecutiveBlocks.ts
@@ -84,20 +84,14 @@ export function mergeConsecutiveBlocks(fn: HIRFunction): void {
         id: predecessor.terminal.id,
         lvalue: {
           kind: 'Identifier',
-          identifier: phi.id,
+          identifier: phi.place.identifier,
           effect: Effect.ConditionallyMutate,
           reactive: false,
           loc: GeneratedSource,
         },
         value: {
           kind: 'LoadLocal',
-          place: {
-            kind: 'Identifier',
-            identifier: operand,
-            effect: Effect.Read,
-            reactive: false,
-            loc: GeneratedSource,
-          },
+          place: {...operand},
           loc: GeneratedSource,
         },
         loc: GeneratedSource,

diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts
@@ -163,13 +163,13 @@ export function printInstruction(instr: ReactiveInstruction): string {
 
 export function printPhi(phi: Phi): string {
   const items = [];
-  items.push(printIdentifier(phi.id));
-  items.push(printMutableRange(phi.id));
-  items.push(printType(phi.id.type));
+  items.push(printPlace(phi.place));
+  items.push(printMutableRange(phi.place.identifier));
+  items.push(printType(phi.place.identifier.type));
   items.push(': phi(');
   const phis = [];
-  for (const [blockId, id] of phi.operands) {
-    phis.push(`bb${blockId}: ${printIdentifier(id)}`);
+  for (const [blockId, place] of phi.operands) {
+    phis.push(`bb${blockId}: ${printPlace(place)}`);
   }
 
   items.push(phis.join(', '));

diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts
@@ -607,7 +607,7 @@ function collectDependencies(
     // Record referenced optional chains in phis
     for (const phi of block.phis) {
       for (const operand of phi.operands) {
-        const maybeOptionalChain = temporaries.get(operand[1].id);
+        const maybeOptionalChain = temporaries.get(operand[1].identifier.id);
         if (maybeOptionalChain) {
           context.visitDependency(maybeOptionalChain);
         }

diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferAliasForPhis.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferAliasForPhis.ts
@@ -15,11 +15,11 @@ export function inferAliasForPhis(
   for (const [_, block] of func.body.blocks) {
     for (const phi of block.phis) {
       const isPhiMutatedAfterCreation: boolean =
-        phi.id.mutableRange.end >
+        phi.place.identifier.mutableRange.end >
         (block.instructions.at(0)?.id ?? block.terminal.id);
       if (isPhiMutatedAfterCreation) {
         for (const [, operand] of phi.operands) {
-          aliases.union([phi.id, operand]);
+          aliases.union([phi.place.identifier, operand.identifier]);
         }
       }
     }

diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableLifetimes.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableLifetimes.ts
@@ -116,19 +116,23 @@ export function inferMutableLifetimes(
   for (const [_, block] of func.body.blocks) {
     for (const phi of block.phis) {
       const isPhiMutatedAfterCreation: boolean =
-        phi.id.mutableRange.end >
+        phi.place.identifier.mutableRange.end >
         (block.instructions.at(0)?.id ?? block.terminal.id);
       if (
         inferMutableRangeForStores &&
         isPhiMutatedAfterCreation &&
-        phi.id.mutableRange.start === 0
+        phi.place.identifier.mutableRange.start === 0
       ) {
         for (const [, operand] of phi.operands) {
-          if (phi.id.mutableRange.start === 0) {
-            phi.id.mutableRange.start = operand.mutableRange.start;
+          if (phi.place.identifier.mutableRange.start === 0) {
+            phi.place.identifier.mutableRange.start =
+              operand.identifier.mutableRange.start;
           } else {
-            phi.id.mutableRange.start = makeInstructionId(
-              Math.min(phi.id.mutableRange.start, operand.mutableRange.start),
+            phi.place.identifier.mutableRange.start = makeInstructionId(
+              Math.min(
+                phi.place.identifier.mutableRange.start,
+                operand.identifier.mutableRange.start,
+              ),
             );
           }
         }

diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts
@@ -162,23 +162,23 @@ export function inferReactivePlaces(fn: HIRFunction): void {
       let hasReactiveControl = isReactiveControlledBlock(block.id);
 
       for (const phi of block.phis) {
-        if (reactiveIdentifiers.isReactiveIdentifier(phi.id)) {
+        if (reactiveIdentifiers.isReactive(phi.place)) {
           // Already marked reactive on a previous pass
           continue;
         }
         let isPhiReactive = false;
         for (const [, operand] of phi.operands) {
-          if (reactiveIdentifiers.isReactiveIdentifier(operand)) {
+          if (reactiveIdentifiers.isReactive(operand)) {
             isPhiReactive = true;
             break;
           }
         }
         if (isPhiReactive) {
-          reactiveIdentifiers.markReactiveIdentifier(phi.id);
+          reactiveIdentifiers.markReactive(phi.place);
         } else {
           for (const [pred] of phi.operands) {
             if (isReactiveControlledBlock(pred)) {
-              reactiveIdentifiers.markReactiveIdentifier(phi.id);
+              reactiveIdentifiers.markReactive(phi.place);
               break;
             }
           }

diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts
@@ -635,7 +635,7 @@ class InferenceState {
   inferPhi(phi: Phi): void {
     const values: Set<InstructionValue> = new Set();
     for (const [_, operand] of phi.operands) {
-      const operandValues = this.#variables.get(operand.id);
+      const operandValues = this.#variables.get(operand.identifier.id);
       // This is a backedge that will be handled later by State.merge
       if (operandValues === undefined) continue;
       for (const v of operandValues) {
@@ -644,7 +644,7 @@ class InferenceState {
     }
 
     if (values.size > 0) {
-      this.#variables.set(phi.id.id, values);
+      this.#variables.set(phi.place.identifier.id, values);
     }
   }
 }

diff --git a/compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts b/compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
@@ -117,7 +117,7 @@ function applyConstantPropagation(
     for (const phi of block.phis) {
       let value = evaluatePhi(phi, constants);
       if (value !== null) {
-        constants.set(phi.id.id, value);
+        constants.set(phi.place.identifier.id, value);
       }
     }
 
@@ -167,7 +167,7 @@ function applyConstantPropagation(
 function evaluatePhi(phi: Phi, constants: Constants): Constant | null {
   let value: Constant | null = null;
   for (const [, operand] of phi.operands) {
-    const operandValue = constants.get(operand.id) ?? null;
+    const operandValue = constants.get(operand.identifier.id) ?? null;
     // did not find a constant, can't constant propogate
     if (operandValue === null) {
       return null;

diff --git a/compiler/packages/babel-plugin-react-compiler/src/Optimization/DeadCodeElimination.ts b/compiler/packages/babel-plugin-react-compiler/src/Optimization/DeadCodeElimination.ts
@@ -42,7 +42,7 @@ export function deadCodeElimination(fn: HIRFunction): void {
    */
   for (const [, block] of fn.body.blocks) {
     for (const phi of block.phis) {
-      if (!state.isIdOrNameUsed(phi.id)) {
+      if (!state.isIdOrNameUsed(phi.place.identifier)) {
         block.phis.delete(phi);
       }
     }
@@ -159,9 +159,9 @@ function findReferencedIdentifiers(fn: HIRFunction): State {
         }
       }
       for (const phi of block.phis) {
-        if (state.isIdOrNameUsed(phi.id)) {
+        if (state.isIdOrNameUsed(phi.place.identifier)) {
           for (const [_pred, operand] of phi.operands) {
-            state.reference(operand);
+            state.reference(operand.identifier);
           }
         }
       }

diff --git a/compiler/packages/babel-plugin-react-compiler/src/Optimization/PruneMaybeThrows.ts b/compiler/packages/babel-plugin-react-compiler/src/Optimization/PruneMaybeThrows.ts
@@ -23,7 +23,7 @@ import {
   removeUnnecessaryTryCatch,
   removeUnreachableForUpdates,
 } from '../HIR/HIRBuilder';
-import {printIdentifier} from '../HIR/PrintHIR';
+import {printPlace} from '../HIR/PrintHIR';
 
 /*
  * This pass prunes `maybe-throw` terminals for blocks that can provably *never* throw.
@@ -55,7 +55,7 @@ export function pruneMaybeThrows(fn: HIRFunction): void {
               loc: GeneratedSource,
               description: `Could not find mapping for predecessor bb${predecessor} in block bb${
                 block.id
-              } for phi ${printIdentifier(phi.id)}`,
+              } for phi ${printPlace(phi.place)}`,
               suggestions: null,
             });
             phi.operands.delete(predecessor);

diff --git a/...er/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts b/...er/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts
@@ -281,22 +281,25 @@ export function findDisjointMutableValues(
      */
     for (const phi of block.phis) {
       if (
-        phi.id.mutableRange.start + 1 !== phi.id.mutableRange.end &&
-        phi.id.mutableRange.end >
+        phi.place.identifier.mutableRange.start + 1 !==
+          phi.place.identifier.mutableRange.end &&
+        phi.place.identifier.mutableRange.end >
           (block.instructions.at(0)?.id ?? block.terminal.id)
       ) {
-        const operands = [phi.id];
-        const declaration = declarations.get(phi.id.declarationId);
+        const operands = [phi.place.identifier];
+        const declaration = declarations.get(
+          phi.place.identifier.declarationId,
+        );
         if (declaration !== undefined) {
           operands.push(declaration);
         }
         for (const [_, phiId] of phi.operands) {
-          operands.push(phiId);
+          operands.push(phiId.identifier);
         }
         scopeIdentifiers.union(operands);
       } else if (fn.env.config.enableForest) {
         for (const [, phiId] of phi.operands) {
-          scopeIdentifiers.union([phi.id, phiId]);
+          scopeIdentifiers.union([phi.place.identifier, phiId.identifier]);
         }
       }
     }

diff --git a/compiler/packages/babel-plugin-react-compiler/src/SSA/EliminateRedundantPhi.ts b/compiler/packages/babel-plugin-react-compiler/src/SSA/EliminateRedundantPhi.ts
@@ -68,18 +68,13 @@ export function eliminateRedundantPhi(
       // Find any redundant phis
       phis: for (const phi of block.phis) {
         // Remap phis in case operands are from eliminated phis
-        phi.operands = new Map(
-          Array.from(phi.operands).map(([block, id]) => [
-            block,
-            rewrites.get(id) ?? id,
-          ]),
-        );
+        phi.operands.forEach((place, _) => rewritePlace(place, rewrites));
         // Find if the phi can be eliminated
         let same: Identifier | null = null;
         for (const [_, operand] of phi.operands) {
           if (
-            (same !== null && operand.id === same.id) ||
-            operand.id === phi.id.id
+            (same !== null && operand.identifier.id === same.id) ||
+            operand.identifier.id === phi.place.identifier.id
           ) {
             /*
              * This operand is the same as the phi or is the same as the
@@ -94,7 +89,7 @@ export function eliminateRedundantPhi(
             continue phis;
           } else {
             // First non-phi operand
-            same = operand;
+            same = operand.identifier;
           }
         }
         CompilerError.invariant(same !== null, {
@@ -103,7 +98,7 @@ export function eliminateRedundantPhi(
           loc: null,
           suggestions: null,
         });
-        rewrites.set(phi.id, same);
+        rewrites.set(phi.place.identifier, same);
         block.phis.delete(phi);
       }