On Tue, Oct 01, 2019 at 06:56:01PM -0500, Segher Boessenkool wrote:
> 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).
No right now we need one (no prefix) and the other (no pcrel) is desirable, but
if we can only have one, it will mean extra instructions being generated.
In the case of no prefix, we need this for stack_protect_testdi and
stack_protect_setdi. There if we have a large stack frame and enable stack
protection, we don't want the register allocator from re-combining the insns to
an insn with a large offset, which it will do, even though the predicate does
not allow this case. This was discovered when we tried to build glibc, and one
module (vfwprintf-internal.c) has a large stack frame and is built with
-fstack-protector-all.
In the case of no pc-rel, this occurs in optimizing vector extracts where the
vector is in memory. In this code, we only have one temporary base register.
In the case where the address is PC-relative, and the element number being
extracted is variable, we would need two temporary base registers, one to hold
the PC-relative address, and the other to hold the offset from the start of the
vector. So here, we disallow PC-relative addresses, but numeric addresses that
result in a prefixed instruction are fine, since the code calculates the
offset, adds in the offset, and then does the memory operation.
Aaron found this bug where it previously tried to use the single temporary with
two different uses.
If we only had a no prefixed constraint, the code would not combine the vector
extract from memory, and instead, it would load the whole vector into a
register, and then do the appropriate shifts, VSLO, etc. to extract the
element. I imagine the case shows up when you have large stack frames (or very
large structures).
> 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".
Yes, I've been thinking that for some time. But I'm not going to worry about
that until the patches are in.
> 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.
Well, wY does allow prefixed addresses (because it calls mem_operand_ds_form
which also was modified to support prefixed addresses), so it wouldn't help.
> > 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?
No, it needs to be part of this patch. It was just missing from the patch I
sent out.
> > + /* 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?
Just trying to do the test before the call, so the default case (not 'future')
won't have the call/return.
> > +;; 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?
Again, I'm trying not to have the extra call/return in the pipeline.
> What is INSN_FORM_BAD about, in both functions?
Because I think it is a bad idea to return true in the case where the memory is
not correct (for example, if somebody created an address with 35-bit offsets,
the address_to_insn_form would return INSN_FORM_BAD, but you would get a false
positive if non_pcrel_memory returned true for it).
> > +;; 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?
Again, just micro-optimizing.
> > + bool pcrel_p = TARGET_PCREL && pcrel_local_address (addr, Pmode);
>
> Similar as above: should TARGET_PCREL be part of pcrel_local_address?
Sure it can.
> > @@ -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?
If you have:
static vector double v;
double
get_v_n (size_t n)
{
return vec_extract (v, n);
}
It calls rs6000_split_vec_extract_var with:
dest = (mem:V2DF (SYMBOL_REF:DImode "v"))
element = (reg:DI <xxx>)
tmp_gpr = (reg:DI <yyy>)
tmp_altivec = (scratch:V2DF)
So to turn this into a (MEM:DF ...) to access the element, it needs to
calculate the offset from the beginning of the vector into tmp_gpr. But since
the hardware doesn't allow indexed PC-relative instructions, we have to load
the address into a register to do an indexed load.
Because that combination is not allowed, the compiler then loads up the address
into a temporary register, and calls rs6000_split_vec_extract_var with:
(MEM:V2DF (reg:DI <zzz>))
I.e.
rldicl 3,3,0,63
pla 9,.LANCHOR0@pcrel
sldi 10,3,3
lfdx 1,9,10
> > + /* 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?
Probably it should be (or remove the tests elsewhere).
> > @@ -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.
Ok.
> > +/* 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.
Ok.
> > --- 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?
As far as I can tell it was wrong. But I can add 4 to the values if you think
it might be needed.
> > @@ -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?
Dunno. The original code just used 8, which is wrong. I was trying to guess
what the maximum # of insns to load up a 128-bit value in two GPRs was. I can
certainly use 8 like we have now. Fortunately for the time it matters, it will
have been split and then the normal counts for loading up a single register
will be correct.
> > -(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.
For the define_expand, you need it to be more general, so that you can call
make_memory_non_prefixed.
> > @@ -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?
I thought it was P8 code, not P9. On P8, you would have to do two MTVSRD's and
a combine insn, which you can't express in a normal move.
>
> > +
> > + (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).
See above. If is is a separate patch, it will at least need to have the
gcc_unreachable's to flag when it is used.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797
