[PATCH], V4, patch #4.1: Enable prefixed/pc-rel addressing (revised)
Segher Boessenkool
segher@kernel.crashing.org
Tue Oct 1 23:56:00 GMT 2019
Hi Mike,
On Mon, Sep 30, 2019 at 10:12:54AM -0400, Michael Meissner wrote:
> I needed
> to add a second memory constraint ('eM') that prevents using a prefixed
> address.
Do we need both em and eM? Do we ever want to allow prefixed insns but not
pcrel? Or, alternatively, this only uses eM for some insns where the "p"
prefix trick won't work (because there are multiple insns in the template);
we could solve that some other way (by inserting the p's manually for
example).
But what should inline asm users do wrt prefixed/pcrel memory? Should
they not use prefixed memory at all? That means for asm we should always
disallow prefixed for "m".
Having both em and eM names is a bit confusing (which is what?) The eM
one should not be documented in the user manual, probably.
Maybe just using wY here will work just as well? That is also not ideal
of course, but we already have that one anyway.
> 4) In the previous patch, I missed setting the prefixed size and non prefixed
> size for mov<mode>_ppc64 in the insn. This pattern is used for moving PTImode
> in GPR registers (on non-VSX systems, it would move TImode also). By the time
> it gets to final, it will have been split, but it is still useful to get the
> sizes correct before the mode is split.
So that is a separate patch? Please send it as one, then?
> + /* The LWA instruction uses the DS-form format where the bottom two bits of
> + the offset must be 0. The prefixed PLWA does not have this
> + restriction. */
> + if (TARGET_PREFIXED_ADDR
> + && address_is_prefixed (addr, DImode, NON_PREFIXED_DS))
> + return true;
Should TARGET_PREFIXED_ADDR be part of address_is_prefixed, instead of
part of all its callers?
> +;; Return 1 if op is a memory operand that is not prefixed.
> +(define_predicate "non_prefixed_memory"
> + (match_code "mem")
> +{
> + if (!memory_operand (op, mode))
> + return false;
> +
> + enum insn_form iform
> + = address_to_insn_form (XEXP (op, 0), mode, NON_PREFIXED_DEFAULT);
> +
> + return (iform != INSN_FORM_PREFIXED_NUMERIC
> + && iform != INSN_FORM_PCREL_LOCAL
> + && iform != INSN_FORM_BAD);
> +})
> +
> +(define_predicate "non_pcrel_memory"
> + (match_code "mem")
> +{
> + if (!memory_operand (op, mode))
> + return false;
> +
> + enum insn_form iform
> + = address_to_insn_form (XEXP (op, 0), mode, NON_PREFIXED_DEFAULT);
> +
> + return (iform != INSN_FORM_PCREL_EXTERNAL
> + && iform != INSN_FORM_PCREL_LOCAL
> + && iform != INSN_FORM_BAD);
> +})
Why does non_prefixed_memory not check INSN_FORM_PCREL_EXTERNAL? Why does
non_prefixed_memory not use non_pcrel_memory, instead of open-coding it?
What is INSN_FORM_BAD about, in both functions?
> +;; Return 1 if op is either a register operand or a memory operand that does
> +;; not use a PC-relative address.
> +(define_predicate "reg_or_non_pcrel_memory"
> + (match_code "reg,subreg,mem")
> +{
> + if (REG_P (op) || SUBREG_P (op))
> + return register_operand (op, mode);
> +
> + return non_pcrel_memory (op, mode);
> +})
Why do we need this predicate? Should it use register_operand like this,
or should it use gpc_reg_operand?
> + bool pcrel_p = TARGET_PCREL && pcrel_local_address (addr, Pmode);
Similar as above: should TARGET_PCREL be part of pcrel_local_address?
> @@ -6860,6 +6904,12 @@ rs6000_split_vec_extract_var (rtx dest,
> systems. */
> if (MEM_P (src))
> {
> + /* If this is a PC-relative address, we would need another register to
> + hold the address of the vector along with the variable offset. The
> + callers should use reg_or_non_pcrel_memory to make sure we don't
> + get a PC-relative address here. */
I don't understand this comment, nor the problem. Please expand?
> + /* Allow prefixed instructions if supported. If the bottom two bits of the
> + offset are non-zero, we could use a prefixed instruction (which does not
> + have the DS-form constraint that the traditional instruction had) instead
> + of forcing the unaligned offset to a GPR. */
> + if (address_is_prefixed (addr, mode, NON_PREFIXED_DS))
> + return true;
Here (and for DQ) you aren't testing TARGET_PREFIXED?
> @@ -7371,7 +7435,7 @@ mem_operand_gpr (rtx op, machine_mode mo
> causes a wrap, so test only the low 16 bits. */
> offset = ((offset & 0xffff) ^ 0x8000) - 0x8000;
>
> - return offset + 0x8000 < 0x10000u - extra;
> + return SIGNED_16BIT_OFFSET_EXTRA_P (offset, extra);
This is a separate patch (and pre-approved).
> @@ -7404,7 +7475,7 @@ mem_operand_ds_form (rtx op, machine_mod
> causes a wrap, so test only the low 16 bits. */
> offset = ((offset & 0xffff) ^ 0x8000) - 0x8000;
>
> - return offset + 0x8000 < 0x10000u - extra;
> + return SIGNED_16BIT_OFFSET_EXTRA_P (offset, extra);
Together with this.
> - offset += 0x8000;
> - return offset < 0x10000 - extra;
> + if (TARGET_PREFIXED_ADDR)
> + return SIGNED_34BIT_OFFSET_EXTRA_P (offset, extra);
> + else
> + return SIGNED_16BIT_OFFSET_EXTRA_P (offset, extra);
And the 16-bit part of this.
> - unsigned HOST_WIDE_INT val = INTVAL (XEXP (addr, 1));
> - return val + 0x8000 >= 0x10000 - (TARGET_POWERPC64 ? 8 : 12);
> + HOST_WIDE_INT val = INTVAL (XEXP (addr, 1));
> + HOST_WIDE_INT extra = TARGET_POWERPC64 ? 8 : 12;
> + if (TARGET_PREFIXED_ADDR)
> + return !SIGNED_34BIT_OFFSET_EXTRA_P (val, extra);
> + else
> + return !SIGNED_16BIT_OFFSET_EXTRA_P (val, extra);
And this.
> +/* 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)
Separate patch please. This is a whole different issue.
> --- gcc/config/rs6000/rs6000.md (revision 276284)
> +++ gcc/config/rs6000/rs6000.md (working copy)
> @@ -7774,8 +7774,9 @@ (define_insn_and_split "*mov<mode>_64bit
> "&& reload_completed"
> [(pc)]
> { rs6000_split_multireg_move (operands[0], operands[1]); DONE; }
> - [(set_attr "length" "8")
> - (set_attr "isa" "*,*,*,*,*,*,*,*,p8v,p8v")])
> + [(set_attr "isa" "*,*,*,*,*,*,*,*,p8v,p8v")
> + (set_attr "non_prefixed_length" "8")
> + (set_attr "prefixed_length" "20")])
This one is fine.
> @@ -7786,8 +7787,12 @@ (define_insn_and_split "*movtd_64bit_nod
> "#"
> "&& reload_completed"
> [(pc)]
> -{ rs6000_split_multireg_move (operands[0], operands[1]); DONE; }
> - [(set_attr "length" "8,8,8,12,12,8")])
> +{
> + rs6000_split_multireg_move (operands[0], operands[1]);
> + DONE;
> +}
> + [(set_attr "non_prefixed_length" "8")
> + (set_attr "prefixed_length" "20")])
But this one I don't get... Various alternatives were 12 before, what
changed? Or was it wrong?
> @@ -8984,7 +8989,8 @@ (define_insn "*mov<mode>_ppc64"
> return rs6000_output_move_128bit (operands);
> }
> [(set_attr "type" "store,store,load,load,*,*")
> - (set_attr "length" "8")])
> + (set_attr "non_prefixed_length" "8,8,8,8,8,40")
> + (set_attr "prefixed_length" "20,20,20,20,8,40")])
What about the 40 here?
> -(define_insn "stack_protect_setdi"
> - [(set (match_operand:DI 0 "memory_operand" "=Y")
> - (unspec:DI [(match_operand:DI 1 "memory_operand" "Y")] UNSPEC_SP_SET))
> +(define_expand "stack_protect_setdi"
> + [(parallel [(set (match_operand:DI 0 "memory_operand")
> + (unspec:DI [(match_operand:DI 1 "memory_operand")]
> + UNSPEC_SP_SET))
> + (set (match_scratch:DI 2)
> + (const_int 0))])]
> + "TARGET_64BIT"
> +{
> + if (TARGET_PREFIXED_ADDR)
> + {
> + operands[0] = make_memory_non_prefixed (operands[0]);
> + operands[1] = make_memory_non_prefixed (operands[1]);
> + }
> +})
I don't understand why this is needed... Won't a better predicate do
this easier, safer, simpler? Better than memory_operand, which of course
allows pretty much anything.
> @@ -1149,10 +1149,30 @@ (define_insn "vsx_mov<mode>_64bit"
> "vecstore, vecload, vecsimple, mffgpr, mftgpr, load,
> store, load, store, *, vecsimple, vecsimple,
> vecsimple, *, *, vecstore, vecload")
> - (set_attr "length"
> - "*, *, *, 8, *, 8,
> - 8, 8, 8, 8, *, *,
> - *, 20, 8, *, *")
> + (set (attr "non_prefixed_length")
> + (cond [(and (eq_attr "alternative" "4") ;; MTVSRDD
> + (match_test "TARGET_P9_VECTOR"))
> + (const_string "4")
> +
> + (eq_attr "alternative" "3,4") ;; GPR <-> VSX
> + (const_string "8")
TARGET_P9_VECTOR is always true for alt 4 (it uses the "we" constraint).
And exactly the same is true for alt 3? And MTVSRDD has nothing to do
with it?
> +
> + (eq_attr "alternative" "5,6,7,8") ;; GPR load/store
> + (const_string "8")]
> + (const_string "*")))
This loses that alt 13 was len 20, what happened there? And alts 9 and 14?
> @@ -3235,9 +3255,10 @@ (define_insn "vsx_vslo_<mode>"
> ;; Variable V2DI/V2DF extract
> (define_insn_and_split "vsx_extract_<mode>_var"
> [(set (match_operand:<VS_scalar> 0 "gpc_reg_operand" "=v,wa,r")
> - (unspec:<VS_scalar> [(match_operand:VSX_D 1 "input_operand" "v,m,m")
> - (match_operand:DI 2 "gpc_reg_operand" "r,r,r")]
> - UNSPEC_VSX_EXTRACT))
> + (unspec:<VS_scalar>
> + [(match_operand:VSX_D 1 "reg_or_non_pcrel_memory" "v,em,em")
> + (match_operand:DI 2 "gpc_reg_operand" "r,r,r")]
> + UNSPEC_VSX_EXTRACT))
Please explain why this needs to be non-pcrel (and split this to a separate
patch if possible).
Segher
More information about the Gcc-patches
mailing list