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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur 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 "
----------------
sidbav wrote:
> 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.  
The "there is only 1 edge going to an exit block" is exactly what I was wondering. Consider:
```
header:
  br label %exiting

exiting:
   switch i32 %a, label %header [
      i32 0, label %exit
      i32 1, label %exit
   ]

exit:
  ret void
```

This loop has one exiting block, one exit block. but two exiting edges. My question is whether this is either handled correctly by UnrollAndJam or if there is something else checking for this.

`L->getExitingBlock()` does return `%exiting` in this case, but it looks like `L->getExitBlock()` returns nullptr for this case (`getUniqueExitBlock()` would return `%exit`), so seems I answered my own question.


================
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
+
----------------
sidbav wrote:
> 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. 
Did you have a look at `getUniqueExitBlocks()`?

As far as I understand, multiple exiting edges are not supported at all, even if pointing to the same exit block, so the use if `!getExitBlock()` seems correct.


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

https://reviews.llvm.org/D95806