This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, V3, #4 of 10], Add general prefixed/pcrel support
Hi!
(Please split off paddi to a separate patch?)
On Mon, Aug 26, 2019 at 04:43:37PM -0400, Michael Meissner wrote:
> (prefixed_paddi_p): Fix thinkos in last patch.
Do that separately please. Don't hide this in another patch like this.
Hrm, this is not in this patch at all? Fix the changelog, then :-)
> --- gcc/config/rs6000/predicates.md (revision 274870)
> +++ gcc/config/rs6000/predicates.md (working copy)
> @@ -839,7 +839,8 @@ (define_special_predicate "indexed_addre
> (define_predicate "add_operand"
> (if_then_else (match_code "const_int")
> (match_test "satisfies_constraint_I (op)
> - || satisfies_constraint_L (op)")
> + || satisfies_constraint_L (op)
> + || satisfies_constraint_eI (op)")
> (match_operand 0 "gpc_reg_operand")))
>
> ;; Return 1 if the operand is either a non-special register, or 0, or -1.
> @@ -852,7 +853,8 @@ (define_predicate "adde_operand"
> (define_predicate "non_add_cint_operand"
> (and (match_code "const_int")
> (match_test "!satisfies_constraint_I (op)
> - && !satisfies_constraint_L (op)")))
> + && !satisfies_constraint_L (op)
> + && !satisfies_constraint_eI (op)")))
(define_predicate "non_add_cint_operand"
(and (match_code "const_int")
(not (match_operand 0 "add_operand"))))
? You can do that *now*, and it is pre-approved. (This could use a better
name btw., I always have to look up what it means; a longer name is fine as
well of course, it is used only once or so).
> @@ -933,6 +935,13 @@ (define_predicate "lwa_operand"
> return false;
>
> addr = XEXP (inner, 0);
> +
> + /* 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 (prefixed_local_addr_p (addr, mode, TRAD_INSN_DS))
> + return true;
Why does the decision whether something is a valid prefixed lwa_operand
need to know the non-prefixed lwa is a DS-form instruction?
And "local" is a head-scratcher for this condition, too.
> +;; Return 1 if op is a memory operand that is not prefixed.
> +(define_predicate "non_prefixed_mem_operand"
> + (match_code "mem")
> +{
> + if (!memory_operand (op, mode))
> + return false;
> +
> + return !prefixed_local_addr_p (XEXP (op, 0), GET_MODE (op),
> + TRAD_INSN_DEFAULT);
> +})
Use match_operand for the first condition please (and then match_test for
the second?)
This does make it seem like we need a prefixed_local_mem_p as well? So
that we need neither that XEXP nor that GET_MODE.
> @@ -5735,6 +5735,10 @@ num_insns_constant_gpr (HOST_WIDE_INT va
> && (value >> 31 == -1 || value >> 31 == 0))
> return 1;
>
> + /* PADDI can support up to 34 bit signed integers. */
> + else if (TARGET_PREFIXED_ADDR && SIGNED_34BIT_OFFSET_P (value))
> + return 1;
Write this earlier, together with the 16BIT one?
> @@ -6905,6 +6909,7 @@ rs6000_adjust_vec_address (rtx scalar_re
> rtx element_offset;
> rtx new_addr;
> bool valid_addr_p;
> + bool pcrel_p = TARGET_PCREL && pcrel_local_address (addr, Pmode);
This is used 159 lines later. Please refactor things. That would make
a separate patch *before* this one.
> + /* Optimize pc-relative addresses. */
> + else if (pcrel_p)
> + {
> + if (CONST_INT_P (element_offset))
> + {
> + rtx addr2 = addr;
This var needs a better name and/or comments. Or maybe just factoring.
> @@ -7007,9 +7050,8 @@ rs6000_adjust_vec_address (rtx scalar_re
>
> /* If we have a PLUS, we need to see whether the particular register class
> allows for D-FORM or X-FORM addressing. */
> - if (GET_CODE (new_addr) == PLUS)
> + if (GET_CODE (new_addr) == PLUS || pcrel_p)
That second condition needs a comment.
> @@ -7609,7 +7675,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);
Please do all these things first too, as a separate patch.
> - 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);
So this you could just do the 16BIT first, and then *this* patch will add
the 34BIT thing, in an easy-to-read patch.
> + {
> + /* There is no prefixed version of the load/store with update. */
> + return !prefixed_local_addr_p (XEXP (x, 1), mode, TRAD_INSN_DEFAULT);
> + }
If you pass the actual MEM, the prefixed_local_mem_p function can return
false, itself.
> - 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;
The 8 vs. 12 could use a comment (yes, I know it was there already). Do you
know what this is about, why it is 8 and 12?
> +/* Make a memory address non-prefixed if it is prefixed. */
"Return an RTX that is like MEM but does not need prefixed instructions
to access."?
> +rtx
> +make_memory_non_prefixed (rtx mem)
> +{
> + gcc_assert (MEM_P (mem));
> + if (prefixed_local_addr_p (XEXP (mem, 0), GET_MODE (mem), TRAD_INSN_DEFAULT))
Swap the condition and do an early-out, please.
> + {
> + rtx old_addr = XEXP (mem, 0);
> + rtx new_addr;
You you also need to strip CONST from around the address here?
> @@ -21060,7 +21168,8 @@ rs6000_rtx_costs (rtx x, machine_mode mo
> || outer_code == PLUS
> || outer_code == MINUS)
> && (satisfies_constraint_I (x)
> - || satisfies_constraint_L (x)))
> + || satisfies_constraint_L (x)
> + || satisfies_constraint_eI (x)))
Just use add_operand here, maybe?
OTOH, do we want to count prefixed insns as not more expensive than
prefixed ones?
> +/* How many real instructions are generated for this insn? This is slightly
"How many machine instructions are generated for INSN".
> +static int
> +rs6000_num_insns (rtx_insn *insn)
> +{
> + /* 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. */
> + return 2 + (length - 20) / 4;
> + }
> +
> + return length / 4;
> +}
Yuck. It only needs an approximate answer, but why then handle all kinds
of cases that you cannot test (because they do not currently happen)?
Instead, handle prefixed insns one step up, in insn_cost itself? It
knows more, it can make better estimates. It can do it per instruction
type, importantly.
> (define_insn "*add<mode>3"
> - [(set (match_operand:GPR 0 "gpc_reg_operand" "=r,r,r")
> - (plus:GPR (match_operand:GPR 1 "gpc_reg_operand" "%r,b,b")
> - (match_operand:GPR 2 "add_operand" "r,I,L")))]
> + [(set (match_operand:GPR 0 "gpc_reg_operand" "=r,r,r,r")
> + (plus:GPR (match_operand:GPR 1 "gpc_reg_operand" "%r,b,b,b")
> + (match_operand:GPR 2 "add_operand" "r,I,L,eI")))]
> ""
> "@
> add %0,%1,%2
> addi %0,%1,%2
> - addis %0,%1,%v2"
> - [(set_attr "type" "add")])
> + addis %0,%1,%v2
> + addi %0,%1,%2"
> + [(set_attr "type" "add")
> + (set_attr "isa" "*,*,*,fut")])
Okay.
> @@ -6909,22 +6911,22 @@ (define_insn "movsi_low"
>
> ;; MR LA LWZ LFIWZX LXSIWZX
> ;; STW STFIWX STXSIWX LI LIS
> -;; # XXLOR XXSPLTIB 0 XXSPLTIB -1 VSPLTISW
> -;; XXLXOR 0 XXLORC -1 P9 const MTVSRWZ MFVSRWZ
> -;; MF%1 MT%0 NOP
> +;; PLI # XXLOR XXSPLTIB 0 XXSPLTIB -1
> +;; VSPLTISW XXLXOR 0 XXLORC -1 P9 const MTVSRWZ
> +;; MFVSRWZ MF%1 MT%0 NOP
So this is adding just the PLI? Put it on a line of its own then? And
don't reformat the existing stuff.
It would be nice if this all can be formatted a bit nicer eventually, in
more logical groups, not strictly five per line, which isn't need for
anything and not helpful at all either. So for this maybe
mr la
lwz lfiwzx lxsiwzx
stw stfiwx stxsiwx
li lis pli #
etc. You also have the restriction that the order matters somewhat, but
there still is a lot of room to make it easier to read.
> -;; Split a load of a large constant into the appropriate two-insn
> -;; sequence.
> +;; Split a load of a large constant into the appropriate two-insn sequence. On
> +;; systems that support PADDI (PLI), we can use PLI to load any 32-bit constant
> +;; in one instruction.
>
> (define_split
> [(set (match_operand:SI 0 "gpc_reg_operand")
> (match_operand:SI 1 "const_int_operand"))]
> "(unsigned HOST_WIDE_INT) (INTVAL (operands[1]) + 0x8000) >= 0x10000
Use the 16BIT thing here?
> @@ -7769,9 +7782,13 @@ (define_insn_and_split "*mov<mode>_64bit
> "#"
> "&& reload_completed"
> [(pc)]
> -{ rs6000_split_multireg_move (operands[0], operands[1]); DONE; }
> - [(set_attr "length" "8,8,8,8,12,12,8,8,8")
> - (set_attr "isa" "*,*,*,*,*,*,*,p8v,p8v")])
> +{
> + rs6000_split_multireg_move (operands[0], operands[1]);
> + DONE;
> +}
> + [(set_attr "isa" "*,*,*,*,*,*,*,*,p8v,p8v")
> + (set_attr "non_prefixed_length" "8")
> + (set_attr "prefixed_length" "20")])
Should this have a separate alternative for prefixed addressing?
What happened to the 12's?
> @@ -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")
> +
> + (eq_attr "alternative" "5,6,7,8") ;; GPR load/store
> + (const_string "8")]
> + (const_string "*")))
Why handle alternative 4 separately like this? Shouldn't there just be
a separate alternative for the p9 version?
Segher