Page MenuHomePhabricator

[InstCombine] replace phi values from unreachable blocks with 'undef'
ClosedPublic

Authored by spatel on Sep 19 2020, 8:28 AM.

Details

Summary

The test (currently crashing) is reduced from the example provided in the post-commit discussion in D87149.
Here we replace with undef because we can't remove the operand completely (because InstCombine preserves the CFG).
We used to do something similar for branches, but we changed that to use a fixed constant value rather than undef because branching on undef became poison/UB.

I'm not sure what the cost/implications of this approach are. If we want to fix the crash more directly, we could just add a clause for uses and restrict the transform from D87149.

Diff Detail

Event Timeline

spatel created this revision.Sep 19 2020, 8:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2020, 8:28 AM
spatel requested review of this revision.Sep 19 2020, 8:28 AM
nikic added a comment.Sep 19 2020, 8:42 AM

I don't think this fully addresses the problem, in that this relies on the phi getting processed before the unreachable, which is not a given. This would have to happen prior to the main InstCombine run, at the same time we remove instructions in unreachable blocks. It's probably not worth it. I think we should fix this the direct way, by performing a replaceInstUsesWith(*Prev, UndefValue(Prev->getType()) before the eraseInstFromFunction() in the unreachable transform.

nlopes added a subscriber: nlopes.Sep 19 2020, 8:43 AM
nlopes added inline comments.
llvm/test/Transforms/InstCombine/phi.ll
1207

The patch LGTM. Just a side comment: we should avoid tests with br undef as we define that as UB, and therefore it makes any transformation correct. Which in turn essentially disables verification by Alive2.
(I know we still have many such tests. I think we talked in the past of writing a script to do a mass change of these to use input variables)

spatel updated this revision to Diff 293018.Sep 20 2020, 5:53 AM

Patch updated:

  1. Use the direct approach to fix the bug - replace with undef before trying to delete.
  2. Avoid branch on undef in crashing test case.
spatel marked an inline comment as done.Sep 20 2020, 5:54 AM
lebedev.ri accepted this revision.Sep 20 2020, 6:02 AM

lg

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2816

This needs a motivational comment

This revision is now accepted and ready to land.Sep 20 2020, 6:02 AM
spatel marked an inline comment as done.Sep 20 2020, 9:24 AM
spatel added inline comments.
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2816

Thanks! Will add comment and change the patch title/description on commit.

This revision was automatically updated to reflect the committed changes.
spatel marked an inline comment as done.
aqjune added inline comments.Sep 20 2020, 3:34 PM
llvm/test/Transforms/InstCombine/phi.ll
1207

I remember bugpoint was the culprit who inserted a lot of br undef.
I tried to fix it in the past, but it wasn't trivial.