This is an archive of the discontinued LLVM Phabricator instance.

[LoopUnroll+LoopUnswitch] do not transform loops containing callbr
ClosedPublic

Authored by nickdesaulniers on Jul 8 2019, 2:56 PM.

Details

Summary

There is currently a correctness issue when unrolling loops containing
callbr's where their indirect targets are being updated correctly to the
newly created labels, but their operands are not. This manifests in
unrolled loops where the second and subsequent copies of callbr
instructions have blockaddresses of the label from the first instance of
the unrolled loop, which would result in nonsensical runtime control
flow.

For now, conservatively do not unroll the loop. In the future, I think
we can pursue unrolling such loops provided we transform the cloned
callbr's operands correctly.

Such a transform and its legalities are being discussed in:
https://reviews.llvm.org/D64101

Link: https://bugs.llvm.org/show_bug.cgi?id=42489
Link: https://groups.google.com/forum/#!topic/clang-built-linux/z-hRWP9KqPI

Event Timeline

nickdesaulniers created this revision.Jul 8 2019, 2:56 PM
  • add back previous fixes from v2
fhahn accepted this revision.Jul 10 2019, 7:58 AM

LGTM thanks. Please wait with committing a day or so, in case Hal or Eli have additional comments.

isSafeToClone is also used by LoopUnswitch, it would be good to add a test with callbr there as well, to make sure we do not miss it, in case we remove the restriction again.

This revision is now accepted and ready to land.Jul 10 2019, 7:58 AM
efriedma accepted this revision.Jul 10 2019, 11:26 AM
efriedma added a subscriber: efriedma.

LGTM

it would be good to add a test with callbr there as well, to make sure we do not miss it, in case we remove the restriction again.

I agree and would be happy to do so. Would you prefer that additional test case in this commit or a subsequent commit (either way, before I re-visit https://reviews.llvm.org/D64101)?

hfinkel accepted this revision.Jul 10 2019, 5:21 PM

it would be good to add a test with callbr there as well, to make sure we do not miss it, in case we remove the restriction again.

I agree and would be happy to do so. Would you prefer that additional test case in this commit or a subsequent commit (either way, before I re-visit https://reviews.llvm.org/D64101)?

Probably with this commit: If this gets reverted for whatever reason, that test would also need to be reverted, right?

Otherwise, LGTM too.

  • add test case for LoopUnswitch
nickdesaulniers retitled this revision from [LoopUnroll] do not unroll loops containing callbr to [LoopUnroll+LoopUnswitch] do not transform loops containing callbr.Jul 11 2019, 1:27 PM

Thanks for the reviews thus far. Parting thoughts on the newly added test case (llvm/test/Transforms/LoopUnswitch/callbr.ll) before I commit, tomorrow?

This revision was automatically updated to reflect the committed changes.