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

[David]

davidxl added a subscriber: davidxl.
davidxl added a comment.

The main problem with the patch is that the cold loop detection can have many false positives. With profile feedback,  cold loops can be reliably detected but not with the method described in this patch. Without profile feedback, the cold loop filtering should probably not be turned on by default unless it is optimized for size. When the filtering is not on, we should not pay the compile time penalty of computing  BFI unconditionally either. Cong has recently made it possible to compute BFI without relying on running the wrapper pass.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:496
@@ +495,3 @@
+  // is less than ColdFrequency% of the function entry baseline frequency.
+  BlockFrequency LoopEntryFreq = BFI->getBlockFreq(loopPreheader);
+  if (LoopUnswitchWithBlockFrequency && LoopEntryFreq < ColdEntryFreq)
----------------
reames wrote:
> chenli wrote:
> > I chose to use loop preheader's frequency for coldness comparison. This will ignore the size of the loop because it does not count backedges or iteration numbers. My reasoning is that if the loop was only entered once in the pass 1000 executions, we should be confident that it will never be entered again. Therefore, no matter how many  times the loop iterates, it shouldn't affect our decision. But I could see the other way around. I am open to change it if anyone prefers the other way.
> I feel pretty strongly that making decisions based on the number of times the loop executes rather than the number of times the header executes is likely to produce negative results.  
> 
> Consider:
> while(true) {
>   if (loop invariant) {
>   }
>   if (loop invariant2) {
>   }
>   if (loop invariant3) {
>   }
>   do work();
> }
> 
> Your change would completely disable unswitching in this loop which seems non-ideal.  
The hotness of the loop should be measured by the  frequency of the hottest BB in the loop relative to the function entry.

With PGO, you will need to check the execution count of the hottest BB in the loop (relative to some threshold set by the program summary).


================
Comment at: test/Transforms/LoopUnswitch/cold-loop.ll:21
@@ +20,3 @@
+continue:
+  br i1 %cond3, label %do_something, label %do_something_else  ; non-trivial condition  
+
----------------
reames wrote:
> This is perhaps a bad example.  In this particular case, we could split the loop into two subloops without code growth.  Maybe tweak your example so it's more obvious why we want to avoid unswitching this?
Unswitching duplicates the loop, so size overhead is the loop setup code (in this case probably just a branch associated with backedge).


http://reviews.llvm.org/D11605