Skip to content

Conversation

@ivafanas
Copy link
Contributor

Tail duplicator removes the first PHI income from the predecessor basic block, while it should remove all operands for this block.

PHI instructions happen to have duplicated values for the same predecessor block:

We have caught the bug on custom out-of-tree backend. TailDuplicator should remove all operands corresponding to the removing block.

Please note, that bug likely does not affect in-tree backends, because:

  • It happens only in scenario of partial tail duplication (i.e. tail block is duplicated in some predecessors, but not in all of them)
  • It happens in Pre-RA tail duplication only (Post-RA does not contain PHIs, obviously)
  • The only backend (I know) uses Pre-RA tail duplication is X86. It uses tail duplication via early-tailduplication pass which declines partial tail duplication via canCompletelyDuplicateBB check, because it uses TailDuplicator::tailDuplicateBlocks public API.

So, bug happens only in the case of pre-ra partial tail duplication if backend uses TailDuplicator::tailDuplicate public API directly.

That's why I can not add reproducer test for in-tree backends.

@ivafanas
Copy link
Contributor Author

@fhahn, @arsenm

Could you please review the PR?

If it is ok, could you please merge it? I do not have commit access.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought there was a helper for this somewhere? The IR has one, and isn't there one used in the MachineBasicBlock CFG helpers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've run grep by getMBB() and seems like there is no helper. PHI input removal code is seems to be re-implemented from scratch each time (or copy-pasted)

@ivafanas ivafanas force-pushed the dev/fix-partial-phi-input-removal-in-tail-duplicator branch from 95c356b to 79dd1ed Compare September 13, 2025 00:37
@arsenm arsenm merged commit ffcaeca into llvm:main Sep 13, 2025
9 checks passed
@ivafanas
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants