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

[Tobias Grosser]

On 02/12/2013 08:38 PM, Sebastian Pop wrote:
> 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?

Sorry.

We remove the dimension from an isl_space. The isl space is just a 
description of the dimensions that exist in the iteration space. It does
not have any constraints attached to the dimensions. In case the 
dimensions are unnamed, all dimensions are identical. If you remove the 
first or the second, at the end you will have a space with one dimension 
less than the input space.

>> 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]

Yes.

>> 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.

Yes, please test. I can not imagine any problem, but it's good if you 
test it.

Tobi