Re: [PATCH], PowerPC, Patch #9, Refine calculation of whether an address offset is d-form, ds-form, or dq-form

[Michael Meissner <meissner@linux.ibm.com>]

On Fri, Jul 12, 2019 at 11:49:51AM -0500, Segher Boessenkool wrote:
> Many of those are not clear why you allow or do not allow certain forms,
> or what each is meant to be used for.  For example:
> 
> > +  enum rs6000_offset_format format_gpr_64bit
> > +    = (TARGET_POWERPC64) ? OFFSET_FORMAT_DS : OFFSET_FORMAT_D;
> 
> Why does non-powerpc64 allow D?  It cannot even do 64-bit GPR accesses
> *at all*!
> 

A single instruction cannot do 64-bit GPR loads, BUT the tests are needed for
register allocation before the instruction is split.  So in 32-bit mode, load
of DImode is split into 2 loads of SImode, which is D-format.

> > +  enum rs6000_offset_format format_gpr_128bit
> > +    = (TARGET_QUAD_MEMORY) ? OFFSET_FORMAT_DQ : format_gpr_64bit;
> 
> 128-bit GPRs do not exist, either.

Yes they do during register allocation, and are split later before final.

> > +  enum rs6000_offset_format format_fpr_64bit
> > +    = (TARGET_SOFT_FLOAT) ? OFFSET_FORMAT_D : format_gpr_64bit;
> 
> Why does soft float allow more?  Is that even tested?

Because on a 32-bit system, soft float DFmode goes into 2 GPRs that are D-format.
On a 64-bit system, soft float DFmode goes into 1 GPR that is DS-format.

> > +  enum rs6000_offset_format format_vector
> > +    = (TARGET_P9_VECTOR) ? OFFSET_FORMAT_DQ : OFFSET_FORMAT_NONE;
> 
> Erm, what?  This could use a comment.  Many insns are DS btw, not DQ.
> 
> > +  enum rs6000_offset_format format_tf
> > +    = (TARGET_IEEEQUAD) ? format_vector : format_fpr_64bit;
> 
> TF is either IF or KF.

Yes, and when TF is KFmode, it is a vector (DQ-format), but when it is IFmode,
it is a pair of DFmode values (i.e. typically D-format, but it can be DS-format
when loading into the traditional Altivec registers.

This is in part why I think the mode format is broken.  But, until register
allocation, we have to guess and that is what this function is trying to do.
It is used by the legitimate address functions and the constraints.

But then I've felt that the current legitimate address stuff has been broken
ever since I started working on GCC 1.37 because you don't have the context of
whether it is a load or store, and don't have the register loaded to or stored
after register allocation.  But it is what it is.

> 
> > +  /* Small integers.  */
> > +  reg_addr[E_QImode].offset_format = OFFSET_FORMAT_D;
> > +  reg_addr[E_HImode].offset_format = OFFSET_FORMAT_D;
> > +  reg_addr[E_CQImode].offset_format = OFFSET_FORMAT_D;
> > +  reg_addr[E_CHImode].offset_format = OFFSET_FORMAT_D;
> 
> Do we handle CQI and CHI anywhere else?

It depends.  It may use this format early in the RTL generation until the
complex types are split.

> 
> > +  /* While DImode can be loaded into a FPR register with LFD (d-form), the
> > +     normal use is to use a GPR (ds-form in 64-bit).  */
> > +  reg_addr[E_DImode].offset_format = format_gpr_64bit;
> > +  reg_addr[E_CDImode].offset_format = format_gpr_64bit;
> 
> I'm not sure that comment helps anything -- I don't know what it is trying
> to say?

Normally when you use a DImode, it goes into a GPR (i.e. DS-format in 64-bit).
However, there are times when you want to use a DImode in a FPR, when you can
use D-format.  As an example, the test dimode_off.c hand crafts loads and
stores with odd offsets.  We had to de-tune movdi so that the register
allocator would not load the value into a FPR and do a direct move (or do a
direct move and store from the FPR).

> > +  /* Condition registers (uses LWZ/STW).  */
> > +  reg_addr[E_CCmode].offset_format = OFFSET_FORMAT_D;
> > +  reg_addr[E_CCUNSmode].offset_format = OFFSET_FORMAT_D;
> > +  reg_addr[E_CCFPmode].offset_format = OFFSET_FORMAT_D;
> > +  reg_addr[E_CCEQmode].offset_format = OFFSET_FORMAT_D;
> 
> Okay, why do we make the "allowed address form" function support modes
> that we have no load/store insns for?!

But we do have load and store functions for the CC* modes.  See movcc_internal1.

> > +  /* 128-bit integers.  */
> > +  if (TARGET_POWERPC64)
> > +    {
> > +      reg_addr[E_TImode].offset_format = format_vector;
> 
> TI is not normally / always / often in vectors.

Not now, but it will be.

> So what is this really doing?  Preferred form for accesses in a certain
> mode?

Basically the whole thing is a guess of what the valid offset format for a mode
is.  This is needed for the legitimate address functions
(i.e. rs6000_legitimate_address_p, rs6000_legitimate_offset_address_p) to
validate whether we allow odd addresses or not.

> > +  /* Initial the mapping of mode to offset formats.  */
> > +  initialize_offset_formats ();
> 
> That comment is redundant.  (Also, spelling).
> 
> > +  /* Is this a valid prefixed address?  If the bottom four bits of the offset
> > +     are non-zero, we could use a prefixed instruction (which does not have the
> > +     DQ-form constraint that the traditional instruction had) instead of
> > +     forcing the unaligned offset to a GPR.  */
> > +  if (mode_supports_prefixed_address_p (mode)
> > +      && rs6000_prefixed_address_format (addr, OFFSET_FORMAT_DQ))
> > +    return true;
> 
> The comment says something different than the code does?
> 
> > +      switch (format)
> > +	{
> > +	default:
> > +	  gcc_unreachable ();
> 
> First not but the end at default go should.
> 
> > +/* Return true if ADDR is a valid prefixed memory address that uses mode
> > +   MODE.  */
> > +
> > +bool
> > +rs6000_prefixed_address_mode (rtx addr, machine_mode mode)
> 
> Needs _p or *_is_* or whatever.  The name should not suggest it returns
> a mode, since it doesn't.
> 
> 
> Please change format to form, and change all names and comments so it is
> clear what things do, and that it makes *sense*.  It might well work, but
> I cannot make heads or tails of it.  Maybe if I would spend another day
> on it?
> 
> 
> Segher
> 

-- 
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