This is an archive of the discontinued LLVM Phabricator instance.

[LoopUnswitch]Fix comparison for DomTree updates.
ClosedPublic

Authored by asbirlea on Jun 21 2018, 2:57 PM.

Details

Summary

In LoopUnswitch when replacing a branch Parent -> Succ with a conditional
branch Parent -> True & Parent->False, the DomTree updates should insert an edge for
each of True/False if True/False are different than Succ, and delete Parent->Succ edge
if both are different. The comparison with Succ appears to be incorect,
it's comparing with Parent instead.
There is no test failing either before or after this change, but it seems to me this is
the right way to do the update.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Jun 21 2018, 2:57 PM
kuhar added inline comments.Jun 21 2018, 3:10 PM
lib/Transforms/Scalar/LoopUnswitch.cpp
945 ↗(On Diff #152387)

If it's illegal to loop back to preheader, maybe there should be an assert for that?

947 ↗(On Diff #152387)

Is it possible that TrueDest == FalseDest? In this is the case, we should check it here as well.

asbirlea added inline comments.Jun 21 2018, 3:37 PM
lib/Transforms/Scalar/LoopUnswitch.cpp
945 ↗(On Diff #152387)

I'm not sure we can get that info in this method. This is essentially an util, independent of the loopunswitch logic at the callsite.

947 ↗(On Diff #152387)

This should not be possible.
The logic is contained to LoopUnswitch and the two callsites for this method each have a new block as the TrueDest and an existing block as FalseDest. I can add an assert (TrueDest != FalseDest), if you think it would be helpful.

kuhar accepted this revision.Jun 21 2018, 3:43 PM

LGTM

lib/Transforms/Scalar/LoopUnswitch.cpp
947 ↗(On Diff #152387)

I think it would make it clear for whoever tries to figure out the correct logic here later.

This revision is now accepted and ready to land.Jun 21 2018, 3:43 PM
asbirlea updated this revision to Diff 152403.Jun 21 2018, 4:29 PM
asbirlea marked 5 inline comments as done.

Add assert.

Thank you for the review!

lib/Transforms/Scalar/LoopUnswitch.cpp
947 ↗(On Diff #152387)

Ack, done.

chandlerc accepted this revision.Jun 21 2018, 4:42 PM

It'd be nice to get a test case here, but if its not feasible, the change is obviously correct (or at least an improvement!!!)

LGTM.

This revision was automatically updated to reflect the committed changes.