[PATCH] D63446: Expand cloneLoopWithPreheader() to support cloning loop nest

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:744
   Loop *ParentLoop = OrigLoop->getParentLoop();
+  std::map<Loop *, Loop *> LMap;
 
----------------
Consider using `DenseMap`.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:772
+      NewLoop = LI->AllocateLoop();
+      LMap[CurLoop] = NewLoop;
+
----------------
[style] I like getting a reference that can be updated instead of another map lookup. I.e.
```
Loop *&NewLoop = LMap[CurLoop];
...
NewLoop = LI->AllocateLoop();
```


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:777-778
+      assert(OrigParent && "Could not find the original parent loop");
+      Loop *NewParentLoop = LMap[OrigParent];
+      assert(NewParentLoop && "Could not find the new parent loop");
+
----------------
[serious] Since the iteration order of `OrigLoop->getBlocks()` is undefined, we might get a block that is nested in two loops without having visited the parent first, i.e. `NewParentLoop` would be null in this case.

Consider iterating over `LoopInfo`'s loop tree before iterating over all block to ensure that the new loop structure has been created.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63446/new/

https://reviews.llvm.org/D63446