Re: [PATCH] Add Octeon2 indexed load instruction support (and also DSP64 LDX support)

[Andrew Pinski <andrew dot pinski at caviumnetworks dot com>]

On Sat, Dec 17, 2011 at 3:11 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Andrew Pinski <andrew.pinski@caviumnetworks.com> writes:
>> Index: testsuite/gcc.target/mips/octeon2-lx-1.c
>> ===================================================================
>> --- testsuite/gcc.target/mips/octeon2-lx-1.c Â(revision 0)
>> +++ testsuite/gcc.target/mips/octeon2-lx-1.c Â(revision 0)
>> @@ -0,0 +1,18 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-march=octeon2 -O -mgp64" } */
>> +
>> +#define TEST(N, R, T) \
>> + ÂT f##N (T j, R *b, long long i) { return j + b[i]; } \
>> + ÂT g##N (T j, unsigned R *b, long long i) { return j + b[i]; }
>> +
>> +TEST (1, char, int)
>> +TEST (2, char, long long)
>> +/* { dg-final { scan-assembler-times "\tlbx\t" 2 } } */
>> +/* { dg-final { scan-assembler-times "\tlbux\t" 2 } } */
>> +TEST (3, short, int)
>> +TEST (4, short, long long)
>> +/* { dg-final { scan-assembler-times "\tlhx\t" 2 } } */
>> +/* { dg-final { scan-assembler-times "\tlhux\t" 2 } } */
>> +TEST (5, int, long long)
>> +/* { dg-final { scan-assembler-times "\tlwx\t" 1 } } */
>> +/* { dg-final { scan-assembler-times "\tlwux\t" 1 } } */
>
> There's obviously nothing wrong with testing long long indices,
> but it doesn't seem very typical. ÂI'd prefer the attached, which
> tests both "long long" and "int". Â(I checked that it works for
> -mabi=n32 and -mabi=64 on mips64-linux-gnu.)
>
>> -(define_insn "mips_lwx_<mode>"
>> - Â[(set (match_operand:SI 0 "register_operand" "=d")
>> - Â Â (mem:SI (plus:P (match_operand:P 1 "register_operand" "d")
>> - Â Â Â Â Â Â Â Â Â Â (match_operand:P 2 "register_operand" "d"))))]
>> - Â"ISA_HAS_DSP"
>> - Â"lwx\t%0,%2(%1)"
>
> (Reviewing the patch made me realise that this ought to be using IMOVE32,
> just like lwxs does. ÂBut that's something for another time.)
>
>> +(define_expand "mips_ldx"
>> + Â[(match_operand:DI 0 "register_operand")
>> + Â (match_operand 1 "pmode_register_operand")
>> + Â (match_operand:SI 2 "register_operand")]
>> + Â"ISA_HAS_DSP && TARGET_64BIT"
>> +{
>> + Âoperands[2] = convert_to_mode (Pmode, operands[2], false);
>> + Âemit_insn (PMODE_INSN (gen_mips_ldx,
>> + Â Â Â Â Â Â Â Â Â Â Â(operands[0], operands[1], operands[2])));
>> + ÂDONE;
>> +})
>
> Seems like this and lwx could be combined using :GPR.
>
>> -;; This attribute gives the length suffix for a sign- or zero-extension
>> -;; instruction.
>> -(define_mode_attr size [(QI "b") (HI "h")])
>> +;; This attribute gives the length suffix for a sign-, zero-extension
>> +;; load and store instruction.
>> +(define_mode_attr size [(QI "b") (HI "h") (SI "w") (DI "d")])
>> +(define_mode_attr SIZE [(QI "B") (HI "H") (SI "W") (DI "D")])
>
> ;; This attribute gives the length suffix for a load or store instruction.
> ;; The same suffixes work for zero and sign extensions.
>
>> Index: config/mips/mips.c
>> ===================================================================
>> --- config/mips/mips.c    Â(revision 182342)
>> +++ config/mips/mips.c    Â(working copy)
>> @@ -2159,6 +2159,29 @@ mips_lwxs_address_p (rtx addr)
>> Â Â Â}
>> Â Âreturn false;
>> Â}
>> +
>> +/* Return true if ADDR matches the pattern for the L{B,H,W,D}{,U}X load
>> + Â indexed address instruction. ÂNote that such addresses are
>> + Â not considered legitimate in the TARGET_LEGITIMATE_ADDRESS_P
>> + Â sense, because their use is so restricted. Â*/
>> +
>> +static bool
>> +mips_loadindexed_address_p (rtx addr, enum machine_mode mode)
>
> Nitlet, but I'd prefer mips_lx_address_p or mips_load_indexed_address_p.
>
>> @@ -3552,6 +3575,11 @@ mips_rtx_costs (rtx x, int code, int out
>> Â Â Â Â *total = COSTS_N_INSNS (2);
>> Â Â Â Â return true;
>> Â Â Â }
>> + Â Â Âif (mips_loadindexed_address_p (addr, mode))
>> + Â Â {
>> + Â Â Â *total = COSTS_N_INSNS (2);
>> + Â Â Â return true;
>> + Â Â }
>
> Please combine this with the lwxs condition.
>
>> @@ -12959,6 +12988,9 @@ static const struct mips_builtin_descrip
>> Â ÂDIRECT_BUILTIN (mult, MIPS_DI_FTYPE_SI_SI, dsp_32),
>> Â ÂDIRECT_BUILTIN (multu, MIPS_DI_FTYPE_USI_USI, dsp_32),
>>
>> + Â/* Built-in functions for the DSP ASE (64-bit only). Â*/
>> + ÂDIRECT_BUILTIN (ldx, MIPS_DI_FTYPE_POINTER_SI, dsp_64),
>> +
>> Â Â/* The following are for the MIPS DSP ASE REV 2 (32-bit only). Â*/
>> Â ÂDIRECT_BUILTIN (dpa_w_ph, MIPS_DI_FTYPE_DI_V2HI_V2HI, dspr2_32),
>> Â ÂDIRECT_BUILTIN (dps_w_ph, MIPS_DI_FTYPE_DI_V2HI_V2HI, dspr2_32),
>
> You need to add this to the list in extend.texi too.
>
>> +#define ISA_HAS_LDX Â Â Â Â Â((ISA_HAS_DSP || TARGET_OCTEON2) && TARGET_64BIT)
>
> Long line:
>
> #define ISA_HAS_LDX Â Â Â Â Â Â ((ISA_HAS_DSP || TARGET_OCTEON2) \
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â && TARGET_64BIT)
>
> OK otherwise, thanks.

Here is the patch which I committed after a bootstrap/test on mips64-linux-gnu.

Thanks,
Andrew Pinski

gcc/ChangeLog:
* config/mips/mips.md (size): Add SI and DI.
(SIZE): New mode attribute.
(U): New code attribute.
* config/mips/mips-dsp.md (mips_lbux): Use gen_mips_lbux_extsi.
(mips_lbux_<mode>): Delete.
(mips_l<SHORT:size><u>x_ext<GPR:mode>_<P:mode>): New pattern.
(mips_lhx): Use gen_mips_lhx_extsi.
(mips_lhx_<mode>): Delete.
(mips_lwx): Delete.
(mips_l<size>x): New expand.
(mips_lwx_<mode>): Delete.
(mips_l<GPR:size>x_<P:mode>): New pattern.
(*mips_lw<u>x_<P:mode>_ext): Likewise.
* config/mips/mips-ftypes.def: Add DI f(POINTER, SI) function type.
* config/mips/mips.c (mips_lx_address_p): New function.
(mips_rtx_costs <case MEM>): Call mips_lx_address_p.
(dsp64): New availability predicate.
(mips_builtins): Add an entry for __builtin_mips_ldx.
* config/mips/mips.h (ISA_HAS_LBX): New define.
(ISA_HAS_LBUX): Likewise.
(ISA_HAS_LHX): Likewise.
(ISA_HAS_LHUX): Likewise.
(ISA_HAS_LWX): Likewise.
(ISA_HAS_LWUX): Likewise.
(ISA_HAS_LDX): Likewise.
* doc/extend.texi (__builtin_mips_ldx): Document.

gcc/testsuite/ChangeLog:
* gcc.target/mips/mips64-dsp-ldx1.c: New test.
* gcc.target/mips/octeon2-lx-1.c: New test.
* gcc.target/mips/mips64-dsp-ldx.c: New test.
* gcc.target/mips/octeon2-lx-2.c: New test.
* gcc.target/mips/octeon2-lx-3.c: New test.


>
> Richard
>
>
> /* { dg-do compile } */
> /* { dg-options "-march=octeon2 -O -mgp64" } */
>
> #define TEST(N, R, T) \
> ÂT fll##N (T j, R *b, long long i) { return j + b[i]; } \
> ÂT gll##N (T j, unsigned R *b, long long i) { return j + b[i]; } \
> ÂT fi##N (T j, R *b, int i) { return j + b[i]; } \
> ÂT gi##N (T j, unsigned R *b, int i) { return j + b[i]; } \
>
> TEST (1, char, int)
> TEST (2, char, long long)
> /* { dg-final { scan-assembler-times "\tlbx\t" 4 } } */
> /* { dg-final { scan-assembler-times "\tlbux\t" 4 } } */
> TEST (3, short, int)
> TEST (4, short, long long)
> /* { dg-final { scan-assembler-times "\tlhx\t" 4 } } */
> /* { dg-final { scan-assembler-times "\tlhux\t" 4 } } */
> TEST (5, int, long long)
> /* { dg-final { scan-assembler-times "\tlwx\t" 2 } } */
> /* { dg-final { scan-assembler-times "\tlwux\t" 2 } } */

Attachment: addo2index.diff.txt
Description: Text document