V6, #1 of 17: Use ADJUST_INSN_LENGTH for prefixed instructions

This patch uses the target hook ADJUST_INSN_LENGTH to change the length of
instructions that contain prefixed memory/add instructions.

There are 2 new insn attributes:

1) num_insns: If non-zero, returns the number of machine instructions in an
insn.  This simplifies the calculations in rs6000_insn_cost.

2) max_prefixed_insns: Returns the maximum number of prefixed instructions in
an insn.  Normally this is 1, but in the insns that load up 128-bit values into
GPRs, it will be 2.

This patch replaces patches #2, #3, and #6 from the V5 patch series.

Along with the other patches, I have done bootstraps on a little endian power8
system, and there were no regressions in the test suite.  I have built both
Spec 2006 and Spec 2017 with all of these patches installed using -mcpu=future,
and there were no failures.  Can I check this into the trunk?

Note, I may have limited email access on October 17th and 18th, 2019.

2019-10-15  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000.c (rs6000_insn_cost): Use num_insns insn
	attribute if it exists, rather than the insn size.  If we use the
	insn size, adjust the size to remove the extra size that prefixed
	instructions take.
	* config/rs6000/rs6000.h (ADJUST_INSN_LENGTH): New target hook to
	update the instruction sized if prefixed instructions are used.
	* config/rs6000/rs6000.md (prefixed_length attribute): Delete.
	(non_prefixed_length attribute): Delete.
	(num_insns attribute): New insn attribute to return the number of
	instructions.
	(max_prefixed_insns attribute): New insn attribute to return the
	maximum number of prefixed instructions in an insn.
	(length attribute): Do not adjust for prefix instructions here,
	punt to ADJUST_INSN_LENGTH.
	(mov<mode>_64bit): Set max_prefixed_insns and num_insns.
	(movtd_64bit_nodm): Set max_prefixed_insns and num_insns.
	(mov<mode>_ppc64): Set max_prefixed_insns and num_insns.
	* config/rs6000/vsx.md: (vsx_mov<mode>_64bit): Set
	max_prefixed_insns and num_insns.

Hi!

On Wed, Oct 16, 2019 at 09:35:33AM -0400, Michael Meissner wrote:
> This patch uses the target hook ADJUST_INSN_LENGTH to change the length of
> instructions that contain prefixed memory/add instructions.

That made this amazingly hard to review.  But it might well be worth it,
thankfully :-)

> There are 2 new insn attributes:
> 
> 1) num_insns: If non-zero, returns the number of machine instructions in an
> insn.  This simplifies the calculations in rs6000_insn_cost.

This is great.

> 2) max_prefixed_insns: Returns the maximum number of prefixed instructions in
> an insn.  Normally this is 1, but in the insns that load up 128-bit values into
> GPRs, it will be 2.

This one, I am not so sure.

> -  int n = get_attr_length (insn) / 4;
> +  /* If the insn tells us how many insns there are, use that.  Otherwise use
> +     the length/4.  Adjust the insn length to remove the extra size that
> +     prefixed instructions take.  */

This should be temporary, until we have converted everything to use
num_insns, right?

> +  int n = get_attr_num_insns (insn);
> +  if (n == 0)
> +    {
> +      int length = get_attr_length (insn);
> +      if (get_attr_prefixed (insn) == PREFIXED_YES)
> +	{
> +	  int adjust = 0;
> +	  ADJUST_INSN_LENGTH (insn, adjust);
> +	  length -= adjust;
> +	}
> +
> +      n = length / 4;
> +    }

> --- gcc/config/rs6000/rs6000.h	(revision 277017)
> +++ gcc/config/rs6000/rs6000.h	(working copy)
> @@ -1847,9 +1847,30 @@ extern scalar_int_mode rs6000_pmode;
>  /* Adjust the length of an INSN.  LENGTH is the currently-computed length and
>     should be adjusted to reflect any required changes.  This macro is used when
>     there is some systematic length adjustment required that would be difficult
> -   to express in the length attribute.  */
> +   to express in the length attribute.
>  
> -/* #define ADJUST_INSN_LENGTH(X,LENGTH) */
> +   In the PowerPC, we use this to adjust the length of an instruction if one or
> +   more prefixed instructions are generated, using the attribute
> +   num_prefixed_insns.  A prefixed instruction is 8 bytes instead of 4, but the
> +   hardware requires that a prefied instruciton not cross a 64-byte boundary.

"prefixed instruction does not"

> +   This means the compiler has to assume the length of the first prefixed
> +   instruction is 12 bytes instead of 8 bytes.  Since the length is already set
> +   for the non-prefixed instruction, we just need to udpate for the
> +   difference.  */
> +
> +#define ADJUST_INSN_LENGTH(INSN,LENGTH)					\
> +{									\
> +  if (NONJUMP_INSN_P (INSN))						\
> +    {									\
> +      rtx pattern = PATTERN (INSN);					\
> +      if (GET_CODE (pattern) != USE && GET_CODE (pattern) != CLOBBER	\
> +	  && get_attr_prefixed (INSN) == PREFIXED_YES)			\
> +	{								\
> +	  int num_prefixed = get_attr_max_prefixed_insns (INSN);	\
> +	  (LENGTH) += 4 * (num_prefixed + 1);				\
> +	}								\
> +    }									\
> +}

Please use a function, not a function-like macro.

So this computes the *maximum* RTL instruction length, not considering how
many of the machine insns in it need a prefix insn.  Can't we do better?
Hrm, I guess in all cases that matter we will split early anyway.


> +;; Return the number of real hardware instructions in a combined insn.  If it
> +;; is 0, just use the length / 4.
> +(define_attr "num_insns" "" (const_int 0))

So we could have the default value *be* length/4, not 0?

> +;; If an insn is prefixed, return the maximum number of prefixed instructions
> +;; in the insn.  The macro ADJUST_INSN_LENGTH uses this number to adjust the
> +;; insn length.
> +(define_attr "max_prefixed_insns" "" (const_int 1))

"maximum number of prefixed machine instructions in the RTL instruction".

> +;; Length of the instruction (in bytes).  This length does not consider the
> +;; length for prefixed instructions.  The macro ADJUST_INSN_LENGTH will adjust
> +;; the length if there are prefixed instructions.

That is not what it does...  it uses the maximum number of prefixed insns
there could be.

> +;; While it might be tempting to use num_insns to calculate the length, it can
> +;; be problematical unless all insn lengths are adjusted to use num_insns
> +;; (i.e. if num_insns is 0, it will get the length, which in turn will get
> +;; num_insns and recurse).
> +(define_attr "length" "" (const_int 4))

Yes, and not only is it tempting, it is what we are going to do!  Right?
:-)  Just not just yet.

So please use a function for ADJUST_INSN_LENGTH.  Okay for trunk like that.
Thanks!  And sorry this took so long.


Segher

On Tue, Oct 22, 2019 at 05:27:19PM -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Wed, Oct 16, 2019 at 09:35:33AM -0400, Michael Meissner wrote:
> > This patch uses the target hook ADJUST_INSN_LENGTH to change the length of
> > instructions that contain prefixed memory/add instructions.
> 
> That made this amazingly hard to review.  But it might well be worth it,
> thankfully :-)
> 
> > There are 2 new insn attributes:
> > 
> > 1) num_insns: If non-zero, returns the number of machine instructions in an
> > insn.  This simplifies the calculations in rs6000_insn_cost.
> 
> This is great.
> 
> > 2) max_prefixed_insns: Returns the maximum number of prefixed instructions in
> > an insn.  Normally this is 1, but in the insns that load up 128-bit values into
> > GPRs, it will be 2.
> 
> This one, I am not so sure.

I wanted it to be simple, so in general it was just a constant.  Since the only
user of it has already checked that the insn is prefixed, I didn't think it
needed the prefixed test to set it to 0.

> > -  int n = get_attr_length (insn) / 4;
> > +  /* If the insn tells us how many insns there are, use that.  Otherwise use
> > +     the length/4.  Adjust the insn length to remove the extra size that
> > +     prefixed instructions take.  */
> 
> This should be temporary, until we have converted everything to use
> num_insns, right?

Well there were some 200+ places where length was set.

> > --- gcc/config/rs6000/rs6000.h	(revision 277017)
> > +++ gcc/config/rs6000/rs6000.h	(working copy)
> > @@ -1847,9 +1847,30 @@ extern scalar_int_mode rs6000_pmode;
> >  /* Adjust the length of an INSN.  LENGTH is the currently-computed length and
> >     should be adjusted to reflect any required changes.  This macro is used when
> >     there is some systematic length adjustment required that would be difficult
> > -   to express in the length attribute.  */
> > +   to express in the length attribute.
> >  
> > -/* #define ADJUST_INSN_LENGTH(X,LENGTH) */
> > +   In the PowerPC, we use this to adjust the length of an instruction if one or
> > +   more prefixed instructions are generated, using the attribute
> > +   num_prefixed_insns.  A prefixed instruction is 8 bytes instead of 4, but the
> > +   hardware requires that a prefied instruciton not cross a 64-byte boundary.
> 
> "prefixed instruction does not"

Thanks.

> > +   This means the compiler has to assume the length of the first prefixed
> > +   instruction is 12 bytes instead of 8 bytes.  Since the length is already set
> > +   for the non-prefixed instruction, we just need to udpate for the
> > +   difference.  */
> > +
> > +#define ADJUST_INSN_LENGTH(INSN,LENGTH)					\
> > +{									\
> > +  if (NONJUMP_INSN_P (INSN))						\
> > +    {									\
> > +      rtx pattern = PATTERN (INSN);					\
> > +      if (GET_CODE (pattern) != USE && GET_CODE (pattern) != CLOBBER	\
> > +	  && get_attr_prefixed (INSN) == PREFIXED_YES)			\
> > +	{								\
> > +	  int num_prefixed = get_attr_max_prefixed_insns (INSN);	\
> > +	  (LENGTH) += 4 * (num_prefixed + 1);				\
> > +	}								\
> > +    }									\
> > +}
> 
> Please use a function, not a function-like macro.

Ok, I added rs6000_adjust_insn_length in rs6000.c.

> So this computes the *maximum* RTL instruction length, not considering how
> many of the machine insns in it need a prefix insn.  Can't we do better?
> Hrm, I guess in all cases that matter we will split early anyway.

Well before register allocation for the 128-bit types, you really can't say
what the precise length is, even if it is not prefixed.

And of course even after register allocation, it isn't precise, since the
length of a prefixed instruction is normally 8, but sometimes 12.  So we have
to use 12.

> 
> > +;; Return the number of real hardware instructions in a combined insn.  If it
> > +;; is 0, just use the length / 4.
> > +(define_attr "num_insns" "" (const_int 0))
> 
> So we could have the default value *be* length/4, not 0?

Only if you make sure that every place sets num_insns.  As the comment says,
until it is set every where, you run the risk of a deadly embrace.

> > +;; If an insn is prefixed, return the maximum number of prefixed instructions
> > +;; in the insn.  The macro ADJUST_INSN_LENGTH uses this number to adjust the
> > +;; insn length.
> > +(define_attr "max_prefixed_insns" "" (const_int 1))
> 
> "maximum number of prefixed machine instructions in the RTL instruction".
> 
> So please use a function for ADJUST_INSN_LENGTH.  Okay for trunk like that.
> Thanks!  And sorry this took so long.

Checked in, thanks.

On Wed, Oct 23, 2019 at 05:00:58PM -0400, Michael Meissner wrote:
> On Tue, Oct 22, 2019 at 05:27:19PM -0500, Segher Boessenkool wrote:
> > On Wed, Oct 16, 2019 at 09:35:33AM -0400, Michael Meissner wrote:
> > > -  int n = get_attr_length (insn) / 4;
> > > +  /* If the insn tells us how many insns there are, use that.  Otherwise use
> > > +     the length/4.  Adjust the insn length to remove the extra size that
> > > +     prefixed instructions take.  */
> > 
> > This should be temporary, until we have converted everything to use
> > num_insns, right?
> 
> Well there were some 200+ places where length was set.

Yes, and I did volunteer to do this work, if needed / wanted.

> > Please use a function, not a function-like macro.
> 
> Ok, I added rs6000_adjust_insn_length in rs6000.c.

Thanks.

> > > +;; Return the number of real hardware instructions in a combined insn.  If it
> > > +;; is 0, just use the length / 4.
> > > +(define_attr "num_insns" "" (const_int 0))
> > 
> > So we could have the default value *be* length/4, not 0?
> 
> Only if you make sure that every place sets num_insns.  As the comment says,
> until it is set every where, you run the risk of a deadly embrace.

Sure :-)


Segher

On Wed, Oct 23, 2019 at 04:42:19PM -0500, Segher Boessenkool wrote:
> On Wed, Oct 23, 2019 at 05:00:58PM -0400, Michael Meissner wrote:
> > On Tue, Oct 22, 2019 at 05:27:19PM -0500, Segher Boessenkool wrote:
> > > On Wed, Oct 16, 2019 at 09:35:33AM -0400, Michael Meissner wrote:
> > > > -  int n = get_attr_length (insn) / 4;
> > > > +  /* If the insn tells us how many insns there are, use that.  Otherwise use
> > > > +     the length/4.  Adjust the insn length to remove the extra size that
> > > > +     prefixed instructions take.  */
> > > 
> > > This should be temporary, until we have converted everything to use
> > > num_insns, right?
> > 
> > Well there were some 200+ places where length was set.
> 
> Yes, and I did volunteer to do this work, if needed / wanted.

So I did this, but it does not work: I end up with out-of-range branches.
I rewrote it in a different way: same thing.  And again, and again.

The fundamental problem is that the "length" attribute is special.  It
actually is four attributes:

insn_current_length     (that's the one you expect)
insn_variable_length_p  (true if the length of this insn depends on
                         the (relative) addresses of any (other) insns)
insn_min_length         (minimum length: minimum of the alternatives)
insn_default_length     (which really should be called insn_max_length)

which are used in the shorten pass (which actually makes branches *longer*
in the normal case).  The problem is that these are only computed for static
values that are set in the "length" attribute in the machine description,
while we actually want to set "length" based on some other attributes
("num_insns", "prefixed", "max_prefixed_insns"), either directly or via
adjust_insn_length.

The shorten pass (which lenghtens branches, unless -O0) does not notice
most insns that can become longer, and eventually we ICE because of an
out-of-range branch.

Hrm, maybe if I force insn_variable_length_p to "true".  Let me try that.


Segher