[polly] block generators: fix insertion of values in BBMap

[Tobias Grosser]

On 05/03/2013 07:22 PM, Sebastian Pop wrote:
> Tobias Grosser wrote:
>> On 05/02/2013 11:19 PM, Sebastian Pop wrote:
>>> Hi,
>>>
>>> When BBMap contains a new value for an old value, i.e., BBMap.count(Old) > 0,
>>> BBMap[Old] should not be NULL.  The attached patch adds an assert to verify this
>>> property.
>>
>> That is OK.
>>
>>> The patch also transforms the insertions into BBMap to avoid storing NULL
>>> pointers in the map.  I do not know why the existing code stores NULL pointers:
>>>
>>>    BBMap[Load] = generateScalarLoad(...);
>>>
>>> whereas storing the value into a temporary before inserting in BBMap does not:
>>>
>>>    Value *NewLoad = generateScalarLoad(...);
>>>    BBMap[Load] = NewLoad;
>>>
>>> Looks like a bug due to the order of evaluation of the arguments of a function.
>>
>> This is very surprising. I don't really understand how this fixes
>> the problem. Can you provide a reduced test case?
>
> Nop, I cannot provide a reduced testcase: the most reduced one that I remember
> having seen spans over several screens of bytecode.  It occurs in one of the
> benchmarks in the test-suite with -polly-codegen-scev=true.

Can you send me this one?

> I don't understand why you are insisting in having a reduced testcase for every
> bug: you just can't have a testcase in some circumstances... you have the
> test-suite to catch such cases, and with -polly-codegen-scev=true it is far from
> being clean.

I understand. If it is not possible to find one that is fine, but I 
would like to dig a little in what is going on here.

>>> Ok to commit?
>>
>> I would first like to understand the problem. I do not yet see the
>> reason why the transformation you propose should avoid the
>> generation of NULL pointers. I also don't see where those pointers
>> would come from in the first place.
>>
>> Can you point out what exactly is the bug caused by the order of
>> evaluation of arguments?
>
> As I said, I do not know why this happens: in GDB when "step" through
> generateScalarLoad and "finish" the call, the returned value is non NULL,
> however when printing the value contained in BBMap[Load] after this stmt:
>
>   BBMap[Load] = generateScalarLoad(...);
>
> the value in BBMap[Load] is NULL, and the BBMap.count(Load) is 1.
>
> The only intuitive idea that I have to explain this behavior is that we are
> playing with the undefined behavior of eval order of the params for the function
> standing for "BBMap[Load] = generateScalarLoad()". "BBMap[Load] = " may be
> executed before generateScalarLoad is called.

This sounds very surprising. Even if the parameters are evaluated in 
random order, the actual assignment should be performed only after all 
of them have been evaluated.

I would like to investigate this a little more to really understand the 
cause of this problem. I am afraid we may be hiding undefined behavior 
that was introduced by another bug. As we now at least have a test case 
(even though it is difficult to reduce), I would like to take the chance 
to really understand this.

Cheers,
Tobias