This is an archive of the discontinued LLVM Phabricator instance.

[JumpThreading] Don't try to rewrite a use if it's already valid.
ClosedPublic

Authored by mzolotukhin on Jun 12 2018, 6:13 PM.

Details

Summary

When recording uses we need to rewrite after cloning a loop we need to
check if the use is not dominated by the original def. The initial
assumption was that the cloned basic block will introduce a new path and
thus the original def will only dominate the use if they are in the same
BB, but as the reproducer from PR37745 shows it's not always the case.

This fixes PR37745.

Diff Detail

Repository
rL LLVM

Event Timeline

mzolotukhin created this revision.Jun 12 2018, 6:13 PM

The fix solves the problem I saw originally in a out of tree target (described in PR37745) but I don't know JumpThreading enough to say if the fix is good or not. Thanks for working on this.

Could anyone review this fix please?

davide accepted this revision.Jun 20 2018, 4:33 PM
davide added a subscriber: kuhar.

LGTM. @kuhar ?

This revision is now accepted and ready to land.Jun 20 2018, 4:33 PM
kuhar added a comment.Jun 20 2018, 4:58 PM

LGTM. @kuhar ?

I'm not familiar with this part of the optimizer enough to say anything more than it doesn't look obviously broken to me.

This problem seems to be aggravated by the new SSAUpdaterBulk() patch pushed on May 12 ( D44282 ) since the PR37745.ll test case does not fail before that date. I don't understand the SSA updater change enough to understand why this problem only happens with the new bulk updates, @mzolotukhin do you happen to know why?

This is a minimal fix, much smaller than the big SSA changes so other than my request for a comment change it LGTM.

llvm/lib/Transforms/Scalar/JumpThreading.cpp
2032 ↗(On Diff #151079)

Please add a comment stating why the domination check should be skipped: it's non-trivial as it's a different check than the User is BB in the old code.

This revision was automatically updated to reflect the committed changes.

do you happen to know why?

The difference compared to the old SSAUpdater is in how we handled uses and defs located in the same block - it did not lead to a problem with the old SSAUpdater, but was unnecessary anyway, since such uses don't need to be rewritten (not in general, but in this particular case when the use is still dominated by its existing def).

Michael

do you happen to know why?

The difference compared to the old SSAUpdater is in how we handled uses and defs located in the same block - it did not lead to a problem with the old SSAUpdater, but was unnecessary anyway, since such uses don't need to be rewritten (not in general, but in this particular case when the use is still dominated by its existing def).

Michael

Thanks @mzolotukhin for the explanation. And for what it's worth, a delayed LGTM.

Thanks, Brian!