[compiler] Promote temporaries when necessary to prevent codegen reordering over side-effectful operations

[mvitousek]

Stack from ghstack (oldest at bottom):

• -> [compiler] Promote temporaries when necessary to prevent codegen reordering over side-effectful operations #30554

This PR addresses the issue recorded here, where codegen reorders instructions by moving a temporary across a promoted temporary (or an instruction whose lvalue has been pruned) in an unsafe way.

We fix this by adding a new sub-pass to PromoteUsedTemporaries which looks for cases where the definition and the use of an unpromoted temporary, whose value could potentially be mutated, have an instruction in between them that 1. can cause mutation and 2. will be emitted as a separate statement. For our purposes, 1. means function calls and stores, and 2. means either that we've pruned the lvalue or that we've promoted the identifier (or technically that the identifier was named in the first place, but I don't think we could be reodering across it in the first place for this case). If we find such a temporary with an interposing mutative instruction, we'll promote it to a temporary as well.

[vercel]

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	Aug 12, 2024 7:43pm

[josephsavona]

This overall makes sense, just a few questions/comments. I'd love eyes on it from @mofeiZ as well.

[josephsavona]

looks like text got cut off here?

[josephsavona]

seems like you could avoid creating preEntries (which will addd up since we create it for every such instruction) by deferring the set here until after you've iterated state

[josephsavona]

this makes sense, you want to merge the state for the outer block context, but don't want to merge the state of the locals per branch.

can you add some more fixtures to stress test this?

[mvitousek]

It turns out I don't think I can construct an example that exercises this behavior--this would require a temporary to be defined before an if terminal and then used within one branch of the terminal.

I'm inclined to drop this code path because of this--even if I'm wrong and we can construct an example that exercises this, the only consequence would be an unnecessarily promoted temporary.

[josephsavona]

Makes sense!

[mofeiZ]

Logic overall makes sense to me, pretty clever! Is the underlying intuition here that instructions are emitted in the correct order (e.g. IR order which follows source-code evaluation semantics) except for cases in which Forget adds statements, which effectively hoists the statement / rvals used by the statement to be emitted out of order?

I had a bit of trouble following the this.#consts tracking and constStore, but I think I now understand why this is needed (writing down some notes to check my understanding).

Checking constStore when handling non-reorderable instructions makes sense -- const stores are reorderable, but reassignments are not (overall logic seems similar to memory barriers).

[0]: $0 = ... // preEntry = [$0] (rvals evaluated before instruction [1])
[1]: ...      // <-- non-reorderable instruction that gets codegenned to
              // a statement / expressionStatement (e.g. CallExpression)
[2]: <use $0> // later reference to rval should promote $0 to a named temporary

this.#consts track const-bindings to allow reordering of any loads from a const-binding decl, e.g.

[0]: StoreLocal const x$0 = ...
[1]: $1 = LoadLocal x$0
[2]: <non-reorderable / fence instruction>
[3]: <use $1>  // No need to promote $1 since x$0 is a constant binding!

[mofeiZ]

It seems that the fixture results are pretty stable (no false positives!) -- I'd be curious whether we observe any snapshot differences when syncing internally. I've found syncs to be a pretty great stress test for these types of changes

[mvitousek]

@mofeiZ tested this internally (P1510568787) and found that this promotes a lot of global loads, like global.prop in [this example(https://playground.react.dev/#N4Igzg9grgTgxgUxALhAMygOzgFwJYSYAEUYCAKgmDgBR6YAOAlEcADrFEwI6zEA8AQyIMYEBgF5gAcwA2EAEaDZAOlHiAvkQg4AFghhS0dRky0B6AHwcNIDUA). This is technically correct--what if the function call has an alias to global and mutates .prop? However, this is inconsistent with other behavior that Forget assumes around mutability of props on globals (see HIRBuilder.isReorderableExpression). I'm inclined to relax the promotion requirements around reads from globals--if you're using Forget and reading from a mutable global you're already probably in trouble 😅

[josephsavona]

I'm inclined to relax the promotion requirements around reads from globals--if you're using Forget and reading from a mutable global you're already probably in trouble

agreed, let’s relax the promotion requirements. We already freely reorder global accesses in ConstantPropagation.