diff mbox series

Prepare for prefixed instructions on PowerPC

Message ID 20190627201800.GA30249@ibm-toto.the-meissners.org
State New
Headers show
Series Prepare for prefixed instructions on PowerPC | expand

Commit Message

Michael Meissner June 27, 2019, 8:18 p.m. UTC 
A future PowerPC machine may have prefixed instructions.  This patch changes
the RTL "length" attribute of all of the "mov*", and "*extend*" insns from an
explicit "4" to "*".  This change prepares for the length attribute to be set
appropriately for single instruction loads, stores, and add immediates that may
include prefixed instructions.  The idea is to do this patch now, rather than
having these lines be part of the larger patches.

As we discussed off-line earlier, I changed all of the "4" lengths to be "*",
even for instruction alternatives that would not be subject to being changed to
be a prefixed instruction (i.e. the sign extend instruction "extsw"'s length is
set to "*", even though it would not be a prefixed instruction).  I also
changed the Altivec load/store instructions, as well as a power8 permute
instruction just be consistant with the other changes.

This patch does not fix the places where the length is not "4".  Those lengths
will be updated as needed as the rest of the prefixed instruction support is
rolled out.  It is intended to be a simple patch to make the other patches
somewhat simpler.

I did a bootstrap and make check.  There were no regressions.  Can I check this
patch into the trunk?

2019-06-27  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/altivec.md (altivec_mov<mode>, VM2 iterator):
	Change the RTL attribute "length" from "4" to "*" to allow the
	length attribute to be adjusted automatically for prefixed load,
	store, and add immediate instructions.
	* config/rs6000/rs6000.md (extendhi<mode>2, EXTHI iterator):
	Likewise.
	(extendsi<mode>2, EXTSI iterator): Likewise.
	(movsi_internal1): Likewise.
	(movsi_from_sf): Likewise.
	(movdi_from_sf_zero_ext): Likewise.
	(mov<mode>_internal): Likewise.
	(movcc_internal1, QHI iterator): Likewise.
	(mov<mode>_softfloat, FMOVE32 iterator): Likewise.
	(movsf_from_si): Likewise.
	(mov<mode>_hardfloat32, FMOVE64 iterator): Likewise.
	(mov<mode>_softfloat64, FMOVE64 iterator): Likewise.
	(mov<mode>, FMOVE128 iterator): Likewise.
	(movdi_internal64): Likewise.
	* config/rs6000/vsx.md (vsx_le_permute_<mode>, VSX_TI iterator):
	Likewise.
	(vsx_le_undo_permute_<mode>, VSX_TI iterator): Likewise.
	(vsx_mov<mode>_64bit, VSX_M iterator): Likewise.
	(vsx_mov<mode>_32bit, VSX_M iterator): Likewise.
	(vsx_splat_v4sf): Likewise.

Comments

Segher Boessenkool July 1, 2019, 9:27 p.m. UTC | #1 
Hi Mike,

Sorry I missed this patch :-(

On Thu, Jun 27, 2019 at 04:18:00PM -0400, Michael Meissner wrote:
> As we discussed off-line earlier, I changed all of the "4" lengths to be "*",
> even for instruction alternatives that would not be subject to being changed to
> be a prefixed instruction (i.e. the sign extend instruction "extsw"'s length is
> set to "*", even though it would not be a prefixed instruction).

"*" means "use the default for this attribute", which often is nicer to
see than "4".  For example, "8" stands out more in a sea of "*"s.

Usually "*" is the insns defined as "normal" alternatives, and "8" or
"12" etc. are split.

> @@ -7231,7 +7231,7 @@ (define_insn "*movcc_internal1"
>        (const_string "mtjmpr")
>        (const_string "load")
>        (const_string "store")])
> -   (set_attr "length" "4,4,12,4,4,8,4,4,4,4,4,4")])
> +   (set_attr "length" "*,*,12,*,*,8,*,*,*,*,*,*")])

In this case, the "12" and "8" are actually defined as one insn in the
template, with some "\;".  Luckily there aren't many of those.

> @@ -7385,8 +7385,8 @@ (define_insn "*mov<mode>_softfloat"
>           *,          *,         *,         *")
>  
>     (set_attr "length"
> -	"4,          4,         4,         4,         4,         4,
> -         4,          4,         8,         4")])
> +	"*,          *,         *,         *,         *,         *,
> +         *,          *,         8,         *")])

[ That last line should start with a tab as well. ]

The entry before the 8 is split as well.  Maybe that should be "4", to
stand out?  I don't know what works better; your choice.

> @@ -7696,8 +7696,8 @@ (define_insn "*mov<mode>_softfloat64"
>               *,       *,      *")
>  
>     (set_attr "length"
> -            "4,       4,      4,      4,      4,      8,
> -             12,      16,     4")])
> +            "*,       *,      *,      *,      *,      8,
> +             12,      16,     *")])

Same for the last entry here.

> @@ -8760,10 +8760,10 @@ (define_insn "*movdi_internal32"
>            vecsimple")
>     (set_attr "size" "64")
>     (set_attr "length"
> -         "8,         8,         8,         4,         4,         4,
> -          16,        4,         4,         4,         4,         4,
> -          4,         4,         4,         4,         4,         8,
> -          4")
> +         "8,         8,         8,         *,         *,         *,
> +          16,        *,         *,         *,         *,         *,
> +          *,         *,         *,         *,         *,         8,
> +          *")

And the last here.

> @@ -8853,11 +8853,11 @@ (define_insn "*movdi_internal64"
>                  mftgpr,    mffgpr")
>     (set_attr "size" "64")
>     (set_attr "length"
> -               "4,         4,         4,         4,         4,          20,
> -                4,         4,         4,         4,         4,          4,
> -                4,         4,         4,         4,         4,          4,
> -                4,         8,         4,         4,         4,          4,
> -                4,         4")
> +               "*,         *,         *,         *,         *,          20,
> +                *,         *,         *,         *,         *,          *,
> +                *,         *,         *,         *,         *,          *,
> +                *,         8,         *,         *,         *,          *,
> +                *,         *")

And two of the entries here.

> @@ -1150,9 +1150,9 @@ (define_insn "vsx_mov<mode>_64bit"
>                  store,     load,      store,     *,         vecsimple, vecsimple,
>                  vecsimple, *,         *,         vecstore,  vecload")
>     (set_attr "length"
> -               "4,         4,         4,         8,         4,         8,
> -                8,         8,         8,         8,         4,         4,
> -                4,         20,        8,         4,         4")
> +               "*,         *,         *,         8,         *,         8,
> +                8,         8,         8,         8,         *,         *,
> +                *,         20,        8,         *,         *")

No idea which ones are split here :-)  None of the * and all other would
be nice, but who knows :-)


Okay for trunk, maybe with some "4" if you agree that has value.  Thanks!
And again, sorry I missed this patch.


Segher
Michael Meissner July 2, 2019, 11:36 p.m. UTC | #2 
On Mon, Jul 01, 2019 at 04:27:05PM -0500, Segher Boessenkool wrote:
> The entry before the 8 is split as well.  Maybe that should be "4", to
> stand out?  I don't know what works better; your choice.

I'll look into it.  Note, the length is used in two places.  One at the end to
generate the appropriate branches, but the other is in rs6000_insn_cost inside
rs6000.c.  This occurs before the final passes, so it is important that even
though the insn will be split, that the length is still set.  However, things
are rather inconsistant, in that sometimes the length field is accurate, and
sometimes not.

I'm finding that the rs6000_insn_cost issue really muddies things up,
particularly if a vector load/store insn is done in gpr registers, where it can
be 4 insns.
Segher Boessenkool July 3, 2019, 12:09 a.m. UTC | #3 
On Tue, Jul 02, 2019 at 07:36:21PM -0400, Michael Meissner wrote:
> On Mon, Jul 01, 2019 at 04:27:05PM -0500, Segher Boessenkool wrote:
> > The entry before the 8 is split as well.  Maybe that should be "4", to
> > stand out?  I don't know what works better; your choice.
> 
> I'll look into it.  Note, the length is used in two places.  One at the end to
> generate the appropriate branches, but the other is in rs6000_insn_cost inside
> rs6000.c.

We'll need to update our insn_cost for prefixed, sure, it currently does
  int n = get_attr_length (insn) / 4;
to figure out how many machine instructions a pattern is, and then uses
"n" differently for the different types of insn.  We'll need to refine
this a bit for prefixed instructions.

> This occurs before the final passes, so it is important that even
> though the insn will be split, that the length is still set.  However, things
> are rather inconsistant, in that sometimes the length field is accurate, and
> sometimes not.

It *has* to be correct; it is allowed to be pessimistic though.  This
is used to determine if a B-form branch can reach, for example.  You get
ICEs if it isn't correct.  Only on very few testcases, of course :-(

> I'm finding that the rs6000_insn_cost issue really muddies things up,
> particularly if a vector load/store insn is done in gpr registers, where it can
> be 4 insns.

Yeah, it doesn't handle vectors specially *at all* currently, not even
for FP in vectors.  It is pretty much just a switch over the "type", and
for everything vector it gets to
    default:
      cost = COSTS_N_INSNS (n);
(with the above "n") which is a pretty coarse approximation to the truth ;-)

You could try #undef'ing TARGET_INSN_COST (in rs6000.c) for now, and hope
that rs6000_rtx_costs does better for what you need right now.  In the end
it will have to be fixed properly, insn_cost is quite important.


Segher
Michael Meissner July 3, 2019, 4:50 p.m. UTC | #4 
On Tue, Jul 02, 2019 at 07:09:20PM -0500, Segher Boessenkool wrote:
> On Tue, Jul 02, 2019 at 07:36:21PM -0400, Michael Meissner wrote:
> > On Mon, Jul 01, 2019 at 04:27:05PM -0500, Segher Boessenkool wrote:
> > > The entry before the 8 is split as well.  Maybe that should be "4", to
> > > stand out?  I don't know what works better; your choice.
> > 
> > I'll look into it.  Note, the length is used in two places.  One at the end to
> > generate the appropriate branches, but the other is in rs6000_insn_cost inside
> > rs6000.c.
> 
> We'll need to update our insn_cost for prefixed, sure, it currently does
>   int n = get_attr_length (insn) / 4;
> to figure out how many machine instructions a pattern is, and then uses
> "n" differently for the different types of insn.  We'll need to refine
> this a bit for prefixed instructions.

Yes, I have some plans with this regard.  In particular, I will be introducing
a "num_insns" RTL attribute, that if set is the number of instructions that
will be emitted.

If "num_insns" is not set, then it will use the length and look at the
"prefixed" attribute.

> > This occurs before the final passes, so it is important that even
> > though the insn will be split, that the length is still set.  However, things
> > are rather inconsistant, in that sometimes the length field is accurate, and
> > sometimes not.
> 
> It *has* to be correct; it is allowed to be pessimistic though.  This
> is used to determine if a B-form branch can reach, for example.  You get
> ICEs if it isn't correct.  Only on very few testcases, of course :-(

To be clear.  Yes it has to be correct when the label handling is done.

What I was talking about is I've found some insns that don't set the length,
and are split.  Using the insn cost mechanism will mean that these instructions
will be thought of being cheaper than they actually are.

> > I'm finding that the rs6000_insn_cost issue really muddies things up,
> > particularly if a vector load/store insn is done in gpr registers, where it can
> > be 4 insns.
> 
> Yeah, it doesn't handle vectors specially *at all* currently, not even
> for FP in vectors.  It is pretty much just a switch over the "type", and
> for everything vector it gets to
>     default:
>       cost = COSTS_N_INSNS (n);
> (with the above "n") which is a pretty coarse approximation to the truth ;-)
> 
> You could try #undef'ing TARGET_INSN_COST (in rs6000.c) for now, and hope
> that rs6000_rtx_costs does better for what you need right now.  In the end
> it will have to be fixed properly, insn_cost is quite important.
> 
> 
> Segher
>
Michael Meissner July 3, 2019, 5:06 p.m. UTC | #5 
On Mon, Jul 01, 2019 at 04:27:05PM -0500, Segher Boessenkool wrote:
> Hi Mike,
> 
> Sorry I missed this patch :-(
> 
> On Thu, Jun 27, 2019 at 04:18:00PM -0400, Michael Meissner wrote:
> > As we discussed off-line earlier, I changed all of the "4" lengths to be "*",
> > even for instruction alternatives that would not be subject to being changed to
> > be a prefixed instruction (i.e. the sign extend instruction "extsw"'s length is
> > set to "*", even though it would not be a prefixed instruction).
> 
> "*" means "use the default for this attribute", which often is nicer to
> see than "4".  For example, "8" stands out more in a sea of "*"s.
> 
> Usually "*" is the insns defined as "normal" alternatives, and "8" or
> "12" etc. are split.
> 
> > @@ -7231,7 +7231,7 @@ (define_insn "*movcc_internal1"
> >        (const_string "mtjmpr")
> >        (const_string "load")
> >        (const_string "store")])
> > -   (set_attr "length" "4,4,12,4,4,8,4,4,4,4,4,4")])
> > +   (set_attr "length" "*,*,12,*,*,8,*,*,*,*,*,*")])
> 
> In this case, the "12" and "8" are actually defined as one insn in the
> template, with some "\;".  Luckily there aren't many of those.
> 
> > @@ -7385,8 +7385,8 @@ (define_insn "*mov<mode>_softfloat"
> >           *,          *,         *,         *")
> >  
> >     (set_attr "length"
> > -	"4,          4,         4,         4,         4,         4,
> > -         4,          4,         8,         4")])
> > +	"*,          *,         *,         *,         *,         *,
> > +         *,          *,         8,         *")])
> 
> [ That last line should start with a tab as well. ]

Ok.

> The entry before the 8 is split as well.  Maybe that should be "4", to
> stand out?  I don't know what works better; your choice.

Though the "G" constraint specifically says for SFmode it is a single
instruction.

> > @@ -7696,8 +7696,8 @@ (define_insn "*mov<mode>_softfloat64"
> >               *,       *,      *")
> >  
> >     (set_attr "length"
> > -            "4,       4,      4,      4,      4,      8,
> > -             12,      16,     4")])
> > +            "*,       *,      *,      *,      *,      8,
> > +             12,      16,     *")])
> 
> Same for the last entry here.

Well technically that alternative will never fire (destination is "*h" and
source is "0"), and a nop is emitted.  I do wish we could never ever load
floating point into SPR registers.  I've tried in the reload days to eliminate
it, but there was always some abort if it got eliminated.

> 
> > @@ -8760,10 +8760,10 @@ (define_insn "*movdi_internal32"
> >            vecsimple")
> >     (set_attr "size" "64")
> >     (set_attr "length"
> > -         "8,         8,         8,         4,         4,         4,
> > -          16,        4,         4,         4,         4,         4,
> > -          4,         4,         4,         4,         4,         8,
> > -          4")
> > +         "8,         8,         8,         *,         *,         *,
> > +          16,        *,         *,         *,         *,         *,
> > +          *,         *,         *,         *,         *,         8,
> > +          *")
> 
> And the last here.

Well it will be split into a single VSPLTISW instruction.

> 
> > @@ -8853,11 +8853,11 @@ (define_insn "*movdi_internal64"
> >                  mftgpr,    mffgpr")
> >     (set_attr "size" "64")
> >     (set_attr "length"
> > -               "4,         4,         4,         4,         4,          20,
> > -                4,         4,         4,         4,         4,          4,
> > -                4,         4,         4,         4,         4,          4,
> > -                4,         8,         4,         4,         4,          4,
> > -                4,         4")
> > +               "*,         *,         *,         *,         *,          20,
> > +                *,         *,         *,         *,         *,          *,
> > +                *,         *,         *,         *,         *,          *,
> > +                *,         8,         *,         *,         *,          *,
> > +                *,         *")
> 
> And two of the entries here.

Though the second split becomes a single VSPLTISW instruction once again.

> > @@ -1150,9 +1150,9 @@ (define_insn "vsx_mov<mode>_64bit"
> >                  store,     load,      store,     *,         vecsimple, vecsimple,
> >                  vecsimple, *,         *,         vecstore,  vecload")
> >     (set_attr "length"
> > -               "4,         4,         4,         8,         4,         8,
> > -                8,         8,         8,         8,         4,         4,
> > -                4,         20,        8,         4,         4")
> > +               "*,         *,         *,         8,         *,         8,
> > +                8,         8,         8,         8,         *,         *,
> > +                *,         20,        8,         *,         *")
> 
> No idea which ones are split here :-)  None of the * and all other would
> be nice, but who knows :-)

Yeah, it gets complicated with LQ and STQ having such weird rules.

> Okay for trunk, maybe with some "4" if you agree that has value.  Thanks!
> And again, sorry I missed this patch.

NP.
Segher Boessenkool July 3, 2019, 5:55 p.m. UTC | #6 
On Wed, Jul 03, 2019 at 12:50:37PM -0400, Michael Meissner wrote:
> On Tue, Jul 02, 2019 at 07:09:20PM -0500, Segher Boessenkool wrote:
> > We'll need to update our insn_cost for prefixed, sure, it currently does
> >   int n = get_attr_length (insn) / 4;
> > to figure out how many machine instructions a pattern is, and then uses
> > "n" differently for the different types of insn.  We'll need to refine
> > this a bit for prefixed instructions.
> 
> Yes, I have some plans with this regard.  In particular, I will be introducing
> a "num_insns" RTL attribute, that if set is the number of instructions that
> will be emitted.

I don't think this is a good idea.  You can set "cost" directly, if that
is the only thing you need this for?

> What I was talking about is I've found some insns that don't set the length,
> and are split.  Using the insn cost mechanism will mean that these instructions
> will be thought of being cheaper than they actually are.

Yes.  Please split RTL insns as early as possible.  It also matters for
scheduling, and it prevents exponential explosion of the number of
patterns you need.  Only sometimes do you need to split late, usually
because RA can put some registers in memory and you want to handle that
optimally, or things depend on what exact register you were allocated
(cr0 vs. crN for example, but could be GPR vs. VSR).

And again, you can set cost directly; length alone is not usually enough
for determining the cost of split patterns.  But you do need length for
accurate costs with -Os, hrm.


Segher
Michael Meissner July 3, 2019, 6:36 p.m. UTC | #7 
On Wed, Jul 03, 2019 at 12:55:41PM -0500, Segher Boessenkool wrote:
> On Wed, Jul 03, 2019 at 12:50:37PM -0400, Michael Meissner wrote:
> > On Tue, Jul 02, 2019 at 07:09:20PM -0500, Segher Boessenkool wrote:
> > > We'll need to update our insn_cost for prefixed, sure, it currently does
> > >   int n = get_attr_length (insn) / 4;
> > > to figure out how many machine instructions a pattern is, and then uses
> > > "n" differently for the different types of insn.  We'll need to refine
> > > this a bit for prefixed instructions.
> > 
> > Yes, I have some plans with this regard.  In particular, I will be introducing
> > a "num_insns" RTL attribute, that if set is the number of instructions that
> > will be emitted.
> 
> I don't think this is a good idea.  You can set "cost" directly, if that
> is the only thing you need this for?

The trouble is the cost is currently a factor based on type + the cost from the
cost structure.  It really would be hard to set it to a single value in the
insns without having to have complex means for setting the machine dependent
costs.  If the numeric RTL attributes could set the value from a function, it
would be simpler, but that isn't currently supported.

Here is my current version of rs6000_insn_cost.  At the moment, I'm only
setting the "num_insns" in a few places, so the default would normally kick in.

/* How many real instructions are generated for this insn?  This is slightly
   different from the length attribute, in that the length attribute counts the
   number of bytes.  With prefixed instructions, we don't want to count a
   prefixed instruction (length 12 bytes including possible NOP) as taking 3
   instructions, but just one.  */

static int
rs6000_num_insns (rtx_insn *insn)
{
  /* If the insn provides an override, use it.  */
  int num = get_attr_num_insns (insn);

  if (!num)
    {
      /* Try to figure it out based on the length and whether there are
	 prefixed instructions.  While prefixed instructions are only 8 bytes,
	 we have to use 12 as the size of the first prefixed instruction in
	 case the instruction needs to be aligned.  Back to back prefixed
	 instructions would only take 20 bytes, since it is guaranteed that one
	 of the prefixed instructions does not need the alignment.  */
      int length = get_attr_length (insn);

      if (length >= 12 && TARGET_PREFIXED_ADDR
	  && get_attr_prefixed (insn) == PREFIXED_YES)
	{
	  /* Single prefixed instruction.  */
	  if (length == 12)
	    return 1;

	  /* A normal instruction and a prefixed instruction (16) or two back
	     to back prefixed instructions (20).  */
	  if (length == 16 || length == 20)
	    return 2;

	  /* Guess for larger instruction sizes.  */
	  num = 2 + (length - 20) / 4;
	}
      else 
	num = length / 4;
    }

  return num;
}

rs6000_insn_cost (rtx_insn *insn, bool speed)
{
  int cost;

  if (recog_memoized (insn) < 0)
    return 0;

  if (!speed)
    return get_attr_length (insn);

  cost = get_attr_cost (insn);
  if (cost > 0)
    return cost;

  int n = rs6000_num_insns (insn);
  enum attr_type type = get_attr_type (insn);

  switch (type)
    {
    case TYPE_LOAD:
    case TYPE_FPLOAD:
    case TYPE_VECLOAD:
      cost = COSTS_N_INSNS (n + 1);
      break;

    case TYPE_MUL:
      switch (get_attr_size (insn))
	{
	case SIZE_8:
	  cost = COSTS_N_INSNS (n - 1) + rs6000_cost->mulsi_const9;
	  break;
	case SIZE_16:
	  cost = COSTS_N_INSNS (n - 1) + rs6000_cost->mulsi_const;
	  break;
	case SIZE_32:
	  cost = COSTS_N_INSNS (n - 1) + rs6000_cost->mulsi;
	  break;
	case SIZE_64:
	  cost = COSTS_N_INSNS (n - 1) + rs6000_cost->muldi;
	  break;
	default:
	  gcc_unreachable ();
	}
      break;

// ...

> > What I was talking about is I've found some insns that don't set the length,
> > and are split.  Using the insn cost mechanism will mean that these instructions
> > will be thought of being cheaper than they actually are.
> 
> Yes.  Please split RTL insns as early as possible.  It also matters for
> scheduling, and it prevents exponential explosion of the number of
> patterns you need.  Only sometimes do you need to split late, usually
> because RA can put some registers in memory and you want to handle that
> optimally, or things depend on what exact register you were allocated
> (cr0 vs. crN for example, but could be GPR vs. VSR).

Generally most of the places I've been modifying with splits need to be handled
after register allocation.

> And again, you can set cost directly; length alone is not usually enough
> for determining the cost of split patterns.  But you do need length for
> accurate costs with -Os, hrm.
Segher Boessenkool July 3, 2019, 7 p.m. UTC | #8 
On Wed, Jul 03, 2019 at 01:06:27PM -0400, Michael Meissner wrote:
> On Mon, Jul 01, 2019 at 04:27:05PM -0500, Segher Boessenkool wrote:
> > > @@ -7385,8 +7385,8 @@ (define_insn "*mov<mode>_softfloat"
> > >           *,          *,         *,         *")
> > >  
> > >     (set_attr "length"
> > > -	"4,          4,         4,         4,         4,         4,
> > > -         4,          4,         8,         4")])
> > > +	"*,          *,         *,         *,         *,         *,
> > > +         *,          *,         8,         *")])
> > 
> > [ That last line should start with a tab as well. ]
> 
> Ok.
> 
> > The entry before the 8 is split as well.  Maybe that should be "4", to
> > stand out?  I don't know what works better; your choice.
> 
> Though the "G" constraint specifically says for SFmode it is a single
> instruction.

Sure.  But that doesn't mean much, does it?  You cannot see the actual
split insns here, and that the constraint is for
  "Constant that can be copied into GPR with two insns for DF/DD
   and one for SF/SD."
is pretty weak :-)

The default is one insn, that's what * does.  You can still use 4 though
when that is beneficial, to make things a tiny bit clearer maybe.

> > > @@ -7696,8 +7696,8 @@ (define_insn "*mov<mode>_softfloat64"
> > >               *,       *,      *")
> > >  
> > >     (set_attr "length"
> > > -            "4,       4,      4,      4,      4,      8,
> > > -             12,      16,     4")])
> > > +            "*,       *,      *,      *,      *,      8,
> > > +             12,      16,     *")])
> > 
> > Same for the last entry here.
> 
> Well technically that alternative will never fire (destination is "*h" and
> source is "0"), and a nop is emitted.

If it can never fire we should remove it.  But it *can* fire, or I cannot
prove it cannot anyway.  Can you?

This would be a lovely cleanup if we could make it :-/

> I do wish we could never ever load
> floating point into SPR registers.  I've tried in the reload days to eliminate
> it, but there was always some abort if it got eliminated.

It should never load anything into CTR, LR, or VRSAVE, period.  :-)

Maybe we should use those as fixed registers, not allocatable as they
are now.


Segher
Segher Boessenkool July 3, 2019, 7:42 p.m. UTC | #9 
On Wed, Jul 03, 2019 at 02:36:11PM -0400, Michael Meissner wrote:
> On Wed, Jul 03, 2019 at 12:55:41PM -0500, Segher Boessenkool wrote:
> > I don't think this is a good idea.  You can set "cost" directly, if that
> > is the only thing you need this for?
> 
> The trouble is the cost is currently a factor based on type + the cost from the
> cost structure.  It really would be hard to set it to a single value in the
> insns without having to have complex means for setting the machine dependent
> costs.  If the numeric RTL attributes could set the value from a function, it
> would be simpler, but that isn't currently supported.

(set (attr "cost") (symbol_ref "any C expression you want here"))

It is supported.  The syntax is a bit weird, sure :-)


It may well be that some helper attribute can help here, but just
num_insns isn't it.


Segher
diff mbox series

Patch

Index: gcc/config/rs6000/altivec.md
===================================================================
--- gcc/config/rs6000/altivec.md	(revision 272714)
+++ gcc/config/rs6000/altivec.md	(working copy)
@@ -256,7 +256,7 @@  (define_insn "*altivec_mov<mode>"
    * return output_vec_const_move (operands);
    #"
   [(set_attr "type" "vecstore,vecload,veclogical,store,load,*,veclogical,*,*")
-   (set_attr "length" "4,4,4,20,20,20,4,8,32")])
+   (set_attr "length" "*,*,*,20,20,20,*,8,32")])
 
 ;; Unlike other altivec moves, allow the GPRs, since a normal use of TImode
 ;; is for unions.  However for plain data movement, slightly favor the vector
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 272719)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -965,7 +965,7 @@  (define_insn "*extendhi<mode>2"
    vextsh2d %0,%1"
   [(set_attr "type" "load,exts,fpload,vecperm")
    (set_attr "sign_extend" "yes")
-   (set_attr "length" "4,4,8,4")
+   (set_attr "length" "*,*,8,*")
    (set_attr "isa" "*,*,p9v,p9v")])
 
 (define_split
@@ -1040,7 +1040,7 @@  (define_insn "extendsi<mode>2"
    #"
   [(set_attr "type" "load,exts,fpload,fpload,mffgpr,vecexts,vecperm,mftgpr")
    (set_attr "sign_extend" "yes")
-   (set_attr "length" "4,4,4,4,4,4,8,8")
+   (set_attr "length" "*,*,*,*,*,*,8,8")
    (set_attr "isa" "*,*,p6,p8v,p8v,p9v,p8v,p8v")])
 
 (define_split
@@ -6915,11 +6915,11 @@  (define_insn "*movsi_internal1"
 		 veclogical, veclogical,  vecsimple,   mffgpr,      mftgpr,
 		 *,          *,           *")
    (set_attr "length"
-		"4,          4,           4,           4,           4,
-		 4,          4,           4,           4,           4,
-		 8,          4,           4,           4,           4,
-		 4,          4,           8,           4,           4,
-		 4,          4,           4")
+		"*,          *,           *,           *,           *,
+		 *,          *,           *,           *,           *,
+		 8,          *,           *,           *,           *,
+		 *,          *,           8,           *,           *,
+		 *,          *,           *")
    (set_attr "isa"
 		"*,          *,           *,           p8v,         p8v,
 		 *,          p8v,         p8v,         *,           *,
@@ -6995,9 +6995,9 @@  (define_insn_and_split "movsi_from_sf"
 		 fpstore,    fpstore,     fpstore,     mftgpr,   fp,
 		 mffgpr")
    (set_attr "length"
-		"4,          4,           4,           4,        4,
-		 4,          4,           4,           8,        4,
-		 4")
+		"*,          *,           *,           *,        *,
+		 *,          *,           *,           8,        *,
+		 *")
    (set_attr "isa"
 		"*,          *,           p8v,         p8v,      *,
 		 *,          p9v,         p8v,         p8v,      p8v,
@@ -7049,8 +7049,8 @@  (define_insn_and_split "*movdi_from_sf_z
 		"*,          load,        fpload,      fpload,   two,
 		 two,        mffgpr")
    (set_attr "length"
-		"4,          4,           4,           4,        8,
-		 8,          4")
+		"*,          *,           *,           *,        8,
+		 8,          *")
    (set_attr "isa"
 		"*,          *,           p8v,         p8v,      p8v,
 		 p9v,        p8v")])
@@ -7178,9 +7178,9 @@  (define_insn "*mov<mode>_internal"
 		 vecsimple, vecperm,   vecperm,   vecperm,   vecperm,   mftgpr,
 		 mffgpr,    mfjmpr,    mtjmpr,    *")
    (set_attr "length"
-		"4,         4,         4,         4,         4,         4,
-		 4,         4,         4,         4,         8,         4,
-		 4,         4,         4,         4")
+		"*,         *,         *,         *,         *,         *,
+		 *,         *,         *,         *,         8,         *,
+		 *,         *,         *,         *")
    (set_attr "isa"
 		"*,         *,         p9v,       *,         p9v,       *,
 		 p9v,       p9v,       p9v,       p9v,       p9v,       p9v,
@@ -7231,7 +7231,7 @@  (define_insn "*movcc_internal1"
       (const_string "mtjmpr")
       (const_string "load")
       (const_string "store")])
-   (set_attr "length" "4,4,12,4,4,8,4,4,4,4,4,4")])
+   (set_attr "length" "*,*,12,*,*,8,*,*,*,*,*,*")])
 
 ;; For floating-point, we normally deal with the floating-point registers
 ;; unless -msoft-float is used.  The sole exception is that parameter passing
@@ -7385,8 +7385,8 @@  (define_insn "*mov<mode>_softfloat"
          *,          *,         *,         *")
 
    (set_attr "length"
-	"4,          4,         4,         4,         4,         4,
-         4,          4,         8,         4")])
+	"*,          *,         *,         *,         *,         *,
+         *,          *,         8,         *")])
 
 ;; Like movsf, but adjust a SI value to be used in a SF context, i.e.
 ;; (set (reg:SF ...) (subreg:SF (reg:SI ...) 0))
@@ -7448,8 +7448,8 @@  (define_insn_and_split "movsf_from_si"
   DONE;
 }
   [(set_attr "length"
-	    "4,          4,         4,         4,         4,         4,
-	     4,          12,        4,         4")
+	    "*,          *,         *,         *,         *,         *,
+	     *,          12,        *,         *")
    (set_attr "type"
 	    "load,       fpload,    fpload,    fpload,    store,     fpstore,
 	     fpstore,    vecfloat,  mffgpr,    *")
@@ -7586,8 +7586,8 @@  (define_insn "*mov<mode>_hardfloat32"
              store,       load,       two")
    (set_attr "size" "64")
    (set_attr "length"
-            "4,           4,          4,          4,          4,
-             4,           4,          4,          4,          8,
+            "*,           *,          *,          *,          *,
+             *,           *,          *,          *,          8,
              8,           8,          8")
    (set_attr "isa"
             "*,           *,          *,          p9v,        p9v,
@@ -7696,8 +7696,8 @@  (define_insn "*mov<mode>_softfloat64"
              *,       *,      *")
 
    (set_attr "length"
-            "4,       4,      4,      4,      4,      8,
-             12,      16,     4")])
+            "*,       *,      *,      *,      *,      8,
+             12,      16,     *")])
 
 (define_expand "mov<mode>"
   [(set (match_operand:FMOVE128 0 "general_operand")
@@ -8760,10 +8760,10 @@  (define_insn "*movdi_internal32"
           vecsimple")
    (set_attr "size" "64")
    (set_attr "length"
-         "8,         8,         8,         4,         4,         4,
-          16,        4,         4,         4,         4,         4,
-          4,         4,         4,         4,         4,         8,
-          4")
+         "8,         8,         8,         *,         *,         *,
+          16,        *,         *,         *,         *,         *,
+          *,         *,         *,         *,         *,         8,
+          *")
    (set_attr "isa"
          "*,         *,         *,         *,         *,         *,
           *,         p9v,       p7v,       p9v,       p7v,       *,
@@ -8853,11 +8853,11 @@  (define_insn "*movdi_internal64"
                 mftgpr,    mffgpr")
    (set_attr "size" "64")
    (set_attr "length"
-               "4,         4,         4,         4,         4,          20,
-                4,         4,         4,         4,         4,          4,
-                4,         4,         4,         4,         4,          4,
-                4,         8,         4,         4,         4,          4,
-                4,         4")
+               "*,         *,         *,         *,         *,          20,
+                *,         *,         *,         *,         *,          *,
+                *,         *,         *,         *,         *,          *,
+                *,         8,         *,         *,         *,          *,
+                *,         *")
    (set_attr "isa"
                "*,         *,         *,         *,         *,          *,
                 *,         *,         *,         p9v,       p7v,        p9v,
Index: gcc/config/rs6000/vsx.md
===================================================================
--- gcc/config/rs6000/vsx.md	(revision 272714)
+++ gcc/config/rs6000/vsx.md	(working copy)
@@ -923,7 +923,7 @@  (define_insn "*vsx_le_permute_<mode>"
    mr %0,%L1\;mr %L0,%1
    ld%U1%X1 %0,%L1\;ld%U1%X1 %L0,%1
    std%U0%X0 %L1,%0\;std%U0%X0 %1,%L0"
-  [(set_attr "length" "4,4,4,8,8,8")
+  [(set_attr "length" "*,*,*,8,8,8")
    (set_attr "type" "vecperm,vecload,vecstore,*,load,store")])
 
 (define_insn_and_split "*vsx_le_undo_permute_<mode>"
@@ -1150,9 +1150,9 @@  (define_insn "vsx_mov<mode>_64bit"
                 store,     load,      store,     *,         vecsimple, vecsimple,
                 vecsimple, *,         *,         vecstore,  vecload")
    (set_attr "length"
-               "4,         4,         4,         8,         4,         8,
-                8,         8,         8,         8,         4,         4,
-                4,         20,        8,         4,         4")
+               "*,         *,         *,         8,         *,         8,
+                8,         8,         8,         8,         *,         *,
+                *,         20,        8,         *,         *")
    (set_attr "isa"
                "<VSisa>,   <VSisa>,   <VSisa>,   *,         *,         *,
                 *,         *,         *,         *,         p9v,       *,
@@ -1183,9 +1183,9 @@  (define_insn "*vsx_mov<mode>_32bit"
                 vecsimple, vecsimple, vecsimple, *,         *,
                 vecstore,  vecload")
    (set_attr "length"
-               "4,         4,         4,         16,        16,        16,
-                4,         4,         4,         20,        16,
-                4,         4")
+               "*,         *,         *,         16,        16,        16,
+                *,         *,         *,         20,        16,
+                *,         *")
    (set_attr "isa"
                "<VSisa>,   <VSisa>,   <VSisa>,   *,         *,         *,
                 p9v,       *,         <VSisa>,   *,         *,
@@ -4112,7 +4112,7 @@  (define_insn_and_split "vsx_splat_v4sf"
 		      (const_int 0)] UNSPEC_VSX_XXSPLTW))]
   ""
   [(set_attr "type" "vecload,vecperm,mftgpr")
-   (set_attr "length" "4,8,4")
+   (set_attr "length" "*,8,*")
    (set_attr "isa" "*,p8v,*")])
 
 ;; V4SF/V4SI splat from a vector element