[PATCH] D21312: [Polly] Fix assertion due to loop overlap with nonaffine region.

[Michael Kruse via llvm-commits]

Meinersbur added a comment.

Huihui is right, just checking Entry and Exiting blocks is not enough.

The loop is:
Loop at depth 1 containing: %for.cond<header><exiting>,%for.body<exiting>,%if.else,%if.end,%for.inc<latch>

The region is:

  [2] for.body => if.else

Due to how a region is interpreted, %if.then and %while.body belong to the region, even though there is no path from the blocks to the exit node, it just loops infinitely. Loops exclude such dead-end blocks.

Naively, I'd assume regions being 'single entry - single exit' parts that each of the blocks in a region has a path to the exit region. This might be due to the definition of Region::contains
``
return (DT->dominates(entry, BB) && !(DT->dominates(exit, BB) && DT->dominates(entry, exit)));
``
to get around using the postdominator tree. It also means that the definition of Region::contains(Loop *) is wrong(!), ie. this behaviour doesn't seem to be intended. For fixing this, either Regions::contains(Loop*) must be changed or the defintion of what we undesrstand by a region.

For this patch I'd prefer checking all BBs instead of only the loops. Not only infinite loops can be dead-ends, but also Unreachable instructions. These are currently caught by polly::isErrorBlock, but maybe isErrorBlock could/should also be extended to recognize dead-end loops. I'd prefer a single mechanism for such cases instead of multiple in different parts of the code.

Huihui, can you add a descriptive comment about these findings into this patch (that is, why a region is not entirely contained in a loop)? I'd accept it even when only checking boxed loops, assuming that the UnreachableInst case will be caught by isErrorBlock().


================
Comment at: test/ScopDetectionDiagnostics/ReportLoopOverlapWithNonAffineSubRegion.ll:1-2
@@ +1,3 @@
+; RUN: opt -pass-remarks-missed="polly-detect" -polly-allow-nonaffine-loops -polly-process-unprofitable -analyze  -polly-detect < %s 2>&1 | FileCheck %s --check-prefix=REJECTLOOPOVERLAPREGION
+; RUN: opt -pass-remarks-missed="polly-detect" -polly-allow-nonaffine-loops=false -polly-process-unprofitable -analyze  -polly-detect < %s 2>&1 | FileCheck %s --check-prefix=REJECTNONAFFINELOOPS
+
----------------
Please use %loadPolly. It allows unit testing when using `-load LLVMPolly.so`.  -polly-process-unprofitable is automatically added with it.

Ie. 
```
; RUN: opt %loadPolly -pass-remarks-missed="polly-detect" -polly-allow-nonaffine-loops -analyze  -polly-detect < %s 2>&1 | FileCheck %s --check-prefix=REJECTLOOPOVERLAPREGION
; RUN: opt %loadPolly -pass-remarks-missed="polly-detect" -polly-allow-nonaffine-loops=false -analyze  -polly-detect < %s 2>&1 | FileCheck %s --check-prefix=REJECTNONAFFINELOOPS
```

Sorry for not noticing earlier.


Repository:
  rL LLVM

http://reviews.llvm.org/D21312