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
