[PATCH] D23348: CodeExtractor : Move exit block splitting logic from PartialInliner to CodeExtractor

[Sean Silva via llvm-commits]

silvas added inline comments.

================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:182
@@ +181,3 @@
+  for (BasicBlock *Pred : predecessors(Block))
+    if (Blocks.count(Pred) == IfWithinSet)
+      Preds.insert(Pred);
----------------
You may want to explicitly cast the result of 'Blocks.count(Pred)` to bool. Otherwise, I think the bool will promote to int which won't behave as intended.

================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:852
@@ -795,3 +851,3 @@
   DenseMap<BasicBlock *, BlockFrequency> ExitWeights;
-  SmallPtrSet<BasicBlock *, 1> ExitBlocks;
+  std::unordered_map<BasicBlock *, unsigned> ExitBlocks;
   for (BasicBlock *Block : Blocks) {
----------------
We generally prefer DenseMap to std::unordered_map. Can you use `DenseMap<BasicBlock *, unsigned>`? If not, a comment would be useful.

http://llvm.org/docs/ProgrammersManual.html#llvm-adt-densemap-h

Also, this could use a comment explaining what the `unsigned` means. Maybe add to the comment just above ExitWeights?

================
Comment at: test/Transforms/CodeExtractor/MultipleExitBranchProb.ll:34
@@ -33,2 +33,2 @@
 
-; CHECK: [[COUNT1]] = !{!"branch_weights", i32 8, i32 31}
+; CHECK: [[COUNT1]] = !{!"branch_weights", i32 {{(31, i32 8)|(8, i32 31)}}}
----------------
The output of the compiler must be deterministic. You shouldn't be depending on the iteration order of a map or the numerical values of pointers (can be changed by ASLR etc.).

================
Comment at: test/Transforms/CodeExtractor/SplitExitBlockPhiNodes.ll:1
@@ +1,2 @@
+; RUN: opt < %s -partial-inliner -S | FileCheck %s
+; This testcase checks CodeExtractors ability to split
----------------
You mentioned that this patch is just moving the logic from PartialInliner to CodeExtractor. So does this test case currently pass?

================
Comment at: test/Transforms/CodeExtractor/SplitExitBlockPhiNodes.ll:6
@@ +5,3 @@
+
+define internal i32 @inlinedFunc(i1 %cond) {
+entry:
----------------
Explain which blocks get outlined and what split block we are supposed to be inserting.

================
Comment at: test/Transforms/CodeExtractor/SplitExitBlockPhiNodes.ll:27
@@ +26,2 @@
+; CHECK-LABEL: @inlinedFunc.1_if.then
+; CHECK: phi i32
----------------
You probably want a CHECK-LABEL for each function that you expect in the output. That way the CHECK for the phi node is restricted to being inside the function you want.


Also, you probably want to check a bit more, such as which BB's end up in which functions.


https://reviews.llvm.org/D23348