This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] V6, #1 of 17: Use ADJUST_INSN_LENGTH for prefixed instructions
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.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797