On Thu, Jul 11, 2019 at 03:44:02PM -0400, Michael Meissner wrote:
> --- gcc/config/rs6000/rs6000-protos.h (revision 273371)
> +++ gcc/config/rs6000/rs6000-protos.h (working copy)
> @@ -154,6 +154,18 @@ extern align_flags rs6000_loop_align (rt
> extern void rs6000_split_logical (rtx [], enum rtx_code, bool, bool, bool);
> extern bool rs6000_pcrel_p (struct function *);
> extern bool rs6000_fndecl_pcrel_p (const_tree);
> +
> +/* Enumeration to describe the offsettable address formats of PowerPC memory
> + instructions. */
> +enum rs6000_offset_format {
> + OFFSET_FORMAT_NONE, /* No offset format is available. */
> + OFFSET_FORMAT_D, /* All 16-bits are available. */
> + OFFSET_FORMAT_DS, /* Bottom 2 bits must be 0. */
> + OFFSET_FORMAT_DQ /* Bottom 4 bits must be 0. */
> +};
It's called "instruction form", let's not call it "address format".
"Form" is shorter and more correct, as well.
> +/* Helper function to return whether a MODE can do prefixed loads/stores.
> + VOIDmode is used when we are loading the pc-relative address into a base
> + register, but we are not using it as part of a memory operation. As modes
> + add support for prefixed memory, they will be added here. */
> +
> +static bool
> +mode_supports_prefixed_address_p (machine_mode mode)
> +{
> + return mode == VOIDmode;
> +}
s/Helper function to return/Return/
It's not obvious from this comment that "mode" is the mode of the thing
that is loaded or stored (and not the address of the datum, or something).
The VOIDmode comment wants to say that you use VOIDmode with this function
when you aren't actually loading or storing anything, just taking an
address?
The last line shouldn't be here: predictions will never be true ;-)
What qualifies as a "prefixed load/store" for this?
> +/* Initiailize the table of offset address formats. Unfortunately we do not
(Typo)
> + /* Set offset format for some types based on the switches. */
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*!
> + enum rs6000_offset_format format_gpr_128bit
> + = (TARGET_QUAD_MEMORY) ? OFFSET_FORMAT_DQ : format_gpr_64bit;
128-bit GPRs do not exist, either.
> + 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?
> + 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.
> + /* 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?
> + /* 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?
> + /* 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?!
> + /* 128-bit integers. */
> + if (TARGET_POWERPC64)
> + {
> + reg_addr[E_TImode].offset_format = format_vector;
TI is not normally / always / often in vectors.
So what is this really doing? Preferred form for accesses in a certain
mode?
> + /* 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
