[PATCH] D22744: CodeExtractor : Add ability to preserve profile data.

[Sean Silva via llvm-commits]

silvas added a comment.

Sorry for the delay on getting to this.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:149
@@ -137,3 +148,3 @@
   Function *ExtractedFunction =
-      CodeExtractor(ToExtract, &DT).extractCodeRegion();
+      CodeExtractor(ToExtract, &DT, false, BFI).extractCodeRegion();
 
----------------
Can you add a comment before the false showing the name of the argument? E.g. `CodeExtractor(ToExtract, &DT, /*TheArgName*/false, BFI).extractCodeRegion();`

================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:690
@@ +689,3 @@
+
+  // Make sure that the weights are valid, we cant have a weight of zero.
+  bool HasValidWeights = true;
----------------
Can you explain more how we would get into a situation with a weight of zero and why we need to check it? maybe parts of this should be proper methods on BFI? E.g. below you are directly accessing the impl class so it sounds like something here should maybe be a method on BFI.

also, a nit: can't

================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:742
@@ +741,3 @@
+  // is
+  //  greater than 1.
+  BranchProbabilityInfo *BPI = nullptr;
----------------
Should we change the getBPI method to return non-const? I assume the author made them const for a reason. E.g. BFI might be relying on BPI not changing under it.

================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:808
@@ +807,3 @@
+        BFI->getProfileCountFromFreq(EntryFreq.getFrequency());
+    newFunction->setEntryCount( OptEntryCount.getValue());
+    BFI->setBlockFreq(codeReplacer, EntryFreq.getFrequency());
----------------
nit: no space after the open paren

================
Comment at: test/Transforms/CodeExtractor/2016-07-20-MultipleExitBranchProb.ll:30
@@ +29,2 @@
+
+; CHECK: !4 = !{!"branch_weights", i32 8, i32 31}
----------------
Can you do a check with a pattern match and capture a variable, like you do for the other test?

You should be able to do something like test/Transforms/PGOProfile/branch1.ll to check the profile metadata on a particular instruction:

```
; USE: br i1 %cmp, label %if.then, label %if.end
; USE-SAME: !prof ![[BW_ENTRY:[0-9]+]]
; USE-DAG: ![[BW_ENTRY]] = !{!"branch_weights", i32 2, i32 1}
```


https://reviews.llvm.org/D22744