Page MenuHomePhabricator

[Cloning] cloneLoopWithPreheader(): add assert to ensure no sub-loops
ClosedPublic

Authored by vaivaswatha on Jan 6 2016, 5:58 AM.

Details

Summary

cloneLoopWithPreheader() does not update LoopInfo for sub-loop of
the original loop being cloned. Add assert to ensure no sub-loops for loop being cloned.

Diff Detail

Repository
rL LLVM

Event Timeline

vaivaswatha updated this revision to Diff 44111.Jan 6 2016, 5:58 AM
vaivaswatha retitled this revision from to [NOP][Cloning] Add comment to cloneLoopWithPreheader() mentioning that it does not update LoopInfo for sub-loops..
vaivaswatha updated this object.
vaivaswatha added reviewers: ashutosh.nema, hfinkel.
vaivaswatha added a subscriber: llvm-commits.
anemet edited edge metadata.Jan 12 2016, 9:28 PM

Good point, thanks.

Can you please also add an assert to check that there are no subloops in OrigLoop?

We could also rename the function to cloneInnerLoopWithPreheader?

anemet requested changes to this revision.Apr 8 2016, 5:14 PM
anemet edited edge metadata.
This revision now requires changes to proceed.Apr 8 2016, 5:14 PM

Hey sorry I'd forgotten about this patch. I'll work on it and resubmit in a day or two.

vaivaswatha updated this revision to Diff 53540.EditedApr 13 2016, 5:07 AM
vaivaswatha edited edge metadata.

[Cloning] Add assert to ensure no sub-loops for loop being cloned. Also rename
function to reflect its inability to clone sub-loops. (It can clone sub-loops,
but does not correctly update LoopInfo).

  1. Updating D15922: [NOP][Cloning] Add comment to cloneLoopWithPreheader() mentioning that it does not update LoopInfo for sub-loops.
vaivaswatha retitled this revision from [NOP][Cloning] Add comment to cloneLoopWithPreheader() mentioning that it does not update LoopInfo for sub-loops. to [Cloning] rename cloneLoopWithPreheader() and add assert to ensure no sub-loops.Apr 13 2016, 5:09 AM
vaivaswatha updated this object.
vaivaswatha edited edge metadata.
ashutosh.nema added inline comments.Apr 13 2016, 7:27 AM
include/llvm/Transforms/Utils/Cloning.h
224 ↗(On Diff #53540)

may not => should not

lib/Transforms/Utils/CloneFunction.cpp
705 ↗(On Diff #53540)

may not => should not

706 ↗(On Diff #53540)

cloneInnerLoopWithPreheader looks little confusing, consider L1 { L2{ L3{} } }, L2 is also a inner loop, But here we clone innermost loop only. How about cloneInnermostLoopWithPreheader ?

711 ↗(On Diff #53540)

Please assert with some message, i.e. Loop is not innermost

Updating based on review comments

Thanks Vaivaswatha, looks OK to me except the name of function.
As I mentioned earlier ‘cloneInnermostLoopWithPreheader’ looks more intuitive
vs ‘cloneInnerLoopWithPreheader’ because we clone the innermost loop only.

Adam, Hal:
What’s your opinion on this name change ?

I see lot of indentation problems here, please correct them.
Try using clang-format its useful.

lib/Transforms/Scalar/LoopDistribute.cpp
125 ↗(On Diff #53997)

Correct indentation

128 ↗(On Diff #53997)

Correct indentation

lib/Transforms/Utils/CloneFunction.cpp
706 ↗(On Diff #53997)

Correct indentation.

711 ↗(On Diff #53997)

Correct indentation.

lib/Transforms/Utils/LoopVersioning.cpp
106 ↗(On Diff #53997)

Correct indentation.

Thanks Vaivaswatha, looks OK to me except the name of function.
As I mentioned earlier ‘cloneInnermostLoopWithPreheader’ looks more intuitive
vs ‘cloneInnerLoopWithPreheader’ because we clone the innermost loop only.

Adam, Hal:
What’s your opinion on this name change ?

Sounds good to me.

hfinkel edited edge metadata.Apr 18 2016, 12:45 PM

Thanks Vaivaswatha, looks OK to me except the name of function.
As I mentioned earlier ‘cloneInnermostLoopWithPreheader’ looks more intuitive
vs ‘cloneInnerLoopWithPreheader’ because we clone the innermost loop only.

Adam, Hal:
What’s your opinion on this name change ?

Sounds good to me.

Me too.

vaivaswatha edited edge metadata.

Updating based on review comments

Indentation is still not as per llvm coding standard, please run clang-format.
You can specifically run it on the code added by you using below command:

$ clang-format -i -lines=start:end <file>

Refer below page for more details:
http://clang.llvm.org/docs/ClangFormat.html

Upon reflection, cloning an outer loop is something we'll want to be able to do at some point. Why don't we just add the assertion for now, and then fix the function once we have a concrete use case?

Upon reflection, cloning an outer loop is something we'll want to be able to do at some point. Why don't we just add the assertion for now, and then fix the function once we have a concrete use case?

I agree with this.

Updating based on review comments.

vaivaswatha retitled this revision from [Cloning] rename cloneLoopWithPreheader() and add assert to ensure no sub-loops to [Cloning] cloneLoopWithPreheader(): add assert to ensure no sub-loops.Apr 26 2016, 9:21 PM
vaivaswatha updated this object.
hfinkel added inline comments.Apr 26 2016, 9:26 PM
include/llvm/Transforms/Utils/Cloning.h
224 ↗(On Diff #55159)
/// Note: Only inner loops are supported.

(TODO comments in header files are discouraged)

lib/Transforms/Utils/CloneFunction.cpp
705 ↗(On Diff #55159)

No need to add this here (actually, we shouldn't be duplicating comments like this between header and source files at all).

vaivaswatha updated this object.

Updating based on review comments

hfinkel accepted this revision.Apr 26 2016, 9:56 PM
hfinkel edited edge metadata.

LGTM.

This revision was automatically updated to reflect the committed changes.