Message ID | 20181125121912.GO21617@bubble.grove.modra.org |
---|---|
State | New |
Headers | show |
Series | [RS6000] num_insns_constant ICE | expand |
Hi! On Sun, Nov 25, 2018 at 10:49:12PM +1030, Alan Modra wrote: > This patch came about from investigating an ICE that appeared when I > was retesting an old half-baked patch of mine to rs6000_rtx_costs. > If a const_double is fed to rs6000_is_valid_and_mask and from there to > rs6000_is_valid_mask where INTVAL is used, gcc will ICE. > > The num_insns_constant ICE was introduced with git commit f337168d97. > However, the code was buggy before that. There was no point in > testing for a mask since the mask predicates only handle const_int. > In fact, I don't think the function ever handled floating point > constants that might match a load of minus one and mask. It does now. > I've added a few comments regarding splitters so the next person > looking at this code can see how this works. > > The patch also extracts code out of num_insns_constant that needed to > handle multiple gprs for DFmode constants in 32-bit mode, to a > function that handles multiple gprs a little more generally. I don't > think there is any need for anything but the 32-bit DFmode case > currently, but this allows for possible future uses. The > CONST_WIDE_INT case is also not used currently, and needed fixing. > Adding CONST_WIDE_INT_NUNITS - 1 only makes sense if the elements of > the array were being shifted into a register of size larger than the > element size (which is 64-bits). > > Boostrapped etc. powerpc64le-linux and powerpc64-linux. > > * config/rs6000/rs6000.c (num_insns_constant_gpr): Renamed from > num_insns_constant_wide. Make static. Revise comment. > (num_insns_constant_multi): New function. > (num_insns_constant): Formatting. Correct CONST_WIDE_INT > calculation. Simplify and extract code common to both > CONST_INT and CONST_DOUBLE. Add gcc_unreachable for unhandled > const_double modes. > * config/rs6000/rs6000-protos.h (num_insns_const_wide): Delete. > +/* Helper for num_insn_constant. Allow constants formed by the > + num_insns_constant_gpr sequences, plus li -1, rldicl/rldicr/rlwinm, > + and handle modes that require multiple gprs. */ > + > +static int > +num_insns_constant_multi (HOST_WIDE_INT value, machine_mode mode) > +{ > + int nregs = (GET_MODE_SIZE (mode) + UNITS_PER_WORD - 1) / UNITS_PER_WORD; > + int total = 0; > + while (nregs-- > 0) > + { > + int ins = num_insns_constant_gpr (value); You probably should mask "value" here so that it is only one GPR. Alternatively, make num_insns_constant_gpr handle that. Consider what happens for a 64-bit number with 32-bit registers here? Also, s/ins/insns/ please. > + if (ins > 2 > + /* We won't get more than 2 from num_insns_constant_gpr > + except when TARGET_POWERPC64 and mode is DImode or > + wider, so the register mode must be DImode. */ > + && rs6000_is_valid_and_mask (GEN_INT (value), DImode)) > + ins = 2; > + total += ins; > + value >>= 32; > + if (TARGET_POWERPC64) > + value >>= 32; That's just value >>= BITS_PER_WORD; > + long l[2]; > + val = l[WORDS_BIG_ENDIAN ? 0 : 1] << 32; This doesn't work, as in the other patch: long can be 32 bit. Looks good otherwise. Segher
On Thu, Nov 29, 2018 at 01:52:34PM -0600, Segher Boessenkool wrote: > On Sun, Nov 25, 2018 at 10:49:12PM +1030, Alan Modra wrote: > > + while (nregs-- > 0) > > + { > > + int ins = num_insns_constant_gpr (value); > > You probably should mask "value" here so that it is only one GPR. > Alternatively, make num_insns_constant_gpr handle that. Consider what > happens for a 64-bit number with 32-bit registers here? Oh, right, the low part will always give an answer of 2 if the high parts isn't merely a sign extension. > > + val = l[WORDS_BIG_ENDIAN ? 0 : 1] << 32; > > This doesn't work, as in the other patch: long can be 32 bit. Huh, I originally had "high" and "low" HOST_WIDE_INTs which avoided this sort of problem on 32-bit hosts. Revised patch follows. Bootstrapped and regression tested powerpc64le-linux. Hopefully this has removed all the stupidity. * config/rs6000/rs6000.c (num_insns_constant_gpr): Renamed from num_insns_constant_wide. Make static. Revise comment. (num_insns_constant_multi): New function. (num_insns_constant): Formatting. Correct CONST_WIDE_INT calculation. Simplify and extract code common to both CONST_INT and CONST_DOUBLE. Add gcc_unreachable for unhandled const_double modes. * config/rs6000/rs6000-protos.h (num_insns_const_wide): Delete. diff --git a/gcc/config/rs6000/rs6000-protos.h b/gcc/config/rs6000/rs6000-protos.h index 1dfe7995ff1..dfee1f28aa9 100644 --- a/gcc/config/rs6000/rs6000-protos.h +++ b/gcc/config/rs6000/rs6000-protos.h @@ -36,7 +36,6 @@ extern int vspltis_shifted (rtx); extern HOST_WIDE_INT const_vector_elt_as_int (rtx, unsigned int); extern bool macho_lo_sum_memory_operand (rtx, machine_mode); extern int num_insns_constant (rtx, machine_mode); -extern int num_insns_constant_wide (HOST_WIDE_INT); extern int small_data_operand (rtx, machine_mode); extern bool mem_operand_gpr (rtx, machine_mode); extern bool mem_operand_ds_form (rtx, machine_mode); diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 02e69c103ec..c438fdc64fe 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -5821,11 +5821,12 @@ direct_return (void) return 0; } -/* Return the number of instructions it takes to form a constant in an - integer register. */ +/* Helper for num_insns_constant. Calculate number of instructions to + load VALUE to a single gpr using combinations of addi, addis, ori, + oris and sldi instructions. */ -int -num_insns_constant_wide (HOST_WIDE_INT value) +static int +num_insns_constant_gpr (HOST_WIDE_INT value) { /* signed constant loadable with addi */ if (((unsigned HOST_WIDE_INT) value + 0x8000) < 0x10000) @@ -5847,85 +5848,108 @@ num_insns_constant_wide (HOST_WIDE_INT value) high >>= 1; if (low == 0) - return num_insns_constant_wide (high) + 1; + return num_insns_constant_gpr (high) + 1; else if (high == 0) - return num_insns_constant_wide (low) + 1; + return num_insns_constant_gpr (low) + 1; else - return (num_insns_constant_wide (high) - + num_insns_constant_wide (low) + 1); + return (num_insns_constant_gpr (high) + + num_insns_constant_gpr (low) + 1); } else return 2; } +/* Helper for num_insns_constant. Allow constants formed by the + num_insns_constant_gpr sequences, plus li -1, rldicl/rldicr/rlwinm, + and handle modes that require multiple gprs. */ + +static int +num_insns_constant_multi (HOST_WIDE_INT value, machine_mode mode) +{ + int nregs = (GET_MODE_SIZE (mode) + UNITS_PER_WORD - 1) / UNITS_PER_WORD; + int total = 0; + while (nregs-- > 0) + { + HOST_WIDE_INT low = sext_hwi (value, BITS_PER_WORD); + int insns = num_insns_constant_gpr (low); + if (insns > 2 + /* We won't get more than 2 from num_insns_constant_gpr + except when TARGET_POWERPC64 and mode is DImode or + wider, so the register mode must be DImode. */ + && rs6000_is_valid_and_mask (GEN_INT (low), DImode)) + insns = 2; + total += insns; + value >>= BITS_PER_WORD; + } + return total; +} + +/* Return the number of instructions it takes to form a constant in as + many gprs are needed for MODE. */ + int num_insns_constant (rtx op, machine_mode mode) { - HOST_WIDE_INT low, high; + HOST_WIDE_INT val; switch (GET_CODE (op)) { case CONST_INT: - if ((INTVAL (op) >> 31) != 0 && (INTVAL (op) >> 31) != -1 - && rs6000_is_valid_and_mask (op, mode)) - return 2; - else - return num_insns_constant_wide (INTVAL (op)); + val = INTVAL (op); + break; case CONST_WIDE_INT: { - int i; - int ins = CONST_WIDE_INT_NUNITS (op) - 1; - for (i = 0; i < CONST_WIDE_INT_NUNITS (op); i++) - ins += num_insns_constant_wide (CONST_WIDE_INT_ELT (op, i)); - return ins; + int insns = 0; + for (int i = 0; i < CONST_WIDE_INT_NUNITS (op); i++) + insns += num_insns_constant_multi (CONST_WIDE_INT_ELT (op, i), + DImode); + return insns; } - case CONST_DOUBLE: + case CONST_DOUBLE: + { + const struct real_value *rv = CONST_DOUBLE_REAL_VALUE (op); + if (mode == SFmode || mode == SDmode) { long l; - if (DECIMAL_FLOAT_MODE_P (mode)) - REAL_VALUE_TO_TARGET_DECIMAL32 - (*CONST_DOUBLE_REAL_VALUE (op), l); + if (mode == SDmode) + REAL_VALUE_TO_TARGET_DECIMAL32 (*rv, l); else - REAL_VALUE_TO_TARGET_SINGLE (*CONST_DOUBLE_REAL_VALUE (op), l); - return num_insns_constant_wide ((HOST_WIDE_INT) l); + REAL_VALUE_TO_TARGET_SINGLE (*rv, l); + /* See the first define_split in rs6000.md handling a + const_double_operand. */ + val = l; + mode = SImode; } - - long l[2]; - if (DECIMAL_FLOAT_MODE_P (mode)) - REAL_VALUE_TO_TARGET_DECIMAL64 (*CONST_DOUBLE_REAL_VALUE (op), l); - else - REAL_VALUE_TO_TARGET_DOUBLE (*CONST_DOUBLE_REAL_VALUE (op), l); - high = l[WORDS_BIG_ENDIAN == 0]; - low = l[WORDS_BIG_ENDIAN != 0]; - - if (TARGET_32BIT) - return (num_insns_constant_wide (low) - + num_insns_constant_wide (high)); - else + else if (mode == DFmode || mode == DDmode) { - if ((high == 0 && low >= 0) - || (high == -1 && low < 0)) - return num_insns_constant_wide (low); - - else if (rs6000_is_valid_and_mask (op, mode)) - return 2; - - else if (low == 0) - return num_insns_constant_wide (high) + 1; + long l[2]; + if (mode == DDmode) + REAL_VALUE_TO_TARGET_DECIMAL64 (*rv, l); else - return (num_insns_constant_wide (high) - + num_insns_constant_wide (low) + 1); + REAL_VALUE_TO_TARGET_DOUBLE (*rv, l); + + /* See the second (32-bit) and third (64-bit) define_split + in rs6000.md handling a const_double_operand. */ + val = (unsigned HOST_WIDE_INT) l[WORDS_BIG_ENDIAN ? 0 : 1] << 32; + val |= l[WORDS_BIG_ENDIAN ? 1 : 0] & 0xffffffffUL; + mode = DImode; } + else + gcc_unreachable (); + } + break; default: gcc_unreachable (); } + + return num_insns_constant_multi (val, mode); } /* Interpret element ELT of the CONST_VECTOR OP as an integer value.
On Fri, Nov 30, 2018 at 06:16:40PM +1030, Alan Modra wrote: > On Thu, Nov 29, 2018 at 01:52:34PM -0600, Segher Boessenkool wrote: > > On Sun, Nov 25, 2018 at 10:49:12PM +1030, Alan Modra wrote: > > > + while (nregs-- > 0) > > > + { > > > + int ins = num_insns_constant_gpr (value); > > > > You probably should mask "value" here so that it is only one GPR. > > Alternatively, make num_insns_constant_gpr handle that. Consider what > > happens for a 64-bit number with 32-bit registers here? > > Oh, right, the low part will always give an answer of 2 if the high > parts isn't merely a sign extension. > > > > + val = l[WORDS_BIG_ENDIAN ? 0 : 1] << 32; > > > > This doesn't work, as in the other patch: long can be 32 bit. > > Huh, I originally had "high" and "low" HOST_WIDE_INTs which avoided > this sort of problem on 32-bit hosts. > > Revised patch follows. Bootstrapped and regression tested > powerpc64le-linux. Hopefully this has removed all the stupidity. I think it is fine like this, or certainly an improvement :-) Okay for trunk. Thanks! Segher > * config/rs6000/rs6000.c (num_insns_constant_gpr): Renamed from > num_insns_constant_wide. Make static. Revise comment. > (num_insns_constant_multi): New function. > (num_insns_constant): Formatting. Correct CONST_WIDE_INT > calculation. Simplify and extract code common to both > CONST_INT and CONST_DOUBLE. Add gcc_unreachable for unhandled > const_double modes. > * config/rs6000/rs6000-protos.h (num_insns_const_wide): Delete.
diff --git a/gcc/config/rs6000/rs6000-protos.h b/gcc/config/rs6000/rs6000-protos.h index 1dfe7995ff1..dfee1f28aa9 100644 --- a/gcc/config/rs6000/rs6000-protos.h +++ b/gcc/config/rs6000/rs6000-protos.h @@ -36,7 +36,6 @@ extern int vspltis_shifted (rtx); extern HOST_WIDE_INT const_vector_elt_as_int (rtx, unsigned int); extern bool macho_lo_sum_memory_operand (rtx, machine_mode); extern int num_insns_constant (rtx, machine_mode); -extern int num_insns_constant_wide (HOST_WIDE_INT); extern int small_data_operand (rtx, machine_mode); extern bool mem_operand_gpr (rtx, machine_mode); extern bool mem_operand_ds_form (rtx, machine_mode); diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 42dcb61904c..3364068d976 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -5815,11 +5815,12 @@ direct_return (void) return 0; } -/* Return the number of instructions it takes to form a constant in an - integer register. */ +/* Helper for num_insn_constant. Calculate number of instructions to + load VALUE to a single gpr using combinations of addi, addis, ori, + oris and sldi instructions. */ -int -num_insns_constant_wide (HOST_WIDE_INT value) +static int +num_insns_constant_gpr (HOST_WIDE_INT value) { /* signed constant loadable with addi */ if (((unsigned HOST_WIDE_INT) value + 0x8000) < 0x10000) @@ -5841,85 +5842,108 @@ num_insns_constant_wide (HOST_WIDE_INT value) high >>= 1; if (low == 0) - return num_insns_constant_wide (high) + 1; + return num_insns_constant_gpr (high) + 1; else if (high == 0) - return num_insns_constant_wide (low) + 1; + return num_insns_constant_gpr (low) + 1; else - return (num_insns_constant_wide (high) - + num_insns_constant_wide (low) + 1); + return (num_insns_constant_gpr (high) + + num_insns_constant_gpr (low) + 1); } else return 2; } +/* Helper for num_insn_constant. Allow constants formed by the + num_insns_constant_gpr sequences, plus li -1, rldicl/rldicr/rlwinm, + and handle modes that require multiple gprs. */ + +static int +num_insns_constant_multi (HOST_WIDE_INT value, machine_mode mode) +{ + int nregs = (GET_MODE_SIZE (mode) + UNITS_PER_WORD - 1) / UNITS_PER_WORD; + int total = 0; + while (nregs-- > 0) + { + int ins = num_insns_constant_gpr (value); + if (ins > 2 + /* We won't get more than 2 from num_insns_constant_gpr + except when TARGET_POWERPC64 and mode is DImode or + wider, so the register mode must be DImode. */ + && rs6000_is_valid_and_mask (GEN_INT (value), DImode)) + ins = 2; + total += ins; + value >>= 32; + if (TARGET_POWERPC64) + value >>= 32; + } + return total; +} + +/* Return the number of instructions it takes to form a constant in as + many gprs are needed for MODE. */ + int num_insns_constant (rtx op, machine_mode mode) { - HOST_WIDE_INT low, high; + HOST_WIDE_INT val; switch (GET_CODE (op)) { case CONST_INT: - if ((INTVAL (op) >> 31) != 0 && (INTVAL (op) >> 31) != -1 - && rs6000_is_valid_and_mask (op, mode)) - return 2; - else - return num_insns_constant_wide (INTVAL (op)); + val = INTVAL (op); + break; case CONST_WIDE_INT: { - int i; - int ins = CONST_WIDE_INT_NUNITS (op) - 1; - for (i = 0; i < CONST_WIDE_INT_NUNITS (op); i++) - ins += num_insns_constant_wide (CONST_WIDE_INT_ELT (op, i)); + int ins = 0; + for (int i = 0; i < CONST_WIDE_INT_NUNITS (op); i++) + ins += num_insns_constant_multi (CONST_WIDE_INT_ELT (op, i), DImode); return ins; } - case CONST_DOUBLE: + case CONST_DOUBLE: + { + const struct real_value *rv = CONST_DOUBLE_REAL_VALUE (op); + if (mode == SFmode || mode == SDmode) { long l; - if (DECIMAL_FLOAT_MODE_P (mode)) - REAL_VALUE_TO_TARGET_DECIMAL32 - (*CONST_DOUBLE_REAL_VALUE (op), l); + if (mode == SDmode) + REAL_VALUE_TO_TARGET_DECIMAL32 (*rv, l); else - REAL_VALUE_TO_TARGET_SINGLE (*CONST_DOUBLE_REAL_VALUE (op), l); - return num_insns_constant_wide ((HOST_WIDE_INT) l); + REAL_VALUE_TO_TARGET_SINGLE (*rv, l); + /* See the first define_split in rs6000.md handling a + const_double_operand. */ + val = l; + mode = SImode; } - - long l[2]; - if (DECIMAL_FLOAT_MODE_P (mode)) - REAL_VALUE_TO_TARGET_DECIMAL64 (*CONST_DOUBLE_REAL_VALUE (op), l); - else - REAL_VALUE_TO_TARGET_DOUBLE (*CONST_DOUBLE_REAL_VALUE (op), l); - high = l[WORDS_BIG_ENDIAN == 0]; - low = l[WORDS_BIG_ENDIAN != 0]; - - if (TARGET_32BIT) - return (num_insns_constant_wide (low) - + num_insns_constant_wide (high)); - else + else if (mode == DFmode || mode == DDmode) { - if ((high == 0 && low >= 0) - || (high == -1 && low < 0)) - return num_insns_constant_wide (low); - - else if (rs6000_is_valid_and_mask (op, mode)) - return 2; - - else if (low == 0) - return num_insns_constant_wide (high) + 1; + long l[2]; + if (mode == DDmode) + REAL_VALUE_TO_TARGET_DECIMAL64 (*rv, l); else - return (num_insns_constant_wide (high) - + num_insns_constant_wide (low) + 1); + REAL_VALUE_TO_TARGET_DOUBLE (*rv, l); + + /* See the second (32-bit) and third (64-bit) define_split + in rs6000.md handling a const_double_operand. */ + val = l[WORDS_BIG_ENDIAN ? 0 : 1] << 32; + val |= l[WORDS_BIG_ENDIAN ? 1 : 0] & 0xffffffffUL; + mode = DImode; } + else + gcc_unreachable (); + } + break; default: gcc_unreachable (); } + + return num_insns_constant_multi (val, mode); } /* Interpret element ELT of the CONST_VECTOR OP as an integer value.