Re: [PATCH][3/3] Fix PR54733 Optimize endian independent load/store

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Apr 30, 2014 at 8:23 AM, Thomas Preud'homme
<thomas.preudhomme@arm.com> wrote:
> Hi Richard,
>
> I addressed all your comments but the ones below.
>
>> From: Richard Biener [mailto:richard.guenther@gmail.com]
>>
>> +                 /* Convert the result of load if necessary.  */
>> +                 if (!useless_type_conversion_p (TREE_TYPE (tgt),
>> +                                                 TREE_TYPE (val_tmp)))
>> +                   {
>>
>> why's that?  Shouldn't the load already be emitted of the correct type?
>
> The size would be correct and I wasn't sure if the sign matters. I've read
> useless_type_conversion_p since and it seems I was overly worried.

Yes, the sign matters - I forgot about that.  Eventually the load type
should be just chosen from the target type though.

>>
>> You seem to replace the stmt computing the target value by directly
>> loading into the target.  IMHO that's premature optimization and it
>> would be easier to just replace its rhs (that way the stmt still has
>> a proper location for example).
>
> Right. I merely followed what was already done and indeed the whole
> statement is replaced. I don't understand why you say it's a premature
> optimization. Since the left hand side is kept, isn't it equivalent (lost of
> location excluded)?

premature optimization is the copy-propagation that is applied, but
yes, as the LHS is kept it is otherwise equivalent.  Though as the
LHS is kept you should use gsi_replace which avoids useless
insertion of debug stmts.

> I'll change to avoid such a replacement.

That's the easiest option of course.

>>
>> And now that I am looking and the patch series again - I think
>> you need to verify that you load the correct value.  Consider
>>
>> int foo (char *x, char *y)
>> {
>>    char c0 = x[0];
>>    char c1 = x[1];
>>    *y = 1;
>>    char c2 = x[2];
>>    char c3 = x[3];
>>    return c0 | c1 << 8 | c2 << 16 | c3 << 24;
>> }
>>
>> where do you insert the single load?  The easiest way out
>> (without doing alias walks and whatnot) is to verify that all
>> loads have the same gimple_vuse () attached (also please
>> set that gimple_vuse () on the load you build - that avoids
>> costly computation of virtual operands).
>
> Nice catch. I'll do that.
>
> Thanks for you answer and don't worry for the delay: I have other things
> to keep me busy and stage1 is not closed yet.

stage1 is going to go for at least another half year ;)

Richard.

> Best regards,
>
> Thomas
>
>