Page MenuHomePhabricator

[LoopUnroll] Remap references in peeled iteration
ClosedPublic

Authored by sepavloff on Mar 23 2017, 4:56 AM.

Details

Summary

If a loop contains several BBs, peeling produces code in which jump
targets point to original loop blocks rather than cloned BBs. This
patch uses the same way as loop unrolling to remap cloned blocks.

Diff Detail

Repository
rL LLVM

Event Timeline

sepavloff created this revision.Mar 23 2017, 4:56 AM
mkuper requested changes to this revision.Mar 23 2017, 12:51 PM
mkuper added inline comments.
lib/Transforms/Utils/LoopUnrollPeel.cpp
419 ↗(On Diff #92781)

Something here is wrong. There's already a call to remapInstructionsInBlocks in line 443 (original) that does exactly this, so everything ought to be updated correctly. If it isn't, then something's going wrong between here and there, and I'm not sure what it is.

This revision now requires changes to proceed.Mar 23 2017, 12:51 PM
sepavloff added inline comments.Mar 24 2017, 5:19 AM
lib/Transforms/Utils/LoopUnrollPeel.cpp
419 ↗(On Diff #92781)

Indeed, you are right. But the remapping is made too late, it must be done prior to dominator tree calculation, which in turn must be done prior to call to SplitBlock.

sepavloff updated this revision to Diff 92927.Mar 24 2017, 5:21 AM
sepavloff edited edge metadata.

Updated patch

mkuper accepted this revision.Mar 24 2017, 1:15 PM

LGTM, but please fix the test before committing.

lib/Transforms/Utils/LoopUnrollPeel.cpp
419 ↗(On Diff #92781)

Ok, this makes more sense. So what happened here isn't that we generated bad branches, it's that we produced a malformed dominator tree?
Anyway, thanks for catching this!

test/Transforms/LoopUnroll/peel-loop.ll
99 ↗(On Diff #92927)

Please make the test actually test its output (in particular, that we actually peeled the loop).
I think you can just run the update script on it.

This revision is now accepted and ready to land.Mar 24 2017, 1:15 PM
This revision was automatically updated to reflect the committed changes.

Thank you for review.

lib/Transforms/Utils/LoopUnrollPeel.cpp
419 ↗(On Diff #92781)

Yes, the problem is only malformed dominator tree. It was found in self builds with expensive checks enabled.

test/Transforms/LoopUnroll/peel-loop.ll
99 ↗(On Diff #92927)

Done, but I had to move the test into separate file, as with options of peel-loop.ll the code changed too substantially.