[polly] r183025 - Test that independent block pass does not transform induction variables

[Tobias Grosser]

On 06/10/2013 07:08 AM, Star Tan wrote:
> Hi Tobias,
>
>
> According to your suggestion, I have fixed this problem by refusing scops, in which a loop header is part of the scop, but the entire loop is not. Please find the attached patch file and revise it.

Thanks!

> 0001-ScopDetect-refuse-regions-that-contain-only-part-of-.patch
>
>
>  From 3e6bbcbcaa997c9d65c7627d240137e665317d3b Mon Sep 17 00:00:00 2001
> From: Star Tan<tanmx_star at yeah.net>
> Date: Mon, 10 Jun 2013 21:14:47 +0800
> Subject: [PATCH] ScopDetect: refuse scops that contain only part of a loop.
>
> With SCEV, we have to refuse to scops, in which a loop header is part of the
> scop, but the entire loop is not. Otherwise it will cause error in some
> testcases, such as:
>    test/IndependentBlocks/indvars.ll
>    test/ScopDetect/indvars_1.ll

Instead of pointing to some test cases, it would be good to explain the 
reason for the error.

The problem is that we can currently can only model induction variables 
that are either fully part of our scop (-> we model them as loop 
induction variable) or induction variables that are scop-invariant (-> 
we model them as parameter). A loop that is only partially part of the
scop causes troubles, as there is no good way to handle the induction
variable in the independent blocks pass.

> ---
>   lib/Analysis/ScopDetection.cpp    | 10 +++++++++
>   test/IndependentBlocks/indvars.ll |  3 +--
>   test/ScopDetect/indvars_1.ll      | 43 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 54 insertions(+), 2 deletions(-)
>   create mode 100644 test/ScopDetect/indvars_1.ll
>
> diff --git a/lib/Analysis/ScopDetection.cpp b/lib/Analysis/ScopDetection.cpp
> index 343ac83..edd07bf 100644
> --- a/lib/Analysis/ScopDetection.cpp
> +++ b/lib/Analysis/ScopDetection.cpp
> @@ -138,6 +138,7 @@ STATISTIC(ValidRegion, "Number of regions that a valid part of Scop");
>
>   BADSCOP_STAT(CFG, "CFG too complex");
>   BADSCOP_STAT(IndVar, "Non canonical induction variable in loop");
> +BADSCOP_STAT(IndLoop, "Region only contains part of the loop");
>   BADSCOP_STAT(LoopBound, "Loop bounds can not be computed");
>   BADSCOP_STAT(FuncCall, "Function call with side effects appeared");
>   BADSCOP_STAT(AffFunc, "Expression not affine");
> @@ -413,6 +414,15 @@ bool ScopDetection::isValidLoop(Loop *L, DetectionContext &Context) const {
>       if (!IndVar)
>         INVALID(IndVar,
>                 "No canonical IV at loop header: " << L->getHeader()->getName());
> +  } else {
> +    // If code generation is in scev based mode, we need to ensure that
> +    // the region cannot contain only part of the loop.

I don't think there is a need to restrict this to the scev based mode.

> +    Region &R = Context.CurRegion;
> +    for (Loop::block_iterator I = L->block_begin(), E = L->block_end();
> +         I != E; ++I)
> +      if (!R.contains(*I))
> +        INVALID(IndLoop, "Region only contains part of the loop:"
> +                << L->getHeader()->getName());
>     }

Going over every basic block is too much. It should be sufficient to 
check that all exiting basic blocks are part of the region. As we 
currently only allow a single exiting block, the check can be simplified
even more.

>     // Is the loop count affine?
> diff --git a/test/IndependentBlocks/indvars.ll b/test/IndependentBlocks/indvars.ll
> index aa096f4..6f506bd 100644
> --- a/test/IndependentBlocks/indvars.ll
> +++ b/test/IndependentBlocks/indvars.ll
> @@ -1,5 +1,4 @@
> -; RUN: opt %loadPolly -polly-independent -polly-codegen-scev %s | FileCheck %s
> -; XFAIL: *
> +; RUN: opt %loadPolly -polly-independent -polly-codegen-scev < %s -S | FileCheck %s
>   ;
>   ; Ensure that the independent block pass does not invalidate the induction
>   ; variable here.

You can probably remove this file entirely. The test case is not 
interesting any more as the problematic SCoP is not even detected.

> diff --git a/test/ScopDetect/indvars_1.ll b/test/ScopDetect/indvars_1.ll
> new file mode 100644
> index 0000000..ead1e4d
> --- /dev/null
> +++ b/test/ScopDetect/indvars_1.ll
> @@ -0,0 +1,43 @@
> +; RUN: opt %loadPolly -polly-codegen-isl -polly-codegen-scev < %s -S | FileCheck %s
> +; RUN: opt %loadPolly -analyze -polly-detect -polly-codegen-isl -polly-codegen-scev < %s | FileCheck -check-prefix=CHECK-SCEV %s
> +;
> +; Ensure that the independent block pass does not invalidate the induction
> +; variable here.

This comment is outdated.

> +target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
> +target triple = "x86_64-unknown-linux-gnu"
> +
> +define void @main(i64* %A) nounwind {
> +entry:
> +  br label %for.i
> +
> +for.i:
> +  %indvar.i = phi i64 [ 0, %entry ], [ %indvar.next.i, %for.i.backedge ]
> +  %indvar.next.i = add i64 %indvar.i, 1
> +  %scevgep = getelementptr i64* %A, i64 %indvar.i
> +  store i64 %indvar.i, i64* %scevgep, align 4
> +  br i1 true, label %for.j.preheader, label %for.j2
> +
> +for.j.preheader:
> +  br label %for.j
> +
> +for.j:
> +  %indvar.j = phi i64 [ %indvar.next.j, %for.j ], [ 0, %for.j.preheader ]
> +  %indvar.next.j = add i64 %indvar.j, 1
> +  %exitcond.j = icmp eq i64 %indvar.next.j, 10
> +  br i1 %exitcond.j, label %for.j2, label %for.j
> +
> +for.j2:
> +  fence seq_cst
> +  br label %for.i.backedge
> +
> +for.i.backedge:
> +  %exitcond.i = icmp eq i64 %indvar.next.i, 2048
> +  br i1 %exitcond.i, label %for.i, label %.end
> +
> +.end:
> +  ret void
> +}
> +
> +; CHECK: %indvar.j = phi i64 [ %indvar.next.j, %for.j ], [ 0, %polly.split_new_and_old ]
> +; CHECK-SCEV: Valid Region for Scop: for.j => for.j2

You should check for the output of -polly-detect (See the other test 
cases on the way).

Tobi