Re: PATCH: fine-tuning for can_store_by_pieces

[Mark Mitchell <mark@codesourcery.com>]

Andrew Pinski wrote:
> On 8/15/07, Sandra Loosemore <sandra@codesourcery.com> wrote:
>> On MIPS we can get better code by having can_store_by_pieces
>> differentiate between the cases where it's used for memset
>> operations, and those where it's used to copy a string constant.
>> This patch introduces new SET_RATIO and SET_BY_PIECES_P macros,
>> with appropriate defaults to preserve the existing behavior.  I
>> checked other targets and made the ones that override the default 
>> STORE_BY_PIECES_P clone the same definition for SET_BY_PIECES_P.

> It seems if you gave a testcase where this is profitable, it would be
>  better to judge this patch (and maybe a testcase for the testsuite 
> also).

Sandra's now posted such a testcase, and it looks compelling to me.
Certainly, setting all bytes to a single value is different from copying
one series of bytes to another, so it doesn't surprise me that one wants
different rules for the different cases.

Sandra, I think the generic parts of the patch are OK, with a few nits:

> ! 		     void *constfundata, unsigned int align, int memsetp)

MEMSETP should be a bool, not an int.  And, when literal values are
passed to it, they should be "true" and "false", not "1" and "0".

> * config/sh/sh.h (SET_BY_PIECES_P): Clone from STORE_BY_PIECES_P.

In the various places where you clone the macro, I think you should just
 do:

#define SET_BY_PIECES_P(SIZE, ALIGN) STORE_BY_PIECES_P(SIZE, ALIGN)

In fact, if you do that in expr.c, under #ifndef SET_BY_PIECES_P, I
think you can void changing the other backends at all, as you'll
automatically pick up their STORE_BY_PIECES_P definitions.

If that all works, then the target-independent parts of the patch are OK
with those changes.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713