* [Patch] Cleanup widest_int_mode_for_size
@ 2014-09-23 9:17 James Greenhalgh
2014-09-30 8:37 ` James Greenhalgh
2014-10-31 11:00 ` Richard Biener
0 siblings, 2 replies; 4+ messages in thread
From: James Greenhalgh @ 2014-09-23 9:17 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]
Hi,
The comment on widest_int_mode_for_size claims that it returns the
widest integer mode no wider than size. The implementation looks more
like it finds the widest integer mode smaller than size. Everywhere it
is used, the mode it is looking for is ultimately checked against an
expected alignment or is used for heuristics that should be thinking
about that check, so pull it in to here.
Throughout expr.c corrections are made for this fact - adding one to
the size passed to this function. This feels a bit backwards to me.
This patch fixes that, and then fixes the fallout throughout expr.c.
Generally, this means simplifying a bunch of move_by_pieces style copy
code.
Bootstrapped on x86_64, arm and AArch64 with no issues.
OK for trunk?
Thanks,
James
2014-09-23 James Greenhalgh <james.greenhalgh@arm.com>
* expr.c (MOVE_BY_PIECES_P): Remove off-by-one correction to
move_by_pieces_ninsns.
(CLEAR_BY_PIECES_P): Likewise.
(SET_BY_PIECES_P): Likewise.
(STORE_BY_PIECES_P): Likwise.
(widest_int_mode_for_size): Return the widest mode in which the
given size fits.
(move_by_pieces): Remove off-by-one correction for max_size,
simplify copy loop body.
(move_by_pieces_ninsns): Simplify copy body.
(can_store_by_pieces): Remove off-by-one correction for max_size,
simplify copy body.
(store_by_pieces_1): Likewise.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Patch-Cleanup-widest_int_mode_for_size.patch --]
[-- Type: text/x-patch; name=0001-Patch-Cleanup-widest_int_mode_for_size.patch, Size: 8360 bytes --]
diff --git a/gcc/expr.c b/gcc/expr.c
index a6233f3..0af9b9a 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -161,7 +161,7 @@ static void write_complex_part (rtx, rtx, bool);
to perform a structure copy. */
#ifndef MOVE_BY_PIECES_P
#define MOVE_BY_PIECES_P(SIZE, ALIGN) \
- (move_by_pieces_ninsns (SIZE, ALIGN, MOVE_MAX_PIECES + 1) \
+ (move_by_pieces_ninsns (SIZE, ALIGN, MOVE_MAX_PIECES) \
< (unsigned int) MOVE_RATIO (optimize_insn_for_speed_p ()))
#endif
@@ -169,7 +169,7 @@ static void write_complex_part (rtx, rtx, bool);
called to clear storage. */
#ifndef CLEAR_BY_PIECES_P
#define CLEAR_BY_PIECES_P(SIZE, ALIGN) \
- (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES + 1) \
+ (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES) \
< (unsigned int) CLEAR_RATIO (optimize_insn_for_speed_p ()))
#endif
@@ -177,7 +177,7 @@ static void write_complex_part (rtx, rtx, bool);
called to "memset" storage with byte values other than zero. */
#ifndef SET_BY_PIECES_P
#define SET_BY_PIECES_P(SIZE, ALIGN) \
- (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES + 1) \
+ (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES) \
< (unsigned int) SET_RATIO (optimize_insn_for_speed_p ()))
#endif
@@ -185,7 +185,7 @@ static void write_complex_part (rtx, rtx, bool);
called to "memcpy" storage when the source is a constant string. */
#ifndef STORE_BY_PIECES_P
#define STORE_BY_PIECES_P(SIZE, ALIGN) \
- (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES + 1) \
+ (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES) \
< (unsigned int) MOVE_RATIO (optimize_insn_for_speed_p ()))
#endif
\f
@@ -801,18 +801,23 @@ alignment_for_piecewise_move (unsigned int max_pieces, unsigned int align)
return align;
}
-/* Return the widest integer mode no wider than SIZE. If no such mode
- can be found, return VOIDmode. */
+/* Return the widest integer mode no wider than SIZE which can be accessed
+ at the given ALIGNMENT. If no such mode can be found, return VOIDmode.
+ If SIZE would fit exactly in a mode, return that mode. */
static enum machine_mode
-widest_int_mode_for_size (unsigned int size)
+widest_int_mode_for_size_and_alignment (unsigned int size,
+ unsigned int align)
{
enum machine_mode tmode, mode = VOIDmode;
for (tmode = GET_CLASS_NARROWEST_MODE (MODE_INT);
tmode != VOIDmode; tmode = GET_MODE_WIDER_MODE (tmode))
- if (GET_MODE_SIZE (tmode) < size)
- mode = tmode;
+ {
+ if (GET_MODE_SIZE (tmode) <= size
+ && align >= GET_MODE_ALIGNMENT (mode))
+ mode = tmode;
+ }
return mode;
}
@@ -855,7 +860,7 @@ move_by_pieces (rtx to, rtx from, unsigned HOST_WIDE_INT len,
enum machine_mode to_addr_mode;
enum machine_mode from_addr_mode = get_address_mode (from);
rtx to_addr, from_addr = XEXP (from, 0);
- unsigned int max_size = MOVE_MAX_PIECES + 1;
+ unsigned int max_size = MOVE_MAX_PIECES;
enum insn_code icode;
align = MIN (to ? MEM_ALIGN (to) : align, MEM_ALIGN (from));
@@ -907,7 +912,7 @@ move_by_pieces (rtx to, rtx from, unsigned HOST_WIDE_INT len,
MODE might not be used depending on the definitions of the
USE_* macros below. */
enum machine_mode mode ATTRIBUTE_UNUSED
- = widest_int_mode_for_size (max_size);
+ = widest_int_mode_for_size_and_alignment (max_size, align);
if (USE_LOAD_PRE_DECREMENT (mode) && data.reverse && ! data.autinc_from)
{
@@ -948,18 +953,20 @@ move_by_pieces (rtx to, rtx from, unsigned HOST_WIDE_INT len,
/* First move what we can in the largest integer mode, then go to
successively smaller modes. */
- while (max_size > 1 && data.len > 0)
+ while (data.len > 0)
{
- enum machine_mode mode = widest_int_mode_for_size (max_size);
+ enum machine_mode mode
+ = widest_int_mode_for_size_and_alignment (MIN (max_size, data.len),
+ align);
if (mode == VOIDmode)
break;
icode = optab_handler (mov_optab, mode);
- if (icode != CODE_FOR_nothing && align >= GET_MODE_ALIGNMENT (mode))
+ if (icode != CODE_FOR_nothing)
move_by_pieces_1 (GEN_FCN (icode), mode, &data);
-
- max_size = GET_MODE_SIZE (mode);
+ else
+ max_size = GET_MODE_SIZE (mode) - 1;
}
/* The code above should have handled everything. */
@@ -1008,21 +1015,25 @@ move_by_pieces_ninsns (unsigned HOST_WIDE_INT l, unsigned int align,
align = alignment_for_piecewise_move (MOVE_MAX_PIECES, align);
- while (max_size > 1 && l > 0)
+ while (l > 0)
{
enum machine_mode mode;
enum insn_code icode;
- mode = widest_int_mode_for_size (max_size);
+ mode = widest_int_mode_for_size_and_alignment (MIN (max_size, l),
+ align);
if (mode == VOIDmode)
break;
icode = optab_handler (mov_optab, mode);
- if (icode != CODE_FOR_nothing && align >= GET_MODE_ALIGNMENT (mode))
- n_insns += l / GET_MODE_SIZE (mode), l %= GET_MODE_SIZE (mode);
-
- max_size = GET_MODE_SIZE (mode);
+ if (icode != CODE_FOR_nothing)
+ {
+ n_insns += l / GET_MODE_SIZE (mode);
+ l %= GET_MODE_SIZE (mode);
+ }
+ else
+ max_size = GET_MODE_SIZE (mode) - 1;
}
gcc_assert (!l);
@@ -2475,10 +2486,10 @@ can_store_by_pieces (unsigned HOST_WIDE_INT len,
void *constfundata, unsigned int align, bool memsetp)
{
unsigned HOST_WIDE_INT l;
- unsigned int max_size;
HOST_WIDE_INT offset = 0;
enum machine_mode mode;
enum insn_code icode;
+ unsigned int max_size = STORE_MAX_PIECES;
int reverse;
/* cst is set but not used if LEGITIMATE_CONSTANT doesn't use it. */
rtx cst ATTRIBUTE_UNUSED;
@@ -2501,17 +2512,16 @@ can_store_by_pieces (unsigned HOST_WIDE_INT len,
reverse++)
{
l = len;
- max_size = STORE_MAX_PIECES + 1;
- while (max_size > 1 && l > 0)
+ while (l > 0)
{
- mode = widest_int_mode_for_size (max_size);
+ mode = widest_int_mode_for_size_and_alignment
+ (MIN (max_size, l), align);
if (mode == VOIDmode)
break;
icode = optab_handler (mov_optab, mode);
- if (icode != CODE_FOR_nothing
- && align >= GET_MODE_ALIGNMENT (mode))
+ if (icode != CODE_FOR_nothing)
{
unsigned int size = GET_MODE_SIZE (mode);
@@ -2530,8 +2540,8 @@ can_store_by_pieces (unsigned HOST_WIDE_INT len,
l -= size;
}
}
-
- max_size = GET_MODE_SIZE (mode);
+ else
+ max_size = GET_MODE_SIZE (mode) - 1;
}
/* The code above should have handled everything. */
@@ -2643,7 +2653,7 @@ store_by_pieces_1 (struct store_by_pieces_d *data ATTRIBUTE_UNUSED,
{
enum machine_mode to_addr_mode = get_address_mode (data->to);
rtx to_addr = XEXP (data->to, 0);
- unsigned int max_size = STORE_MAX_PIECES + 1;
+ unsigned int max_size = STORE_MAX_PIECES;
enum insn_code icode;
data->offset = 0;
@@ -2668,7 +2678,7 @@ store_by_pieces_1 (struct store_by_pieces_d *data ATTRIBUTE_UNUSED,
MODE might not be used depending on the definitions of the
USE_* macros below. */
enum machine_mode mode ATTRIBUTE_UNUSED
- = widest_int_mode_for_size (max_size);
+ = widest_int_mode_for_size_and_alignment (max_size, align);
if (USE_STORE_PRE_DECREMENT (mode) && data->reverse && ! data->autinc_to)
{
@@ -2697,18 +2707,20 @@ store_by_pieces_1 (struct store_by_pieces_d *data ATTRIBUTE_UNUSED,
/* First store what we can in the largest integer mode, then go to
successively smaller modes. */
- while (max_size > 1 && data->len > 0)
+ while (data->len > 0)
{
- enum machine_mode mode = widest_int_mode_for_size (max_size);
+ enum machine_mode mode
+ = widest_int_mode_for_size_and_alignment (MIN (max_size, data->len),
+ align);
if (mode == VOIDmode)
break;
icode = optab_handler (mov_optab, mode);
- if (icode != CODE_FOR_nothing && align >= GET_MODE_ALIGNMENT (mode))
+ if (icode != CODE_FOR_nothing)
store_by_pieces_2 (GEN_FCN (icode), mode, data);
-
- max_size = GET_MODE_SIZE (mode);
+ else
+ max_size = GET_MODE_SIZE (mode) - 1;
}
/* The code above should have handled everything. */
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch] Cleanup widest_int_mode_for_size
2014-09-23 9:17 [Patch] Cleanup widest_int_mode_for_size James Greenhalgh
@ 2014-09-30 8:37 ` James Greenhalgh
2014-10-27 11:20 ` James Greenhalgh
2014-10-31 11:00 ` Richard Biener
1 sibling, 1 reply; 4+ messages in thread
From: James Greenhalgh @ 2014-09-30 8:37 UTC (permalink / raw)
To: gcc-patches
*ping*
Thanks,
James
On Tue, Sep 23, 2014 at 10:17:21AM +0100, James Greenhalgh wrote:
>
> Hi,
>
> The comment on widest_int_mode_for_size claims that it returns the
> widest integer mode no wider than size. The implementation looks more
> like it finds the widest integer mode smaller than size. Everywhere it
> is used, the mode it is looking for is ultimately checked against an
> expected alignment or is used for heuristics that should be thinking
> about that check, so pull it in to here.
>
> Throughout expr.c corrections are made for this fact - adding one to
> the size passed to this function. This feels a bit backwards to me.
>
> This patch fixes that, and then fixes the fallout throughout expr.c.
> Generally, this means simplifying a bunch of move_by_pieces style copy
> code.
>
> Bootstrapped on x86_64, arm and AArch64 with no issues.
>
> OK for trunk?
>
> Thanks,
> James
>
> 2014-09-23 James Greenhalgh <james.greenhalgh@arm.com>
>
> * expr.c (MOVE_BY_PIECES_P): Remove off-by-one correction to
> move_by_pieces_ninsns.
> (CLEAR_BY_PIECES_P): Likewise.
> (SET_BY_PIECES_P): Likewise.
> (STORE_BY_PIECES_P): Likwise.
> (widest_int_mode_for_size): Return the widest mode in which the
> given size fits.
> (move_by_pieces): Remove off-by-one correction for max_size,
> simplify copy loop body.
> (move_by_pieces_ninsns): Simplify copy body.
> (can_store_by_pieces): Remove off-by-one correction for max_size,
> simplify copy body.
> (store_by_pieces_1): Likewise.
>
> diff --git a/gcc/expr.c b/gcc/expr.c
> index a6233f3..0af9b9a 100644
> --- a/gcc/expr.c
> +++ b/gcc/expr.c
> @@ -161,7 +161,7 @@ static void write_complex_part (rtx, rtx, bool);
> to perform a structure copy. */
> #ifndef MOVE_BY_PIECES_P
> #define MOVE_BY_PIECES_P(SIZE, ALIGN) \
> - (move_by_pieces_ninsns (SIZE, ALIGN, MOVE_MAX_PIECES + 1) \
> + (move_by_pieces_ninsns (SIZE, ALIGN, MOVE_MAX_PIECES) \
> < (unsigned int) MOVE_RATIO (optimize_insn_for_speed_p ()))
> #endif
>
> @@ -169,7 +169,7 @@ static void write_complex_part (rtx, rtx, bool);
> called to clear storage. */
> #ifndef CLEAR_BY_PIECES_P
> #define CLEAR_BY_PIECES_P(SIZE, ALIGN) \
> - (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES + 1) \
> + (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES) \
> < (unsigned int) CLEAR_RATIO (optimize_insn_for_speed_p ()))
> #endif
>
> @@ -177,7 +177,7 @@ static void write_complex_part (rtx, rtx, bool);
> called to "memset" storage with byte values other than zero. */
> #ifndef SET_BY_PIECES_P
> #define SET_BY_PIECES_P(SIZE, ALIGN) \
> - (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES + 1) \
> + (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES) \
> < (unsigned int) SET_RATIO (optimize_insn_for_speed_p ()))
> #endif
>
> @@ -185,7 +185,7 @@ static void write_complex_part (rtx, rtx, bool);
> called to "memcpy" storage when the source is a constant string. */
> #ifndef STORE_BY_PIECES_P
> #define STORE_BY_PIECES_P(SIZE, ALIGN) \
> - (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES + 1) \
> + (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES) \
> < (unsigned int) MOVE_RATIO (optimize_insn_for_speed_p ()))
> #endif
> \f
> @@ -801,18 +801,23 @@ alignment_for_piecewise_move (unsigned int max_pieces, unsigned int align)
> return align;
> }
>
> -/* Return the widest integer mode no wider than SIZE. If no such mode
> - can be found, return VOIDmode. */
> +/* Return the widest integer mode no wider than SIZE which can be accessed
> + at the given ALIGNMENT. If no such mode can be found, return VOIDmode.
> + If SIZE would fit exactly in a mode, return that mode. */
>
> static enum machine_mode
> -widest_int_mode_for_size (unsigned int size)
> +widest_int_mode_for_size_and_alignment (unsigned int size,
> + unsigned int align)
> {
> enum machine_mode tmode, mode = VOIDmode;
>
> for (tmode = GET_CLASS_NARROWEST_MODE (MODE_INT);
> tmode != VOIDmode; tmode = GET_MODE_WIDER_MODE (tmode))
> - if (GET_MODE_SIZE (tmode) < size)
> - mode = tmode;
> + {
> + if (GET_MODE_SIZE (tmode) <= size
> + && align >= GET_MODE_ALIGNMENT (mode))
> + mode = tmode;
> + }
>
> return mode;
> }
> @@ -855,7 +860,7 @@ move_by_pieces (rtx to, rtx from, unsigned HOST_WIDE_INT len,
> enum machine_mode to_addr_mode;
> enum machine_mode from_addr_mode = get_address_mode (from);
> rtx to_addr, from_addr = XEXP (from, 0);
> - unsigned int max_size = MOVE_MAX_PIECES + 1;
> + unsigned int max_size = MOVE_MAX_PIECES;
> enum insn_code icode;
>
> align = MIN (to ? MEM_ALIGN (to) : align, MEM_ALIGN (from));
> @@ -907,7 +912,7 @@ move_by_pieces (rtx to, rtx from, unsigned HOST_WIDE_INT len,
> MODE might not be used depending on the definitions of the
> USE_* macros below. */
> enum machine_mode mode ATTRIBUTE_UNUSED
> - = widest_int_mode_for_size (max_size);
> + = widest_int_mode_for_size_and_alignment (max_size, align);
>
> if (USE_LOAD_PRE_DECREMENT (mode) && data.reverse && ! data.autinc_from)
> {
> @@ -948,18 +953,20 @@ move_by_pieces (rtx to, rtx from, unsigned HOST_WIDE_INT len,
> /* First move what we can in the largest integer mode, then go to
> successively smaller modes. */
>
> - while (max_size > 1 && data.len > 0)
> + while (data.len > 0)
> {
> - enum machine_mode mode = widest_int_mode_for_size (max_size);
> + enum machine_mode mode
> + = widest_int_mode_for_size_and_alignment (MIN (max_size, data.len),
> + align);
>
> if (mode == VOIDmode)
> break;
>
> icode = optab_handler (mov_optab, mode);
> - if (icode != CODE_FOR_nothing && align >= GET_MODE_ALIGNMENT (mode))
> + if (icode != CODE_FOR_nothing)
> move_by_pieces_1 (GEN_FCN (icode), mode, &data);
> -
> - max_size = GET_MODE_SIZE (mode);
> + else
> + max_size = GET_MODE_SIZE (mode) - 1;
> }
>
> /* The code above should have handled everything. */
> @@ -1008,21 +1015,25 @@ move_by_pieces_ninsns (unsigned HOST_WIDE_INT l, unsigned int align,
>
> align = alignment_for_piecewise_move (MOVE_MAX_PIECES, align);
>
> - while (max_size > 1 && l > 0)
> + while (l > 0)
> {
> enum machine_mode mode;
> enum insn_code icode;
>
> - mode = widest_int_mode_for_size (max_size);
> + mode = widest_int_mode_for_size_and_alignment (MIN (max_size, l),
> + align);
>
> if (mode == VOIDmode)
> break;
>
> icode = optab_handler (mov_optab, mode);
> - if (icode != CODE_FOR_nothing && align >= GET_MODE_ALIGNMENT (mode))
> - n_insns += l / GET_MODE_SIZE (mode), l %= GET_MODE_SIZE (mode);
> -
> - max_size = GET_MODE_SIZE (mode);
> + if (icode != CODE_FOR_nothing)
> + {
> + n_insns += l / GET_MODE_SIZE (mode);
> + l %= GET_MODE_SIZE (mode);
> + }
> + else
> + max_size = GET_MODE_SIZE (mode) - 1;
> }
>
> gcc_assert (!l);
> @@ -2475,10 +2486,10 @@ can_store_by_pieces (unsigned HOST_WIDE_INT len,
> void *constfundata, unsigned int align, bool memsetp)
> {
> unsigned HOST_WIDE_INT l;
> - unsigned int max_size;
> HOST_WIDE_INT offset = 0;
> enum machine_mode mode;
> enum insn_code icode;
> + unsigned int max_size = STORE_MAX_PIECES;
> int reverse;
> /* cst is set but not used if LEGITIMATE_CONSTANT doesn't use it. */
> rtx cst ATTRIBUTE_UNUSED;
> @@ -2501,17 +2512,16 @@ can_store_by_pieces (unsigned HOST_WIDE_INT len,
> reverse++)
> {
> l = len;
> - max_size = STORE_MAX_PIECES + 1;
> - while (max_size > 1 && l > 0)
> + while (l > 0)
> {
> - mode = widest_int_mode_for_size (max_size);
> + mode = widest_int_mode_for_size_and_alignment
> + (MIN (max_size, l), align);
>
> if (mode == VOIDmode)
> break;
>
> icode = optab_handler (mov_optab, mode);
> - if (icode != CODE_FOR_nothing
> - && align >= GET_MODE_ALIGNMENT (mode))
> + if (icode != CODE_FOR_nothing)
> {
> unsigned int size = GET_MODE_SIZE (mode);
>
> @@ -2530,8 +2540,8 @@ can_store_by_pieces (unsigned HOST_WIDE_INT len,
> l -= size;
> }
> }
> -
> - max_size = GET_MODE_SIZE (mode);
> + else
> + max_size = GET_MODE_SIZE (mode) - 1;
> }
>
> /* The code above should have handled everything. */
> @@ -2643,7 +2653,7 @@ store_by_pieces_1 (struct store_by_pieces_d *data ATTRIBUTE_UNUSED,
> {
> enum machine_mode to_addr_mode = get_address_mode (data->to);
> rtx to_addr = XEXP (data->to, 0);
> - unsigned int max_size = STORE_MAX_PIECES + 1;
> + unsigned int max_size = STORE_MAX_PIECES;
> enum insn_code icode;
>
> data->offset = 0;
> @@ -2668,7 +2678,7 @@ store_by_pieces_1 (struct store_by_pieces_d *data ATTRIBUTE_UNUSED,
> MODE might not be used depending on the definitions of the
> USE_* macros below. */
> enum machine_mode mode ATTRIBUTE_UNUSED
> - = widest_int_mode_for_size (max_size);
> + = widest_int_mode_for_size_and_alignment (max_size, align);
>
> if (USE_STORE_PRE_DECREMENT (mode) && data->reverse && ! data->autinc_to)
> {
> @@ -2697,18 +2707,20 @@ store_by_pieces_1 (struct store_by_pieces_d *data ATTRIBUTE_UNUSED,
> /* First store what we can in the largest integer mode, then go to
> successively smaller modes. */
>
> - while (max_size > 1 && data->len > 0)
> + while (data->len > 0)
> {
> - enum machine_mode mode = widest_int_mode_for_size (max_size);
> + enum machine_mode mode
> + = widest_int_mode_for_size_and_alignment (MIN (max_size, data->len),
> + align);
>
> if (mode == VOIDmode)
> break;
>
> icode = optab_handler (mov_optab, mode);
> - if (icode != CODE_FOR_nothing && align >= GET_MODE_ALIGNMENT (mode))
> + if (icode != CODE_FOR_nothing)
> store_by_pieces_2 (GEN_FCN (icode), mode, data);
> -
> - max_size = GET_MODE_SIZE (mode);
> + else
> + max_size = GET_MODE_SIZE (mode) - 1;
> }
>
> /* The code above should have handled everything. */
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch] Cleanup widest_int_mode_for_size
2014-09-30 8:37 ` James Greenhalgh
@ 2014-10-27 11:20 ` James Greenhalgh
0 siblings, 0 replies; 4+ messages in thread
From: James Greenhalgh @ 2014-10-27 11:20 UTC (permalink / raw)
To: gcc-patches; +Cc: law, rguenther
On Tue, Sep 30, 2014 at 09:37:12AM +0100, James Greenhalgh wrote:
> *ping*
*pingx2*
Cheers,
James
>
> Thanks,
> James
>
> On Tue, Sep 23, 2014 at 10:17:21AM +0100, James Greenhalgh wrote:
> >
> > Hi,
> >
> > The comment on widest_int_mode_for_size claims that it returns the
> > widest integer mode no wider than size. The implementation looks more
> > like it finds the widest integer mode smaller than size. Everywhere it
> > is used, the mode it is looking for is ultimately checked against an
> > expected alignment or is used for heuristics that should be thinking
> > about that check, so pull it in to here.
> >
> > Throughout expr.c corrections are made for this fact - adding one to
> > the size passed to this function. This feels a bit backwards to me.
> >
> > This patch fixes that, and then fixes the fallout throughout expr.c.
> > Generally, this means simplifying a bunch of move_by_pieces style copy
> > code.
> >
> > Bootstrapped on x86_64, arm and AArch64 with no issues.
> >
> > OK for trunk?
> >
> > Thanks,
> > James
> >
> > 2014-09-23 James Greenhalgh <james.greenhalgh@arm.com>
> >
> > * expr.c (MOVE_BY_PIECES_P): Remove off-by-one correction to
> > move_by_pieces_ninsns.
> > (CLEAR_BY_PIECES_P): Likewise.
> > (SET_BY_PIECES_P): Likewise.
> > (STORE_BY_PIECES_P): Likwise.
> > (widest_int_mode_for_size): Return the widest mode in which the
> > given size fits.
> > (move_by_pieces): Remove off-by-one correction for max_size,
> > simplify copy loop body.
> > (move_by_pieces_ninsns): Simplify copy body.
> > (can_store_by_pieces): Remove off-by-one correction for max_size,
> > simplify copy body.
> > (store_by_pieces_1): Likewise.
> >
>
> > diff --git a/gcc/expr.c b/gcc/expr.c
> > index a6233f3..0af9b9a 100644
> > --- a/gcc/expr.c
> > +++ b/gcc/expr.c
> > @@ -161,7 +161,7 @@ static void write_complex_part (rtx, rtx, bool);
> > to perform a structure copy. */
> > #ifndef MOVE_BY_PIECES_P
> > #define MOVE_BY_PIECES_P(SIZE, ALIGN) \
> > - (move_by_pieces_ninsns (SIZE, ALIGN, MOVE_MAX_PIECES + 1) \
> > + (move_by_pieces_ninsns (SIZE, ALIGN, MOVE_MAX_PIECES) \
> > < (unsigned int) MOVE_RATIO (optimize_insn_for_speed_p ()))
> > #endif
> >
> > @@ -169,7 +169,7 @@ static void write_complex_part (rtx, rtx, bool);
> > called to clear storage. */
> > #ifndef CLEAR_BY_PIECES_P
> > #define CLEAR_BY_PIECES_P(SIZE, ALIGN) \
> > - (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES + 1) \
> > + (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES) \
> > < (unsigned int) CLEAR_RATIO (optimize_insn_for_speed_p ()))
> > #endif
> >
> > @@ -177,7 +177,7 @@ static void write_complex_part (rtx, rtx, bool);
> > called to "memset" storage with byte values other than zero. */
> > #ifndef SET_BY_PIECES_P
> > #define SET_BY_PIECES_P(SIZE, ALIGN) \
> > - (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES + 1) \
> > + (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES) \
> > < (unsigned int) SET_RATIO (optimize_insn_for_speed_p ()))
> > #endif
> >
> > @@ -185,7 +185,7 @@ static void write_complex_part (rtx, rtx, bool);
> > called to "memcpy" storage when the source is a constant string. */
> > #ifndef STORE_BY_PIECES_P
> > #define STORE_BY_PIECES_P(SIZE, ALIGN) \
> > - (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES + 1) \
> > + (move_by_pieces_ninsns (SIZE, ALIGN, STORE_MAX_PIECES) \
> > < (unsigned int) MOVE_RATIO (optimize_insn_for_speed_p ()))
> > #endif
> >
> > @@ -801,18 +801,23 @@ alignment_for_piecewise_move (unsigned int max_pieces, unsigned int align)
> > return align;
> > }
> >
> > -/* Return the widest integer mode no wider than SIZE. If no such mode
> > - can be found, return VOIDmode. */
> > +/* Return the widest integer mode no wider than SIZE which can be accessed
> > + at the given ALIGNMENT. If no such mode can be found, return VOIDmode.
> > + If SIZE would fit exactly in a mode, return that mode. */
> >
> > static enum machine_mode
> > -widest_int_mode_for_size (unsigned int size)
> > +widest_int_mode_for_size_and_alignment (unsigned int size,
> > + unsigned int align)
> > {
> > enum machine_mode tmode, mode = VOIDmode;
> >
> > for (tmode = GET_CLASS_NARROWEST_MODE (MODE_INT);
> > tmode != VOIDmode; tmode = GET_MODE_WIDER_MODE (tmode))
> > - if (GET_MODE_SIZE (tmode) < size)
> > - mode = tmode;
> > + {
> > + if (GET_MODE_SIZE (tmode) <= size
> > + && align >= GET_MODE_ALIGNMENT (mode))
> > + mode = tmode;
> > + }
> >
> > return mode;
> > }
> > @@ -855,7 +860,7 @@ move_by_pieces (rtx to, rtx from, unsigned HOST_WIDE_INT len,
> > enum machine_mode to_addr_mode;
> > enum machine_mode from_addr_mode = get_address_mode (from);
> > rtx to_addr, from_addr = XEXP (from, 0);
> > - unsigned int max_size = MOVE_MAX_PIECES + 1;
> > + unsigned int max_size = MOVE_MAX_PIECES;
> > enum insn_code icode;
> >
> > align = MIN (to ? MEM_ALIGN (to) : align, MEM_ALIGN (from));
> > @@ -907,7 +912,7 @@ move_by_pieces (rtx to, rtx from, unsigned HOST_WIDE_INT len,
> > MODE might not be used depending on the definitions of the
> > USE_* macros below. */
> > enum machine_mode mode ATTRIBUTE_UNUSED
> > - = widest_int_mode_for_size (max_size);
> > + = widest_int_mode_for_size_and_alignment (max_size, align);
> >
> > if (USE_LOAD_PRE_DECREMENT (mode) && data.reverse && ! data.autinc_from)
> > {
> > @@ -948,18 +953,20 @@ move_by_pieces (rtx to, rtx from, unsigned HOST_WIDE_INT len,
> > /* First move what we can in the largest integer mode, then go to
> > successively smaller modes. */
> >
> > - while (max_size > 1 && data.len > 0)
> > + while (data.len > 0)
> > {
> > - enum machine_mode mode = widest_int_mode_for_size (max_size);
> > + enum machine_mode mode
> > + = widest_int_mode_for_size_and_alignment (MIN (max_size, data.len),
> > + align);
> >
> > if (mode == VOIDmode)
> > break;
> >
> > icode = optab_handler (mov_optab, mode);
> > - if (icode != CODE_FOR_nothing && align >= GET_MODE_ALIGNMENT (mode))
> > + if (icode != CODE_FOR_nothing)
> > move_by_pieces_1 (GEN_FCN (icode), mode, &data);
> > -
> > - max_size = GET_MODE_SIZE (mode);
> > + else
> > + max_size = GET_MODE_SIZE (mode) - 1;
> > }
> >
> > /* The code above should have handled everything. */
> > @@ -1008,21 +1015,25 @@ move_by_pieces_ninsns (unsigned HOST_WIDE_INT l, unsigned int align,
> >
> > align = alignment_for_piecewise_move (MOVE_MAX_PIECES, align);
> >
> > - while (max_size > 1 && l > 0)
> > + while (l > 0)
> > {
> > enum machine_mode mode;
> > enum insn_code icode;
> >
> > - mode = widest_int_mode_for_size (max_size);
> > + mode = widest_int_mode_for_size_and_alignment (MIN (max_size, l),
> > + align);
> >
> > if (mode == VOIDmode)
> > break;
> >
> > icode = optab_handler (mov_optab, mode);
> > - if (icode != CODE_FOR_nothing && align >= GET_MODE_ALIGNMENT (mode))
> > - n_insns += l / GET_MODE_SIZE (mode), l %= GET_MODE_SIZE (mode);
> > -
> > - max_size = GET_MODE_SIZE (mode);
> > + if (icode != CODE_FOR_nothing)
> > + {
> > + n_insns += l / GET_MODE_SIZE (mode);
> > + l %= GET_MODE_SIZE (mode);
> > + }
> > + else
> > + max_size = GET_MODE_SIZE (mode) - 1;
> > }
> >
> > gcc_assert (!l);
> > @@ -2475,10 +2486,10 @@ can_store_by_pieces (unsigned HOST_WIDE_INT len,
> > void *constfundata, unsigned int align, bool memsetp)
> > {
> > unsigned HOST_WIDE_INT l;
> > - unsigned int max_size;
> > HOST_WIDE_INT offset = 0;
> > enum machine_mode mode;
> > enum insn_code icode;
> > + unsigned int max_size = STORE_MAX_PIECES;
> > int reverse;
> > /* cst is set but not used if LEGITIMATE_CONSTANT doesn't use it. */
> > rtx cst ATTRIBUTE_UNUSED;
> > @@ -2501,17 +2512,16 @@ can_store_by_pieces (unsigned HOST_WIDE_INT len,
> > reverse++)
> > {
> > l = len;
> > - max_size = STORE_MAX_PIECES + 1;
> > - while (max_size > 1 && l > 0)
> > + while (l > 0)
> > {
> > - mode = widest_int_mode_for_size (max_size);
> > + mode = widest_int_mode_for_size_and_alignment
> > + (MIN (max_size, l), align);
> >
> > if (mode == VOIDmode)
> > break;
> >
> > icode = optab_handler (mov_optab, mode);
> > - if (icode != CODE_FOR_nothing
> > - && align >= GET_MODE_ALIGNMENT (mode))
> > + if (icode != CODE_FOR_nothing)
> > {
> > unsigned int size = GET_MODE_SIZE (mode);
> >
> > @@ -2530,8 +2540,8 @@ can_store_by_pieces (unsigned HOST_WIDE_INT len,
> > l -= size;
> > }
> > }
> > -
> > - max_size = GET_MODE_SIZE (mode);
> > + else
> > + max_size = GET_MODE_SIZE (mode) - 1;
> > }
> >
> > /* The code above should have handled everything. */
> > @@ -2643,7 +2653,7 @@ store_by_pieces_1 (struct store_by_pieces_d *data ATTRIBUTE_UNUSED,
> > {
> > enum machine_mode to_addr_mode = get_address_mode (data->to);
> > rtx to_addr = XEXP (data->to, 0);
> > - unsigned int max_size = STORE_MAX_PIECES + 1;
> > + unsigned int max_size = STORE_MAX_PIECES;
> > enum insn_code icode;
> >
> > data->offset = 0;
> > @@ -2668,7 +2678,7 @@ store_by_pieces_1 (struct store_by_pieces_d *data ATTRIBUTE_UNUSED,
> > MODE might not be used depending on the definitions of the
> > USE_* macros below. */
> > enum machine_mode mode ATTRIBUTE_UNUSED
> > - = widest_int_mode_for_size (max_size);
> > + = widest_int_mode_for_size_and_alignment (max_size, align);
> >
> > if (USE_STORE_PRE_DECREMENT (mode) && data->reverse && ! data->autinc_to)
> > {
> > @@ -2697,18 +2707,20 @@ store_by_pieces_1 (struct store_by_pieces_d *data ATTRIBUTE_UNUSED,
> > /* First store what we can in the largest integer mode, then go to
> > successively smaller modes. */
> >
> > - while (max_size > 1 && data->len > 0)
> > + while (data->len > 0)
> > {
> > - enum machine_mode mode = widest_int_mode_for_size (max_size);
> > + enum machine_mode mode
> > + = widest_int_mode_for_size_and_alignment (MIN (max_size, data->len),
> > + align);
> >
> > if (mode == VOIDmode)
> > break;
> >
> > icode = optab_handler (mov_optab, mode);
> > - if (icode != CODE_FOR_nothing && align >= GET_MODE_ALIGNMENT (mode))
> > + if (icode != CODE_FOR_nothing)
> > store_by_pieces_2 (GEN_FCN (icode), mode, data);
> > -
> > - max_size = GET_MODE_SIZE (mode);
> > + else
> > + max_size = GET_MODE_SIZE (mode) - 1;
> > }
> >
> > /* The code above should have handled everything. */
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch] Cleanup widest_int_mode_for_size
2014-09-23 9:17 [Patch] Cleanup widest_int_mode_for_size James Greenhalgh
2014-09-30 8:37 ` James Greenhalgh
@ 2014-10-31 11:00 ` Richard Biener
1 sibling, 0 replies; 4+ messages in thread
From: Richard Biener @ 2014-10-31 11:00 UTC (permalink / raw)
To: James Greenhalgh; +Cc: GCC Patches
On Tue, Sep 23, 2014 at 11:17 AM, James Greenhalgh
<james.greenhalgh@arm.com> wrote:
>
> Hi,
>
> The comment on widest_int_mode_for_size claims that it returns the
> widest integer mode no wider than size. The implementation looks more
> like it finds the widest integer mode smaller than size. Everywhere it
> is used, the mode it is looking for is ultimately checked against an
> expected alignment or is used for heuristics that should be thinking
> about that check, so pull it in to here.
>
> Throughout expr.c corrections are made for this fact - adding one to
> the size passed to this function. This feels a bit backwards to me.
>
> This patch fixes that, and then fixes the fallout throughout expr.c.
> Generally, this means simplifying a bunch of move_by_pieces style copy
> code.
>
> Bootstrapped on x86_64, arm and AArch64 with no issues.
I suppose you verified the generated code is the same?
Btw, this all looks more like this wants to be an iterator-style approach,
iterating over modes from larger to narrower that honor the alignment.
Repeatedly iterating over all modes in the loops iterating towards
smaller modes looks backward. Iterating by asking for size - 1
in the next round isn't much clearer than asking for size + 1 either.
> OK for trunk?
So I'm not sure this is a real improvement. Instead changing the loops
to explicitely iterate via GET_MODE_NARROWER_MODE (oops - we don't
have that, but it should be easy to add) would be better.
Thanks,
Richard.
> Thanks,
> James
>
> 2014-09-23 James Greenhalgh <james.greenhalgh@arm.com>
>
> * expr.c (MOVE_BY_PIECES_P): Remove off-by-one correction to
> move_by_pieces_ninsns.
> (CLEAR_BY_PIECES_P): Likewise.
> (SET_BY_PIECES_P): Likewise.
> (STORE_BY_PIECES_P): Likwise.
> (widest_int_mode_for_size): Return the widest mode in which the
> given size fits.
> (move_by_pieces): Remove off-by-one correction for max_size,
> simplify copy loop body.
> (move_by_pieces_ninsns): Simplify copy body.
> (can_store_by_pieces): Remove off-by-one correction for max_size,
> simplify copy body.
> (store_by_pieces_1): Likewise.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-10-31 10:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-23 9:17 [Patch] Cleanup widest_int_mode_for_size James Greenhalgh
2014-09-30 8:37 ` James Greenhalgh
2014-10-27 11:20 ` James Greenhalgh
2014-10-31 11:00 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).