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

[Sebastian Pop]

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.

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.

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

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