[complier] bugfix, mergeConsecutiveBlocks pass, fix phi nodes of merging block's successor blocks

[asmjmp0]

Summary

test code:

function test() {
    let v3, v4, acc;
    v3 = false;
    v4 = v3;
    acc = v3;
    if (acc) {
      acc = true;
      v3 = acc;
    }
    if (acc) {
      v3 = v4;
    }
    v4 = v3;
}

after twice constant propagation, we can get the hir code, before invoke eliminateRedundantPhi method.

bb1 (block):
  [1] <unknown> $31 = DeclareLocal Let <unknown> v3$30
  [2] <unknown> $33 = DeclareLocal Let <unknown> v4$32
  [3] <unknown> $35 = DeclareLocal Let <unknown> acc$34
  [4] <unknown> $36 = false
  [5] <unknown> $38 = StoreLocal Reassign <unknown> v3$37 = <unknown> $36
  [6] <unknown> $39 = false
  [7] <unknown> $41 = StoreLocal Reassign <unknown> v4$40 = <unknown> $39
  [8] <unknown> $42 = false
  [9] <unknown> $44 = StoreLocal Reassign <unknown> acc$43 = <unknown> $42
  [10] <unknown> $45 = false
  [11] <unknown> $53 = false
  [12] Goto bb4
bb4 (block):
  predecessor blocks: bb1
  <unknown> v3$58: phi()
  [13] <unknown> $60 = false
  [14] <unknown> $62 = StoreLocal Reassign <unknown> v4$61 = <unknown> $60
  [15] <unknown> $63 = <undefined>
  [16] Return <unknown> $63

we can see the phi node <unknown> v3$58: phi() with empty operands, which make eliminateRedundantPhi method error.

CompilerError [ReactCompilerError]: Invariant: Expected phis to be non-empty

so I remove the phi node with empty operands.

How did you test this change?

I run the command yarn test at compiler dictionary, and all tests passed.

[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	Jan 2, 2025 3:34am

[facebook-github-bot]

Hi @asmjmp0!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[josephsavona]

Thanks for the bug report and fix! To get this landed we need a fixture that reproduces the issue. Please add a test case in https://github.com/facebook/react/tree/main/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler and update snapshot fixtures. Make sure the test case fails without the fix (please include what the exact error was wo the fix, or what the incorrect output was).

[asmjmp0]

function component() {
  let v3, v4, acc;
  v3 = false;
  v4 = v3;
  acc = v3;
-  if (acc) {
-    acc = true;
-    v3 = acc;
-  }
-  if (acc) {
-    v3 = v4;
-  }
  v4 = v3;
  return [acc,v3,v4];
}

these if statements will never be executed since v3 = false and acc = v3, maybe it is better to add some working test fixture?

because of these unreachable if statements that phi nodes with empty operands are generated. If these statements are removed, then it will be impossible to reproduce this bug.

[asmjmp0]

because of these unreachable if statements that phi nodes with empty operands are generated. If these statements are removed, then it will be impossible to reproduce this bug.

Got it, thank you! I simplified the test fixture to

function Component() {
  let foo;
  let bar = false;

  if (bar) bar = true;
  if (bar) foo = 0;
  return foo;
}

And got the same error Invariant: Expected phis to be non-empty preview

But, if I try to reproduce my test fixture in your preview, I got Invariant: [hoisting] Expected value for identifier to be initialized. foo$27 (7:7) this pr preview

perhaps you need to assign an initial value to foo, because after constant propagation and unreachable statements removed, and the variable foo is not assigned a value but is directly returned.

[dimaMachina]

same error if I initialize ah I got what you meant

[asmjmp0]

same error if I initialize ah I got what you meant

after review the case you said, I found that the core of the issue lies in the merge pass, which was not fixing the phi nodes of the successor blocks. This caused the constant propagation pass to mistakenly believe there were no predecessor blocks, leading to the deletion of operands. I have now resubmitted the code, passed the local test cases, could you please take another look?

[dimaMachina]

same error if I initialize ah I got what you meant

after review the case you said, I found that the core of the issue lies in the merge pass, which was not fixing the phi nodes of the successor blocks. This caused the constant propagation pass to mistakenly believe there were no predecessor blocks, leading to the deletion of operands. I have now resubmitted the code, passed the local test cases, could you please take another look?

nice, now looks good to me too 🚀

[josephsavona]

Thank you for finding this bug! The fix here is subtle and i'm not sure exactly where the best place would be, but my instinct is that the fix should be in constant propagation. Also, as written this requires O(n^2) iterations over the blocks, which seems inefficient. Let me take a look.

[josephsavona]

Ok I looked into it, and it does make sense to fix in MergeConsecutiveBlocks but we should iterate once. I put this up in #31959