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

[vaivaswatha]

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.

[vaivaswatha]

Ping !

[anemet]

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?

[vaivaswatha]

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

[vaivaswatha]

[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.

[ashutosh.nema]

include/llvm/Transforms/Utils/Cloning.h

225

may not => should not

lib/Transforms/Utils/CloneFunction.cpp

705

may not => should not

706

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

710

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

[vaivaswatha]

Updating based on review comments

[ashutosh.nema]

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 ?

[ashutosh.nema]

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

Correct indentation.

710

Correct indentation.

lib/Transforms/Utils/LoopVersioning.cpp

106 ↗

(On Diff #53997)

Correct indentation.

[anemet]

In D15922#403741, @ashutosh.nema wrote:

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]

In D15922#404195, @anemet wrote:

In D15922#403741, @ashutosh.nema wrote:

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]

Updating based on review comments

[ashutosh.nema]

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

[hfinkel]

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?

[vaivaswatha]

In D15922#412417, @hfinkel wrote:

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.

[vaivaswatha]

Updating based on review comments.

[hfinkel]

include/llvm/Transforms/Utils/Cloning.h

224

/// Note: Only inner loops are supported.

(TODO comments in header files are discouraged)

lib/Transforms/Utils/CloneFunction.cpp

705

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

[vaivaswatha]

Updating based on review comments

[hfinkel]

LGTM.

include/llvm/Transforms/Utils/Cloning.h

 bool InlineFunction(CallSite CS, InlineFunctionInfo &IFI,
                     AAResults *CalleeAAR = nullptr, bool InsertLifetime = true);
 
 /// \brief Clones a loop \p OrigLoop.  Returns the loop and the blocks in \p
 /// Blocks.
 ///
 /// Updates LoopInfo and DominatorTree assuming the loop is dominated by block
 /// \p LoopDomBB.  Insert the new blocks before block specified in \p Before.
+/// Note: Only innermost loops are supported.
 Loop *cloneLoopWithPreheader(BasicBlock *Before, BasicBlock *LoopDomBB,
                              Loop *OrigLoop, ValueToValueMapTy &VMap,
                              const Twine &NameSuffix, LoopInfo *LI,
                              DominatorTree *DT,
                              SmallVectorImpl<BasicBlock *> &Blocks);
 
 /// \brief Remaps instructions in \p Blocks using the mapping in \p VMap.
 void remapInstructionsInBlocks(const SmallVectorImpl<BasicBlock *> &Blocks,
                                ValueToValueMapTy &VMap);
 
 } // End llvm namespace
 
 #endif

lib/Transforms/Utils/CloneFunction.cpp

for (auto &Inst : *BB)
                        RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
 }
 
 /// \brief Clones a loop \p OrigLoop.  Returns the loop and the blocks in \p
 /// Blocks.
 ///
 /// Updates LoopInfo and DominatorTree assuming the loop is dominated by block
 /// \p LoopDomBB.  Insert the new blocks before block specified in \p Before.
 Loop *llvm::cloneLoopWithPreheader(BasicBlock *Before, BasicBlock *LoopDomBB,
                                    Loop *OrigLoop, ValueToValueMapTy &VMap,
                                    const Twine &NameSuffix, LoopInfo *LI,
                                    DominatorTree *DT,
                                    SmallVectorImpl<BasicBlock *> &Blocks) {
+  assert(OrigLoop->getSubLoops().empty() && 
+         "Loop to be cloned cannot have inner loop");
   Function *F = OrigLoop->getHeader()->getParent();
   Loop *ParentLoop = OrigLoop->getParentLoop();
 
   Loop *NewLoop = new Loop();
   if (ParentLoop)
     ParentLoop->addChildLoop(NewLoop);
   else
     LI->addTopLevelLoop(NewLoop);