| Message ID | 1439341904-9345-2-git-send-email-rth@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Aug 11, 2015 at 06:11:30PM -0700, Richard Henderson wrote: > This allows testing for a mask without having to call GEN_INT. > > Cc: David Edelsohn <dje.gcc@gmail.com> > --- > * config/rs6000/rs6000.c (rs6000_is_valid_mask_wide): Split out from... > (rs6000_is_valid_mask): ... here. > (rs6000_is_valid_and_mask_wide): Split out from... > (rs6000_is_valid_and_mask): ... here. I don't like these "_wide" names much. You could overload the shorter name, if you really think creating some garbage const_int's is too much overhead (it might well be if you use it a lot more in later patches). The original functions really want rtx's since they are used like predicates (so should look and behave like one); rs6000_is_valid_mask itself is different (and a lousy name; suggestions welcome). > -bool > -rs6000_is_valid_mask (rtx mask, int *b, int *e, machine_mode mode) > +static bool > +rs6000_is_valid_mask_wide (unsigned HOST_WIDE_INT val, int *b, int *e, int n) But why change the mode parameter? The code was clearer before. > { > - unsigned HOST_WIDE_INT val = INTVAL (mask); > unsigned HOST_WIDE_INT bit; > int nb, ne; > - int n = GET_MODE_PRECISION (mode); > > - if (mode != DImode && mode != SImode) > - return false; > - > - if (INTVAL (mask) >= 0) > + if ((HOST_WIDE_INT)val >= 0) ^ missing space > { > bit = val & -val; > ne = exact_log2 (bit); > @@ -16430,27 +16427,54 @@ rs6000_is_valid_mask (rtx mask, int *b, int *e, machine_mode mode) > return true; > } > > +bool > +rs6000_is_valid_mask (rtx mask, int *b, int *e, machine_mode mode) > +{ > + int n; > + > + if (mode == DImode) > + n = 64; > + else if (mode == SImode) > + n = 32; > + else > + return false; > + > + unsigned HOST_WIDE_INT val = INTVAL (mask); > + return rs6000_is_valid_mask_wide (val, b, e, n); > +} > + > /* Return whether MASK (a CONST_INT) is a valid mask for any rlwinm, rldicl, > or rldicr instruction, to implement an AND with it in mode MODE. */ > > -bool > -rs6000_is_valid_and_mask (rtx mask, machine_mode mode) > +static bool > +rs6000_is_valid_and_mask_wide (unsigned HOST_WIDE_INT val, machine_mode mode) > { > int nb, ne; > > - if (!rs6000_is_valid_mask (mask, &nb, &ne, mode)) > - return false; > + switch (mode) > + { > + case DImode: > + if (!rs6000_is_valid_mask_wide (val, &nb, &ne, 64)) > + return false; > + /* For DImode, we need a rldicl, rldicr, or a rlwinm with > + mask that does not wrap. */ > + return (ne == 0 || nb == 63 || (nb < 32 && ne <= nb)); > > - /* For DImode, we need a rldicl, rldicr, or a rlwinm with mask that > - does not wrap. */ > - if (mode == DImode) > - return (ne == 0 || nb == 63 || (nb < 32 && ne <= nb)); > + case SImode: > + if (!rs6000_is_valid_mask_wide (val, &nb, &ne, 32)) > + return false; > + /* For SImode, rlwinm can do everything. */ > + return (nb < 32 && ne < 32); > > - /* For SImode, rlwinm can do everything. */ > - if (mode == SImode) > - return (nb < 32 && ne < 32); > + default: > + return false; > + } > +} > > - return false; You don't need any of these changes then, either. > +bool > +rs6000_is_valid_and_mask (rtx mask, machine_mode mode) > +{ > + return rs6000_is_valid_and_mask_wide (UINTVAL (mask), mode); > } > > /* Return the instruction template for an AND with mask in mode MODE, with > @@ -16739,12 +16763,12 @@ rs6000_is_valid_2insn_and (rtx c, machine_mode mode) > > /* Otherwise, fill in the lowest "hole"; if we can do the result with > one insn, we can do the whole thing with two. */ > - unsigned HOST_WIDE_INT val = INTVAL (c); > + unsigned HOST_WIDE_INT val = UINTVAL (c); Does it matter? Segher
On 08/12/2015 06:23 AM, Segher Boessenkool wrote: > On Tue, Aug 11, 2015 at 06:11:30PM -0700, Richard Henderson wrote: >> This allows testing for a mask without having to call GEN_INT. >> >> Cc: David Edelsohn <dje.gcc@gmail.com> >> --- >> * config/rs6000/rs6000.c (rs6000_is_valid_mask_wide): Split out from... >> (rs6000_is_valid_mask): ... here. >> (rs6000_is_valid_and_mask_wide): Split out from... >> (rs6000_is_valid_and_mask): ... here. > > I don't like these "_wide" names much. It follows the existing practice within the backend. > You could overload the shorter > name, if you really think creating some garbage const_int's is too much > overhead (it might well be if you use it a lot more in later patches). At one stage in the development (before I became much leaner with the search for rotate), it really really mattered. >> -bool >> -rs6000_is_valid_mask (rtx mask, int *b, int *e, machine_mode mode) >> +static bool >> +rs6000_is_valid_mask_wide (unsigned HOST_WIDE_INT val, int *b, int *e, int n) > > But why change the mode parameter? The code was clearer before. So that we don't have to look up GET_MODE_BITSIZE (mode). >> +static bool >> +rs6000_is_valid_and_mask_wide (unsigned HOST_WIDE_INT val, machine_mode mode) >> { >> int nb, ne; >> >> - if (!rs6000_is_valid_mask (mask, &nb, &ne, mode)) >> - return false; >> + switch (mode) >> + { >> + case DImode: >> + if (!rs6000_is_valid_mask_wide (val, &nb, &ne, 64)) >> + return false; >> + /* For DImode, we need a rldicl, rldicr, or a rlwinm with >> + mask that does not wrap. */ >> + return (ne == 0 || nb == 63 || (nb < 32 && ne <= nb)); >> >> - /* For DImode, we need a rldicl, rldicr, or a rlwinm with mask that >> - does not wrap. */ >> - if (mode == DImode) >> - return (ne == 0 || nb == 63 || (nb < 32 && ne <= nb)); >> + case SImode: >> + if (!rs6000_is_valid_mask_wide (val, &nb, &ne, 32)) >> + return false; >> + /* For SImode, rlwinm can do everything. */ >> + return (nb < 32 && ne < 32); >> >> - /* For SImode, rlwinm can do everything. */ >> - if (mode == SImode) >> - return (nb < 32 && ne < 32); >> + default: >> + return false; >> + } >> +} >> >> - return false; > > You don't need any of these changes then, either. True, not *needed* per-se, but if you look closer I'm combining conditionals. I think the replacement here is clearer. >> /* Otherwise, fill in the lowest "hole"; if we can do the result with >> one insn, we can do the whole thing with two. */ >> - unsigned HOST_WIDE_INT val = INTVAL (c); >> + unsigned HOST_WIDE_INT val = UINTVAL (c); > > Does it matter? No. r~
On Wed, Aug 12, 2015 at 08:50:48AM -0700, Richard Henderson wrote: > On 08/12/2015 06:23 AM, Segher Boessenkool wrote: > > On Tue, Aug 11, 2015 at 06:11:30PM -0700, Richard Henderson wrote: > >> This allows testing for a mask without having to call GEN_INT. > >> > >> Cc: David Edelsohn <dje.gcc@gmail.com> > >> --- > >> * config/rs6000/rs6000.c (rs6000_is_valid_mask_wide): Split out from... > >> (rs6000_is_valid_mask): ... here. > >> (rs6000_is_valid_and_mask_wide): Split out from... > >> (rs6000_is_valid_and_mask): ... here. > > > > I don't like these "_wide" names much. > > It follows the existing practice within the backend. One existing function name, yes. And you are replacing that function :-) > > You could overload the shorter > > name, if you really think creating some garbage const_int's is too much > > overhead (it might well be if you use it a lot more in later patches). > > At one stage in the development (before I became much leaner with the search > for rotate), it really really mattered. For the AND patterns I considered such a search too; I didn't do it because as you say it would have to consider a *lot* of possibilities, most useless. AND sequences of more than two insns often prevented other optimisation too, so I settled on two insns maximum, and then you can generate it directly no problem. So yes if you call it way too often it also creates too much garbage. > >> -bool > >> -rs6000_is_valid_mask (rtx mask, int *b, int *e, machine_mode mode) > >> +static bool > >> +rs6000_is_valid_mask_wide (unsigned HOST_WIDE_INT val, int *b, int *e, int n) > > > > But why change the mode parameter? The code was clearer before. > > So that we don't have to look up GET_MODE_BITSIZE (mode). Getting rid of a single array lookup matters more than interface clarity? You must have been calling it *very* often! Thankfully you don't anymore. > >> +static bool > >> +rs6000_is_valid_and_mask_wide (unsigned HOST_WIDE_INT val, machine_mode mode) > >> { > >> int nb, ne; > >> > >> - if (!rs6000_is_valid_mask (mask, &nb, &ne, mode)) > >> - return false; > >> + switch (mode) > >> + { > >> + case DImode: > >> + if (!rs6000_is_valid_mask_wide (val, &nb, &ne, 64)) > >> + return false; > >> + /* For DImode, we need a rldicl, rldicr, or a rlwinm with > >> + mask that does not wrap. */ > >> + return (ne == 0 || nb == 63 || (nb < 32 && ne <= nb)); > >> > >> - /* For DImode, we need a rldicl, rldicr, or a rlwinm with mask that > >> - does not wrap. */ > >> - if (mode == DImode) > >> - return (ne == 0 || nb == 63 || (nb < 32 && ne <= nb)); > >> + case SImode: > >> + if (!rs6000_is_valid_mask_wide (val, &nb, &ne, 32)) > >> + return false; > >> + /* For SImode, rlwinm can do everything. */ > >> + return (nb < 32 && ne < 32); > >> > >> - /* For SImode, rlwinm can do everything. */ > >> - if (mode == SImode) > >> - return (nb < 32 && ne < 32); > >> + default: > >> + return false; > >> + } > >> +} > >> > >> - return false; > > > > You don't need any of these changes then, either. > > True, not *needed* per-se, but if you look closer I'm combining conditionals. > I think the replacement here is clearer. You're combining a conditional that you add (for mode -> 32,64), and the code doesn't become any clearer at all IMHO. > >> - unsigned HOST_WIDE_INT val = INTVAL (c); > >> + unsigned HOST_WIDE_INT val = UINTVAL (c); > > > > Does it matter? > > No. Ah okay, you were getting me worried! :-) Segher
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index e37ef9f..a33b9d3 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -1108,6 +1108,8 @@ static tree rs6000_handle_altivec_attribute (tree *, tree, tree, int, bool *); static tree rs6000_handle_struct_attribute (tree *, tree, tree, int, bool *); static tree rs6000_builtin_vectorized_libmass (tree, tree, tree); static void rs6000_emit_set_long_const (rtx, HOST_WIDE_INT); +static bool rs6000_is_valid_and_mask_wide (unsigned HOST_WIDE_INT val, + machine_mode mode); static int rs6000_memory_move_cost (machine_mode, reg_class_t, bool); static bool rs6000_debug_rtx_costs (rtx, machine_mode, int, int, int *, bool); static int rs6000_debug_address_cost (rtx, machine_mode, addr_space_t, @@ -16378,18 +16380,13 @@ validate_condition_mode (enum rtx_code code, machine_mode mode) the single stretch of 1 bits begins; and similarly for B, the bit offset where it ends. */ -bool -rs6000_is_valid_mask (rtx mask, int *b, int *e, machine_mode mode) +static bool +rs6000_is_valid_mask_wide (unsigned HOST_WIDE_INT val, int *b, int *e, int n) { - unsigned HOST_WIDE_INT val = INTVAL (mask); unsigned HOST_WIDE_INT bit; int nb, ne; - int n = GET_MODE_PRECISION (mode); - if (mode != DImode && mode != SImode) - return false; - - if (INTVAL (mask) >= 0) + if ((HOST_WIDE_INT)val >= 0) { bit = val & -val; ne = exact_log2 (bit); @@ -16430,27 +16427,54 @@ rs6000_is_valid_mask (rtx mask, int *b, int *e, machine_mode mode) return true; } +bool +rs6000_is_valid_mask (rtx mask, int *b, int *e, machine_mode mode) +{ + int n; + + if (mode == DImode) + n = 64; + else if (mode == SImode) + n = 32; + else + return false; + + unsigned HOST_WIDE_INT val = INTVAL (mask); + return rs6000_is_valid_mask_wide (val, b, e, n); +} + /* Return whether MASK (a CONST_INT) is a valid mask for any rlwinm, rldicl, or rldicr instruction, to implement an AND with it in mode MODE. */ -bool -rs6000_is_valid_and_mask (rtx mask, machine_mode mode) +static bool +rs6000_is_valid_and_mask_wide (unsigned HOST_WIDE_INT val, machine_mode mode) { int nb, ne; - if (!rs6000_is_valid_mask (mask, &nb, &ne, mode)) - return false; + switch (mode) + { + case DImode: + if (!rs6000_is_valid_mask_wide (val, &nb, &ne, 64)) + return false; + /* For DImode, we need a rldicl, rldicr, or a rlwinm with + mask that does not wrap. */ + return (ne == 0 || nb == 63 || (nb < 32 && ne <= nb)); - /* For DImode, we need a rldicl, rldicr, or a rlwinm with mask that - does not wrap. */ - if (mode == DImode) - return (ne == 0 || nb == 63 || (nb < 32 && ne <= nb)); + case SImode: + if (!rs6000_is_valid_mask_wide (val, &nb, &ne, 32)) + return false; + /* For SImode, rlwinm can do everything. */ + return (nb < 32 && ne < 32); - /* For SImode, rlwinm can do everything. */ - if (mode == SImode) - return (nb < 32 && ne < 32); + default: + return false; + } +} - return false; +bool +rs6000_is_valid_and_mask (rtx mask, machine_mode mode) +{ + return rs6000_is_valid_and_mask_wide (UINTVAL (mask), mode); } /* Return the instruction template for an AND with mask in mode MODE, with @@ -16739,12 +16763,12 @@ rs6000_is_valid_2insn_and (rtx c, machine_mode mode) /* Otherwise, fill in the lowest "hole"; if we can do the result with one insn, we can do the whole thing with two. */ - unsigned HOST_WIDE_INT val = INTVAL (c); + unsigned HOST_WIDE_INT val = UINTVAL (c); unsigned HOST_WIDE_INT bit1 = val & -val; unsigned HOST_WIDE_INT bit2 = (val + bit1) & ~val; unsigned HOST_WIDE_INT val1 = (val + bit1) & val; unsigned HOST_WIDE_INT bit3 = val1 & -val1; - return rs6000_is_valid_and_mask (GEN_INT (val + bit3 - bit2), mode); + return rs6000_is_valid_and_mask_wide (val + bit3 - bit2, mode); } /* Emit a potentially record-form instruction, setting DST from SRC. @@ -16835,10 +16859,10 @@ rs6000_emit_2insn_and (machine_mode mode, rtx *operands, bool expand, int dot) unsigned HOST_WIDE_INT mask1 = -bit3 + bit2 - 1; unsigned HOST_WIDE_INT mask2 = val + bit3 - bit2; - gcc_assert (rs6000_is_valid_and_mask (GEN_INT (mask2), mode)); + gcc_assert (rs6000_is_valid_and_mask_wide (mask2, mode)); /* Two "no-rotate"-and-mask instructions, for SImode. */ - if (rs6000_is_valid_and_mask (GEN_INT (mask1), mode)) + if (rs6000_is_valid_and_mask_wide (mask1, mode)) { gcc_assert (mode == SImode); @@ -16855,7 +16879,7 @@ rs6000_emit_2insn_and (machine_mode mode, rtx *operands, bool expand, int dot) /* Two "no-rotate"-and-mask instructions, for DImode: both are rlwinm insns; we have to do the first in SImode, because it wraps. */ if (mask2 <= 0xffffffff - && rs6000_is_valid_and_mask (GEN_INT (mask1), SImode)) + && rs6000_is_valid_and_mask_wide (mask1, SImode)) { rtx reg = expand ? gen_reg_rtx (mode) : operands[0]; rtx tmp = gen_rtx_AND (SImode, gen_lowpart (SImode, operands[1]), @@ -31070,7 +31094,7 @@ rs6000_rtx_costs (rtx x, machine_mode mode, int outer_code, /* rotate-and-mask (no rotate), andi., andis.: 1 insn. */ HOST_WIDE_INT val = INTVAL (XEXP (x, 1)); - if (rs6000_is_valid_and_mask (XEXP (x, 1), mode) + if (rs6000_is_valid_and_mask_wide (val, mode) || (val & 0xffff) == val || (val & 0xffff0000) == val || ((val & 0xffff) == 0 && mode == SImode))