Skip to content

Conversation

@ivafanas
Copy link
Contributor

@ivafanas ivafanas commented Apr 5, 2025

Bug is detected for Simple case when FalseBB is a successor of both blocks BB and TrueBB.

It is a mistake to add FalseBB as successor of BB after copy-and-merging of TrueBB into BB. MachineVerifier reports an error on duplicate successors and predecessors in this case.

Approximate CFG to trigger the bug:

    BB
   |  \       /
   |   \     /
   |    TrueBB
   |   /     \
   |  /       \
  FalseBB      ...

It is important that branch in TrueBB is not analyzable.

Bug is triggered on the custom out-of-tree backend. Unfortunately, I'm not familiar good enough with other backends to write a good .mir test for this with not analyzable branch. Sorry for that. But fix seems too trivial.

@ivafanas
Copy link
Contributor Author

ivafanas commented Apr 5, 2025

@ostannard @ahatanaka @MatzeB

Could you please review the PR?
Seems like IfConversion.cpp does not have direct owners, and you are the most involved available developers who changed the file.

I do not have write access.
If patch is ok, please, feel free to merge it :)

@kuhar kuhar removed their request for review April 7, 2025 02:51
@ivafanas
Copy link
Contributor Author

ivafanas commented Apr 9, 2025

Please, any feedback?

Even PR rejection is much better than no feedback.

@ostannard
Copy link
Collaborator

I'd prefer to see a test for this before committing it. I'm not very familiar with the if-conversion pass, so it's not clear whether this is the correct fix, or if we should be avoiding doing this optimisation for non-analyzable branches.

@ivafanas ivafanas closed this Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants