This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fix replace select with Phis when branch has the same labels
ClosedPublic

Authored by kpolushin on Jul 16 2020, 11:00 PM.

Details

Summary
define i32 @test(i1 %cond) {
entry:
  br i1 %cond, label %exit, label %exit
exit:
  %result = select i1 %cond, i32 123, i32 456
  ret i32 %result
}

In this test, after applying transformation of replacing select with Phis, the result will be:

define i32 @test(i1 %cond) {
entry:
  br i1 %cond, label %exit, label %exit
exit:
  %result = i32 phi [123, %exit], [123, %exit]
  ret i32 %result
}

That is, select is transformed into an invalid Phi, which will then be reduced to 123 and the second value will be lost.
But it is worth noting that this problem will arise only if select is in the InstCombine worklist will be before the branch.
Otherwise, InstCombine will replace the branch condition with false and transformation will not be applied.

The fix is to check the target labels in the branch condition for equality.

Diff Detail

Event Timeline

kpolushin created this revision.Jul 16 2020, 11:00 PM
mkazantsev requested changes to this revision.Jul 16 2020, 11:03 PM

Please add unit test.

This revision now requires changes to proceed.Jul 16 2020, 11:03 PM
xbolva00 added a subscriber: hans.Jul 16 2020, 11:09 PM

Should this be backported to release branch?

@hans

Please add unit test.

Until then, can we please revert to green?

Can we check in this fix and merge the test later? The bug here is pretty obvious. WDYT?

Please add unit test.

Until then, can we please revert to green?

^^

Please add unit test.

Until then, can we please revert to green?

Can we check in this fix and merge the test later? The bug here is pretty obvious. WDYT?

SGTM
The test is going to be ugly and fragile since it will need to outsmart worklist.

mkazantsev accepted this revision.Jul 16 2020, 11:37 PM

Exactly. Approved per comment above, I will merge it now since Kirill has no committer access.

This revision is now accepted and ready to land.Jul 16 2020, 11:37 PM
This revision was automatically updated to reflect the committed changes.

@kpolushin please follow-up with the test when you know how to create it.

hans added a comment.Jul 17 2020, 2:34 AM

Please add unit test.

Until then, can we please revert to green?

Which one? D83284?

I think in general, reverting to green and then re-landing with a fix and test case is preferable to rushing a fix in like this.

nikic added a comment.Jul 17 2020, 4:11 AM

@hans For 11.x, you'll want to either revert rG9bff376e5c10ea384a6eee93f7d7668d670a66e7 and rGe808cab824488af137b62902e65dec3827b83b46 (there were more changes based on top, but I think they did not make it on the release branch), or apply rGc98988107868db41c12b9d782fae25dea2a81c87 and rGdf6e185e8f895686510117301e568e5043909b66. In this case, the fix is probably simpler than the revert.

hans added a comment.Jul 20 2020, 6:31 AM

or apply rGc98988107868db41c12b9d782fae25dea2a81c87 and rGdf6e185e8f895686510117301e568e5043909b66. In this case, the fix is probably simpler than the revert.

Yes, that seems simpler, so I've done that.

Please let me know if there are any more fixes needed in this area.