[PATCH] D11605: [LoopUnswitch] Add block frequency analysis to recognize hot/cold regions

[Philip Reames via llvm-commits]

reames added a comment.

Minor comments below.  Once addressed, I plan to give a LGTM unless David has further comments in the meantime.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:77
@@ -72,1 +76,3 @@
 
+// FIXME: This is false by default due to pervasive problems with
+// exactly what block frequency models.
----------------
This comment is a bit deceptive.  I'd just drop it.  

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:84
@@ +83,3 @@
+    cl::desc("Enable the use of the block frequency analysis to access PGO "
+             "heuristics to minimize code growth in cold regions and be more "
+             "aggressive in hot regions."));
----------------
Right now, we're only limiting code size increase.  Please adjust the string.

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:88
@@ +87,3 @@
+static cl::opt<unsigned>
+ColdFrequency("loop-unswitch-cold-block-frequency", cl::init(20), cl::Hidden,
+    cl::desc("Block frequency to be considered as cold. Non-trivial "
----------------
I think the default here should be much lower, but we can tune this in separate changes.  My guess here would be something like 1/100 rather than 20/100.  I also think we should have an absolute cutoff, but again, we can tune separately.    

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:176
@@ -156,1 +175,3 @@
 
+    BlockFrequencyInfo BFI;
+    BlockFrequency ColdEntryFreq;
----------------
Add a comment which says what these are used for and that they're only used with the PGO option enabled.

================
Comment at: test/Transforms/LoopUnswitch/cold-loop.ll:39
@@ +38,3 @@
+joint:
+  br label %loop_begin
+
----------------
Please add a couple of instructions here per previous comment on test case.


http://reviews.llvm.org/D11605