Re: [PATCH] Possible Fix for PR42644 and PR42130

[Sebastian Pop <sebpop at gmail dot com>]

On Thu, Jan 28, 2010 at 10:54, Tobias Grosser <grosser@fim.uni-passau.de> wrote:
> On 01/28/10 15:08, Ramakrishna Upadrasta wrote:
>> Thanks everyone for the feedback.
>>
>> 2010/1/27 Tobias Grosser <grosser@fim.uni-passau.de>:
>>>>> first you shouldn't compare TYPE_SIZE but instead use
>>>>> types_compatible_p ().
>>>>
>>>> The attached patch does that.
>>>
>>> I am not too sure about this. I think we should do something like this
>>>
>>> if (!useless_type_conversion_p(long_long_integer_type_node, type)) {
>>> Âgloog_error = true;
>>> Âreturn error_mark_node;
>>> }
>>>
>>> as types_compatible_p is a symmetric function, but we want to use a type
>>> that has the same or bigger precision as the original one, but is signed.
>>
>> I agree that we should be using the asymmetric function
>> 'useless_type_conversion_p' instead of the symmetric
>> 'types_compatible_p'. Made the change.
>>
>> I do not think that we should return error_mark_node. It is causing
>> errors further down the chain. Since we are setting the gloog_error as
>> true, the checking of which is happening, with non-generation of
>> incorrect code (especially for cases when the type is set to
>> size_type), we are safe.
>>
>> The above is an overly conservative approach and this kind of setting
>> really begs for further type support, but as mentioned earlier, it is
>> a temporary solution.
>>
>> Sebastian: I have changed the second comment to reflect the idea that
>> this needs CLooG support. Yes, the initial wording was imprecise.
>>
>> Attached is the patch.
>
> Hi Ramakrishna,
>
> this one looks good to me. As I also worked with you on this patch, I
> would like to wait for Sebastians approval. However for me it seems to
> be fine to be committed to the graphite branch for testing on the SPEC
> and gcc automatic testers.

The patch is not ok yet: please remove all the spaces at the end of
the line.

>> +2010-01-22  Ramakrishna Upadrasta <Ramakrishna.Upadrasta@inria.fr>,
>> +	Tobias Grosser  <grosser@fim.uni-passau.de>

Please align the beginning of the names.

>> +   TODO: Get the types using CLooG instead. This enables further

Point, space, space, new sentence.

>> +    if (!useless_type_conversion_p(long_long_integer_type_node, type))

After a function name you should put a space before opening a parenthesis.

>> +    {
>> +	/*There is no signed type available, that is large

You need a space between beginning of a comment and the first letter.


>>    if (slot && *slot)
>> -    return ((ivtype_map_elt) *slot)->type;
>> +  {
>> +    type = ((ivtype_map_elt) *slot)->type;
>> +
>> +    if (!useless_type_conversion_p(long_long_integer_type_node, type))
>> +    {
>> +	/*There is no signed type available, that is large
>> +	  enough to hold the original value.  */
>> +
>> +	gloog_error = true;
>> +	return long_long_integer_type_node;
>> +    }

Here you should return the "type", and not fall through and return
long_long_integer_type_node.  This could avoid later problems with
pointer types compared to non pointer long_long_integer_type_node.

>> +  }
>>
>> -  return integer_type_node;
>> +  return long_long_integer_type_node;
>>  }

Have you run the testsuite with at least make -k check
RUNTESTFLAGS=graphite.exp?

>> +      printf ("%d ",  (int) Ke[j]);
>> +      printf("# ");

You are executing this program, and this would print on stdout: please
remove the printfs or #ifdef DEBUG them.

Sebastian