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....@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
