Update on "[compiler] Optimize emission in normal (non-value) 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, though i still need to double-check all the output changes.

[ghstack-poisoned]

compiler/packages/babel-plugin-react-compiler/src/Optimization/InstructionReordering.ts

@@ -26,7 +26,6 @@ import {
   eachInstructionValueOperand,
   eachTerminalOperand,
 } from "../HIR/visitors";
-import { mayAllocate } from "../ReactiveScopes/InferReactiveScopeVariables";
 import { getOrInsertWith } from "../Utils/utils";
 
 /**
@@ -93,6 +92,7 @@ type Nodes = Map<IdentifierId, Node>;
 type Node = {
   instruction: Instruction | null;
   dependencies: Set<IdentifierId>;
+  reorderability: Reorderability;
   depth: number | null;
 };
 
@@ -173,21 +173,23 @@ function reorderBlock(
   for (const instr of block.instructions) {
     const { lvalue, value } = instr;
     // Get or create a node for this lvalue
+    const reorderability = getReorderability(instr, references);
     const node = getOrInsertWith(
       locals,
       lvalue.identifier.id,
       () =>
         ({
           instruction: instr,
           dependencies: new Set(),
+          reorderability,
           depth: null,
         }) as Node
     );
     /**
      * Ensure non-reoderable instructions have their order retained by
      * adding explicit dependencies to the previous such instruction.
      */
-    if (getReoderability(instr, references) === Reorderability.Nonreorderable) {
+    if (reorderability === Reorderability.Nonreorderable) {
       if (previous !== null) {
         node.dependencies.add(previous);
       }
@@ -298,9 +300,7 @@ function reorderBlock(
         continue;
       }
       CompilerError.invariant(
-        node.instruction != null &&
-          getReoderability(node.instruction, references) ===
-            Reorderability.Reorderable,
+        node.reorderability === Reorderability.Reorderable,
         {
           reason: `Expected all remaining instructions to be reorderable`,
           loc: node.instruction?.loc ?? block.terminal.loc,
@@ -355,11 +355,7 @@ function reorderBlock(
       if (node === undefined) {
         continue;
       }
-      if (
-        node.instruction !== null &&
-        getReoderability(node.instruction, references) ===
-          Reorderability.Reorderable
-      ) {
+      if (node.reorderability === Reorderability.Reorderable) {
         DEBUG && console.log(`save shared: $${id}`);
         shared.set(id, node);
       } else {
@@ -383,8 +379,7 @@ function getDepth(env: Environment, nodes: Nodes, id: IdentifierId): number {
     return node.depth;
   }
   node.depth = 0; // in case of cycles
-  let depth =
-    node.instruction != null && mayAllocate(env, node.instruction) ? 1 : 0;
+  let depth = node.reorderability === Reorderability.Reorderable ? 0 : 1;
   for (const dep of node.dependencies) {
     depth += getDepth(env, nodes, dep);
   }
@@ -419,7 +414,7 @@ function print(
     print(env, locals, shared, seen, dep, depth + 1);
   }
   console.log(
-    `${"|   ".repeat(depth)}$${id} ${printNode(node)} deps=[${deps.map((x) => `$${x}`).join(", ")}]`
+    `${"|   ".repeat(depth)}$${id} ${printNode(node)} deps=[${deps.map((x) => `$${x}`).join(", ")}] depth=${node.depth}`
   );
 }
 
@@ -470,7 +465,7 @@ enum Reorderability {
   Reorderable,
   Nonreorderable,
 }
-function getReoderability(
+function getReorderability(
   instr: Instruction,
   references: References
 ): Reorderability {

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts

@@ -186,10 +186,7 @@ export function isMutable({ id }: Instruction, place: Place): boolean {
   return id >= range.start && id < range.end;
 }
 
-export function mayAllocate(
-  env: Environment,
-  instruction: Instruction
-): boolean {
+function mayAllocate(env: Environment, instruction: Instruction): boolean {
   const { value } = instruction;
   switch (value.kind) {
     case "Destructure": {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-no-whitespace-btw-text-and-param.expect.md

@@ -29,16 +29,15 @@ import fbt from "fbt";
 const _ = fbt;
 function Component(t0) {
   const $ = _c(2);
+  const { value } = t0;
   let t1;
-  if ($[0] !== t0) {
-    const { value } = t0;
-
+  if ($[0] !== value) {
     t1 = fbt._(
       "Before text{paramName}After text",
       [fbt._param("paramName", value)],
       { hk: "aKEGX" },
     );
-    $[0] = t0;
+    $[0] = value;
     $[1] = t1;
   } else {
     t1 = $[1];

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace.expect.md

@@ -30,10 +30,9 @@ import fbt from "fbt";
 const _ = fbt;
 function Component(t0) {
   const $ = _c(2);
+  const { value } = t0;
   let t1;
-  if ($[0] !== t0) {
-    const { value } = t0;
-
+  if ($[0] !== value) {
     t1 = fbt._(
       "Before text {paramName}",
       [
@@ -45,7 +44,7 @@ function Component(t0) {
       ],
       { hk: "3z5SVE" },
     );
-    $[0] = t0;
+    $[0] = value;
     $[1] = t1;
   } else {
     t1 = $[1];

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-single-space-btw-param-and-text.expect.md

@@ -29,16 +29,15 @@ import fbt from "fbt";
 const _ = fbt;
 function Component(t0) {
   const $ = _c(2);
+  const { value } = t0;
   let t1;
-  if ($[0] !== t0) {
-    const { value } = t0;
-
+  if ($[0] !== value) {
     t1 = fbt._(
       "Before text {paramName} after text",
       [fbt._param("paramName", value)],
       { hk: "26pxNm" },
     );
-    $[0] = t0;
+    $[0] = value;
     $[1] = t1;
   } else {
     t1 = $[1];

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-whitespace-around-param-value.expect.md

@@ -29,16 +29,15 @@ import fbt from "fbt";
 const _ = fbt;
 function Component(t0) {
   const $ = _c(2);
+  const { value } = t0;
   let t1;
-  if ($[0] !== t0) {
-    const { value } = t0;
-
+  if ($[0] !== value) {
     t1 = fbt._(
       "Before text {paramName} after text",
       [fbt._param("paramName", value)],
       { hk: "26pxNm" },
     );
-    $[0] = t0;
+    $[0] = value;
     $[1] = t1;
   } else {
     t1 = $[1];

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-whitespace-within-text.expect.md

@@ -31,16 +31,15 @@ import fbt from "fbt";
 const _ = fbt;
 function Component(t0) {
   const $ = _c(2);
+  const { value } = t0;
   let t1;
-  if ($[0] !== t0) {
-    const { value } = t0;
-
+  if ($[0] !== value) {
     t1 = fbt._(
       "Before text {paramName} after text more text and more and more and more and more and more and more and more and more and blah blah blah blah",
       [fbt._param("paramName", value)],
       { hk: "24ZPpO" },
     );
-    $[0] = t0;
+    $[0] = value;
     $[1] = t1;
   } else {
     t1 = $[1];

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbtparam-with-jsx-element-content.expect.md

@@ -29,11 +29,10 @@ import { c as _c } from "react/compiler-runtime";
 import fbt from "fbt";
 
 function Component(t0) {
-  const $ = _c(2);
+  const $ = _c(4);
+  const { name, data, icon } = t0;
   let t1;
-  if ($[0] !== t0) {
-    const { name, data, icon } = t0;
-
+  if ($[0] !== data || $[1] !== icon || $[2] !== name) {
     t1 = (
       <Text type="body4">
         {fbt._(
@@ -62,10 +61,12 @@ function Component(t0) {
         )}
       </Text>
     );
-    $[0] = t0;
-    $[1] = t1;
+    $[0] = data;
+    $[1] = icon;
+    $[2] = name;
+    $[3] = t1;
   } else {
-    t1 = $[1];
+    t1 = $[3];
   }
   return t1;
 }

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-static.expect.md

@@ -21,24 +21,31 @@ function Component(props) {
 ```javascript
 import { c as _c } from "react/compiler-runtime";
 function Component(props) {
-  const $ = _c(1);
+  const $ = _c(2);
   let t0;
   if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
     const count = new MaybeMutable();
 
-    t0 = (
+    t0 = <span>{maybeMutate(count)}</span>;
+    $[0] = t0;
+  } else {
+    t0 = $[0];
+  }
+  let t1;
+  if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
+    t1 = (
       <View>
         <View>
           <span>Text</span>
-          <span>{maybeMutate(count)}</span>
+          {t0}
         </View>
       </View>
     );
-    $[0] = t0;
+    $[1] = t1;
   } else {
-    t0 = $[0];
+    t1 = $[1];
   }
-  return t0;
+  return t1;
 }
 
 ```

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-preserve-whitespace.expect.md

@@ -36,29 +36,39 @@ import { c as _c } from "react/compiler-runtime";
 import { StaticText1 } from "shared-runtime";
 
 function Component() {
-  const $ = _c(1);
+  const $ = _c(3);
   let t0;
   if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
-    t0 = (
+    t0 = <StaticText1 />;
+    $[0] = t0;
+  } else {
+    t0 = $[0];
+  }
+  let t1;
+  if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
+    t1 = <StaticText1 />;
+    $[1] = t1;
+  } else {
+    t1 = $[1];
+  }
+  let t2;
+  if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
+    t2 = (
       <div>
-        Before text
-        <StaticText1 />
-        Middle text
+        Before text{t0}Middle text
         <StaticText1>
-          Inner before text
-          <StaticText1 />
-          Inner middle text
+          Inner before text{t1}Inner middle text
           <StaticText1 />
           Inner after text
         </StaticText1>
         After text
       </div>
     );
-    $[0] = t0;
+    $[2] = t2;
   } else {
-    t0 = $[0];
+    t2 = $[2];
   }
-  return t0;
+  return t2;
 }
 
 export const FIXTURE_ENTRYPOINT = {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-string-attribute-non-ascii.expect.md

@@ -54,31 +54,31 @@ function Post(t0) {
   const $ = _c(7);
   const { author, text } = t0;
   let t1;
-  if ($[0] !== author) {
-    t1 = <h1>{author}</h1>;
-    $[0] = author;
+  if ($[0] !== text) {
+    t1 = <span>{text}</span>;
+    $[0] = text;
     $[1] = t1;
   } else {
     t1 = $[1];
   }
   let t2;
-  if ($[2] !== text) {
-    t2 = <span>{text}</span>;
-    $[2] = text;
+  if ($[2] !== author) {
+    t2 = <h1>{author}</h1>;
+    $[2] = author;
     $[3] = t2;
   } else {
     t2 = $[3];
   }
   let t3;
-  if ($[4] !== t1 || $[5] !== t2) {
+  if ($[4] !== t2 || $[5] !== t1) {
     t3 = (
       <div>
-        {t1}
         {t2}
+        {t1}
       </div>
     );
-    $[4] = t1;
-    $[5] = t2;
+    $[4] = t2;
+    $[5] = t1;
     $[6] = t3;
   } else {
     t3 = $[6];