On Wed, Apr 30, 2014 at 8:23 AM, Thomas Preud'homme
<thomas.preudho...@arm.com> wrote:
> Hi Richard,
>
> I addressed all your comments but the ones below.
>
>> From: Richard Biener [mailto:richard.guent...@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
>
>
