[compiler] Instruction reordering #29863

josephsavona · 2024-06-11T19:46:45Z

Stack from ghstack (oldest at bottom):

Adds a pass just after DCE to reorder safely reorderable instructions (jsx, primitives, globals) closer to where they are used, to allow other optimization passes to be more effective. Notably, the reordering allows scope merging to be more effective, since that pass relies on two scopes not having intervening instructions — in many cases we can now reorder such instructions out of the way and unlock merging, as demonstrated in the changed fixtures.

The algorithm itself is described in the docblock.

note: This is a cleaned up version of #29579 that is ready for review.

Adds a pass just after DCE to reorder safely reorderable instructions (jsx, primitives, globals) closer to where they are used, to allow other optimization passes to be more effective. Notably, the reordering allows scope merging to be more effective, since that pass relies on two scopes not having intervening instructions — in many cases we can now reorder such instructions out of the way and unlock merging, as demonstrated in the changed fixtures. The algorithm itself is described in the docblock. note: This is a cleaned up version of #29579 that is ready for review. [ghstack-poisoned]

vercel · 2024-06-11T19:46:47Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 12, 2024 10:53pm

Adds a pass just after DCE to reorder safely reorderable instructions (jsx, primitives, globals) closer to where they are used, to allow other optimization passes to be more effective. Notably, the reordering allows scope merging to be more effective, since that pass relies on two scopes not having intervening instructions — in many cases we can now reorder such instructions out of the way and unlock merging, as demonstrated in the changed fixtures. The algorithm itself is described in the docblock. note: This is a cleaned up version of #29579 that is ready for review. ghstack-source-id: 447d764f4728d54b944f3a511859a17722da0fd2 Pull Request resolved: #29863

Adds a pass just after DCE to reorder safely reorderable instructions (jsx, primitives, globals) closer to where they are used, to allow other optimization passes to be more effective. Notably, the reordering allows scope merging to be more effective, since that pass relies on two scopes not having intervening instructions — in many cases we can now reorder such instructions out of the way and unlock merging, as demonstrated in the changed fixtures. The algorithm itself is described in the docblock. note: This is a cleaned up version of #29579 that is ready for review. [ghstack-poisoned]

Adds a pass just after DCE to reorder safely reorderable instructions (jsx, primitives, globals) closer to where they are used, to allow other optimization passes to be more effective. Notably, the reordering allows scope merging to be more effective, since that pass relies on two scopes not having intervening instructions — in many cases we can now reorder such instructions out of the way and unlock merging, as demonstrated in the changed fixtures. The algorithm itself is described in the docblock. note: This is a cleaned up version of #29579 that is ready for review. ghstack-source-id: 69e3f0f60d3471e9d2dc973a494533cc38667bcb Pull Request resolved: #29863

react-sizebot · 2024-06-11T19:51:24Z

Comparing: 55fdcf8...1501466

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.js	=	6.66 kB	6.66 kB	+0.05%	1.82 kB	1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js	=	497.80 kB	497.80 kB	=	89.24 kB	89.24 kB
oss-experimental/react-dom/cjs/react-dom.production.js	=	6.67 kB	6.67 kB	=	1.83 kB	1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js	=	502.62 kB	502.62 kB	=	89.94 kB	89.94 kB
facebook-www/ReactDOM-prod.classic.js	=	597.04 kB	597.04 kB	=	105.31 kB	105.31 kB
facebook-www/ReactDOM-prod.modern.js	=	571.38 kB	571.38 kB	=	101.25 kB	101.25 kB
test_utils/ReactAllWarnings.js	Deleted	62.88 kB	0.00 kB	Deleted	15.69 kB	0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
test_utils/ReactAllWarnings.js	Deleted	62.88 kB	0.00 kB	Deleted	15.69 kB	0.00 kB

Generated by 🚫 dangerJS against 1501466

josephsavona · 2024-06-11T19:52:18Z

...ler/src/__tests__/fixtures/compiler/align-scopes-within-nested-valueblock-in-array.expect.md

  let t1;
-  if ($[0] !== cond1 || $[1] !== cond2 || $[2] !== arr) {
+  if ($[0] !== cond1 || $[1] !== cond2) {
+    const arr = makeArray({ a: 2 }, 2, []);


The previous code was incorrect, but this fixture was added specifically to show the previous output for this case prior to moving to HIR-based scope alignment/merging (note that the fixture has @enableReactiveScopesInHIR:false

josephsavona · 2024-06-11T20:06:23Z

...c/__tests__/fixtures/compiler/allow-global-mutation-in-effect-indirect-usecallback.expect.md

    t4 = [someGlobal];
+    $[3] = t3;


this is because we allow LoadGlobal to not prevent scope merging

josephsavona · 2024-06-11T20:36:51Z

...-compiler/src/__tests__/fixtures/compiler/allow-global-mutation-in-effect-indirect.expect.md

  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
-    t0 = () => {
+    const setGlobal = () => {


nice improvements, grouping more scopes together

Adds a pass just after DCE to reorder safely reorderable instructions (jsx, primitives, globals) closer to where they are used, to allow other optimization passes to be more effective. Notably, the reordering allows scope merging to be more effective, since that pass relies on two scopes not having intervening instructions — in many cases we can now reorder such instructions out of the way and unlock merging, as demonstrated in the changed fixtures. The algorithm itself is described in the docblock. note: This is a cleaned up version of #29579 that is ready for review. [ghstack-poisoned]

Adds a pass just after DCE to reorder safely reorderable instructions (jsx, primitives, globals) closer to where they are used, to allow other optimization passes to be more effective. Notably, the reordering allows scope merging to be more effective, since that pass relies on two scopes not having intervening instructions — in many cases we can now reorder such instructions out of the way and unlock merging, as demonstrated in the changed fixtures. The algorithm itself is described in the docblock. note: This is a cleaned up version of #29579 that is ready for review. ghstack-source-id: fe01f94e186e62171e7cc82e10d7864c48f932be Pull Request resolved: #29863

josephsavona · 2024-06-11T21:51:23Z

...tests__/fixtures/compiler/dont-merge-if-dep-is-inner-declaration-of-previous-scope.expect.md

+  if ($[13] !== a || $[14] !== b) {
+    t3 = [a, b];
+    $[13] = a;
+    $[14] = b;
    $[15] = t3;
  } else {
    t3 = $[15];
  }
  let t4;
-  if ($[16] !== a || $[17] !== b) {
-    t4 = [a, b];
-    $[16] = a;
-    $[17] = b;
+  if ($[16] !== t2 || $[17] !== x) {
+    t4 = <ValidateMemoization inputs={t2} output={x} />;
+    $[16] = t2;
+    $[17] = x;
    $[18] = t4;
  } else {
    t4 = $[18];
  }
  let t5;
-  if ($[19] !== t4 || $[20] !== z) {
-    t5 = <ValidateMemoization inputs={t4} output={z} />;
-    $[19] = t4;
+  if ($[19] !== t3 || $[20] !== z) {
+    t5 = <ValidateMemoization inputs={t3} output={z} />;
+    $[19] = t3;


this example would be more optimal if we could reorder the array literals too, and move them closer to the <ValidateMemoization>, but that gets complicated if the arrays could be mutated

josephsavona · 2024-06-11T21:52:55Z

.../src/__tests__/fixtures/compiler/destructuring-mixed-scope-declarations-and-locals.expect.md

-    $[5] = media;
-    $[6] = onClick;
-    $[7] = t0;
+    $[0] = post;


the merging here is maybe not ideal - now the <Media> element always invalidates whenever post does, rather than only if the specific properties change

josephsavona · 2024-06-11T21:53:40Z

...sts__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.expect.md

-    $[12] = allUrls;
-    $[13] = onClick;
-    $[14] = t1;
+    $[1] = post;


the scope merging here is also not ideal, the <Stringify> element only strictly has to invalidate if these properties change, not for all changes to post

josephsavona · 2024-06-11T21:55:34Z

...c/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md

-    T1 = View;
-    T0 = View;
-    if ($[5] === Symbol.for("react.memo_cache_sentinel")) {
-      t1 = <span>Text</span>;
-      $[5] = t1;
-    } else {
-      t1 = $[5];
-    }


all of this reorders out of the scope, with a huge drop in number of memo slots needed

josephsavona · 2024-06-11T21:58:37Z

...iginal-reactive-scopes-fork/invalid-align-scopes-within-nested-valueblock-in-array.expect.md

  let t1;
-  if ($[0] !== cond1 || $[1] !== cond2 || $[2] !== arr) {
+  if ($[0] !== cond1 || $[1] !== cond2) {
+    const arr = makeArray({ a: 2 }, 2, []);


the previous output was incorrect, but this has @enableReactiveScopesInHIR:false

josephsavona · 2024-06-11T22:03:01Z

...plugin-react-compiler/src/__tests__/fixtures/compiler/repro-no-value-for-temporary.expect.md

  if ($[0] !== thread.threadType || $[1] !== listItem) {
    const isFoo = isFooThread(thread.threadType);
-    t1 = useBar;
-    t2 = listItem;
+    t1 = listItem;


this is presumably a LoadLocal that we can't reorder (yet)

josephsavona · 2024-06-11T22:04:53Z

...ler/src/__tests__/fixtures/compiler/repro-instruction-part-of-already-closed-scope.expect.md

-    const t4 = identity(b);
-    if ($[6] !== t4) {
-      t2 = <Stringify value={t4} />;
-      $[6] = t4;
-      $[7] = t2;
-    } else {
-      t2 = $[7];
-    }
-    const t5 = identity(a);
-    if ($[8] !== t5) {
-      t3 = <Stringify value={t5} />;
-      $[8] = t5;
-      $[9] = t3;
-    } else {
-      t3 = $[9];
-    }


This is fun: we avoid some of the complications from MergeOverlappingScopes and JSX, now that the JSX instructions shift past later mutating instructions

Adds a pass just after DCE to reorder safely reorderable instructions (jsx, primitives, globals) closer to where they are used, to allow other optimization passes to be more effective. Notably, the reordering allows scope merging to be more effective, since that pass relies on two scopes not having intervening instructions — in many cases we can now reorder such instructions out of the way and unlock merging, as demonstrated in the changed fixtures. The algorithm itself is described in the docblock. note: This is a cleaned up version of #29579 that is ready for review. [ghstack-poisoned]

Adds a pass just after DCE to reorder safely reorderable instructions (jsx, primitives, globals) closer to where they are used, to allow other optimization passes to be more effective. Notably, the reordering allows scope merging to be more effective, since that pass relies on two scopes not having intervening instructions — in many cases we can now reorder such instructions out of the way and unlock merging, as demonstrated in the changed fixtures. The algorithm itself is described in the docblock. note: This is a cleaned up version of #29579 that is ready for review. ghstack-source-id: fe01f94e186e62171e7cc82e10d7864c48f932be Pull Request resolved: #29863

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-source-id: 43d0c3b0195ebc69e472aa232e27dec99c2d63dd Pull Request resolved: #29883

josephsavona · 2024-06-12T23:49:44Z

I updated to:

address comments
split out the changes to MergeScopesThatInvalidateTogether into a separate diff, since that is the bulk of the win
put reordering behind a feature flag

Updates our scope merging pass to allow more types of instructions to intervene btw scopes. This includes all the non-allocating kinds of nodes that are considered reorderable in #29863. It's already safe to merge scopes with these instructions — we only merge if the lvalue is not used past the next scope. Additionally, without changing this pass reordering isn't very effective, since we would reorder to add these types of intervening instructions and then not be able to merge scopes. Sequencing this first helps to see the win just from reordering alone. ghstack-source-id: 79263576d8eaeb45ef4d1ec4951478459853a287 Pull Request resolved: #29881

Adds a pass just after DCE to reorder safely reorderable instructions (jsx, primitives, globals) closer to where they are used, to allow other optimization passes to be more effective. Notably, the reordering allows scope merging to be more effective, since that pass relies on two scopes not having intervening instructions — in many cases we can now reorder such instructions out of the way and unlock merging, as demonstrated in the changed fixtures. The algorithm itself is described in the docblock. note: This is a cleaned up version of #29579 that is ready for review. ghstack-source-id: c54a806cad7aefba4ac1876c9fd9b25f9177e95a Pull Request resolved: #29863

…n-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]

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]

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-source-id: 567b2299fb7d7b22e7ae913041079b5c2ea87bd9 Pull Request resolved: #29883

…n-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]

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]

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-source-id: ef609261a7f7a0ca823651683157293158a9f271 Pull Request resolved: #29883

…n-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]

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]

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-source-id: 98824627b66f7a43aeaf141c21efddc60c3cc0b3 Pull Request resolved: #29883

…n-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]

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]

…n-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]

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]

…n-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]

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]

Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain. This PR includes two changes: * First, allow `LoadLocal` to be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly once * Uses a more optimal reordering for statement blocks, while keeping the existing approach for expression 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, as evidenced by the reduced memo slots in the fixtures. [ghstack-poisoned]

Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain. This PR includes two changes: * First, allow `LoadLocal` to be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly once * Uses a more optimal reordering for statement blocks, while keeping the existing approach for expression 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, as evidenced by the reduced memo slots in the fixtures. ghstack-source-id: ad3e516fa474235ced8c5d56f4541d2a7c413608 Pull Request resolved: #29882

In facebook#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 facebook#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-source-id: 476c7b88222ff8bc1e4488d9b4c77c03a41e8448 Pull Request resolved: facebook#29883

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jun 11, 2024

josephsavona commented Jun 11, 2024

View reviewed changes

vercel bot deployed to Preview June 11, 2024 19:56 View deployment

josephsavona commented Jun 11, 2024

View reviewed changes

vercel bot deployed to Preview June 11, 2024 20:56 View deployment

josephsavona commented Jun 11, 2024

View reviewed changes

vercel bot deployed to Preview June 11, 2024 22:19 View deployment

josephsavona requested review from mofeiZ, poteto and gsathya and removed request for mofeiZ June 11, 2024 22:21

josephsavona mentioned this pull request Jun 12, 2024

[compiler] Optimize instruction reordering #29882

Merged

josephsavona merged commit 1501466 into gh/josephsavona/28/base Jun 14, 2024
55 checks passed

josephsavona deleted the gh/josephsavona/28/head branch June 14, 2024 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler] Instruction reordering #29863

[compiler] Instruction reordering #29863

josephsavona commented Jun 11, 2024 •

edited

Loading

vercel bot commented Jun 11, 2024 •

edited

Loading

react-sizebot commented Jun 11, 2024 •

edited

Loading

josephsavona Jun 11, 2024 •

edited

Loading

josephsavona Jun 11, 2024 •

edited

Loading

josephsavona Jun 11, 2024

josephsavona Jun 11, 2024

josephsavona Jun 11, 2024

josephsavona Jun 11, 2024

josephsavona Jun 11, 2024

josephsavona Jun 11, 2024

josephsavona Jun 11, 2024

josephsavona Jun 11, 2024

josephsavona commented Jun 12, 2024

[compiler] Instruction reordering #29863

[compiler] Instruction reordering #29863

Conversation

josephsavona commented Jun 11, 2024 • edited Loading

vercel bot commented Jun 11, 2024 • edited Loading

react-sizebot commented Jun 11, 2024 • edited Loading

Critical size changes

Significant size changes

josephsavona Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

josephsavona Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephsavona commented Jun 12, 2024

josephsavona commented Jun 11, 2024 •

edited

Loading

vercel bot commented Jun 11, 2024 •

edited

Loading

react-sizebot commented Jun 11, 2024 •

edited

Loading

josephsavona Jun 11, 2024 •

edited

Loading

josephsavona Jun 11, 2024 •

edited

Loading