[PATCH] D95806: [LoopUnrollAndJam] Do not allows loops which have no exit(ing) blocks or multiple exit(ing) blocks

[Sidharth Baveja via Phabricator via llvm-commits]

sidbav added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:842
+    // Only loops with a single exiting block can be unrolled and jammed.
+    if (!L->getExitingBlock()) {
+      LLVM_DEBUG(dbgs() << "Won't unroll-and-jam; only loops with single "
----------------
Meinersbur wrote:
> It is possible for a single exiting block to have multiple outgoing edges (likely with a switch terminator). Do you intend to allow this/is it checked somewhere else already?
Yes, this should be allowed (assuming such a loop is following all other unroll and jam requirements) as long as there is only 1 edge  going to an exit block. If a loop has multiple exit blocks, this will be caught in the above check.  


================
Comment at: llvm/test/Transforms/LoopUnrollAndJam/multiple_exit_blocks.ll:5
+; CHECK: Loop Unroll and Jam: F[h] Loop %bb8
+; CHECK: Won't unroll-and-jam; Loop has multiple exit blocks OR does not have an exit block
+
----------------
Whitney wrote:
> Whitney wrote:
> > sidbav wrote:
> > > Whitney wrote:
> > > > Can you please add a testcase with single exit block, but multiple exiting blocks?
> > > The reason that I have not added this test case is when the a loop has a single exit block with multiple exiting blocks, is before checking if the loops qualify for unrolling and jamming, all of the loops are simplified. This results the loop have multiple exiting and exit blocks, which is essentially the same test case as this one. 
> > Who is doing the simplification? A loop with a single exit block and multiple exiting blocks can still be considered as in simplified form. 
> Now I remember, we did `simplifyLoop` in `tryToUnrollAndJamLoop`.
> Who is doing the simplification? A loop with a single exit block and multiple exiting blocks can still be considered as in simplified form. 

Yes that is correct. I have come up with a new test case (in simplified form). 
```
define void @h() {                                                                  
bb:                                                                                 
  store i32 4, i32* @e, align 4                                                     
  %i15 = load i16, i16* @b, align 2                                                 
  %i17 = icmp slt i16 %i15, 1                                                       
  br label %bb8                                                                     
                                                                                    
bb8:                                              ; preds = %bb, %bb47              
  %storemerge15 = phi i32 [ 4, %bb ], [ %i49, %bb47 ]                               
  br i1 %i17, label %bb50, label %bb23

bb23:
  br label %bb24                 
                                                                                    
bb24:                                             ; preds = %bb43, %bb23            
  %storemerge312 = phi i16 [ 0, %bb23 ], [ %i45, %bb43 ]                             
  br label %bb43                                                                    
                                                                                    
bb43:                                             ; preds = %bb24                   
  %i45 = add nuw nsw i16 %storemerge312, 1                                          
  %i13 = icmp ult i16 %storemerge312, 7                                             
  br i1 %i13, label %bb24, label %bb47                                              
                                                                                    
bb47:                                             ; preds = %bb43                   
  %i49 = add nsw i32 %storemerge15, -1                                              
  store i32 %i49, i32* @e, align 4                                                  
  %i7.not = icmp eq i32 %i49, 0                                                     
  br i1 %i7.not, label %bb50, label %bb8                                            
                                                                                    
bb50:                                             ; preds = %bb47                   
  ret void                                                                          
}                                                                                   
```
Although  looking at this we can that  `bb50` is the exit block, and both `bb47` and `bb8` the exiting blocks for the loop with header `bb8`, this test case is still considered to have 2 exit blocks. 

Currently the function `getExitBlocks(ExitBlocksVector)` (defined in `llvm/include/llvm/Analysis/LoopInfoImpl.h`), which the function  `getExitBlock()` calls,  does not check if a basicblock is already added to the vector. It allows for basicblocks to repeated. 

In order to consider a test case like this (a single exit block, multiple exiting blocks), we would have to update getExitBlocks(ExitBlocksVector), and any other similar functions, to use a Set. 


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

https://reviews.llvm.org/D95806