[polly] r174894 - [isl-codegen]: Fix off by one in getNumberOfIterations

[Sebastian Pop]

Tobias Grosser wrote:
> Author: grosser
> Date: Mon Feb 11 11:52:36 2013
> New Revision: 174894
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=174894&view=rev
> Log:
> [isl-codegen]: Fix off by one in getNumberOfIterations
> 
> We need to remove one dimension. Any is correct as long as it exists. 

Can you explain why dropping any dimension is correct?

> We have
> choosen for whatever reason the dimension #dims - 2. This is incorrect if
> there is just one dimension. For CLooG this case did never happen. For isl
> however, the case can happen and causes undefined behavior including crashes.
> We choose now always the last dimension #dims - 1. We could have choosen
> dimension '0' but the last dimension is what we remove conceptionally in the
> algorithm, so it seems better to actually program it that way.

I also thought that dropping the last dimension was the correct thing to do:
this is also what one of the comments says

    // Calculate a map similar to the identity map, but with the last input
    // and output dimension not related.
    //  [i0, i1, i2, i3] -> [i0, i1, i2, o0]

> 
> While at it remove another piece of undefined behavior.
> 
> Added:
>     polly/trunk/test/Isl/CodeGen/20130211-getNumberOfIterations.ll
> Modified:
>     polly/trunk/include/polly/CodeGen/CodeGeneration.h
> 
> Modified: polly/trunk/include/polly/CodeGen/CodeGeneration.h
> URL: http://llvm.org/viewvc/llvm-project/polly/trunk/include/polly/CodeGen/CodeGeneration.h?rev=174894&r1=174893&r2=174894&view=diff
> ==============================================================================
> --- polly/trunk/include/polly/CodeGen/CodeGeneration.h (original)
> +++ polly/trunk/include/polly/CodeGen/CodeGeneration.h Mon Feb 11 11:52:36 2013
> @@ -34,13 +34,15 @@ namespace polly {
>      // and output dimension not related.
>      //  [i0, i1, i2, i3] -> [i0, i1, i2, o0]
>      isl_space *Space = isl_set_get_space(Domain);
> -    Space = isl_space_drop_outputs(Space, Dim - 2, 1);
> +    Space = isl_space_drop_outputs(Space, Dim - 1, 1);

I remember that I complained about this sometime in December when I was testing
with ISL.  Here is how I fixed it:

Space = isl_space_drop_outputs(Space, Dim == 1 ? 0 : Dim - 2, 1);

I remember that I also tried your solution:

Space = isl_space_drop_outputs(Space, Dim - 1, 1);

and I had some problems when testing.  I will now test your patch again and will
tell you if that works.

Thanks,
Sebastian
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation