From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10503 invoked by alias); 7 Jan 2013 21:52:59 -0000 Received: (qmail 10494 invoked by uid 22791); 7 Jan 2013 21:52:59 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_50,DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_SPAMHAUS_DROP,NML_ADSP_CUSTOM_MED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-wg0-f54.google.com (HELO mail-wg0-f54.google.com) (74.125.82.54) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 07 Jan 2013 21:52:52 +0000 Received: by mail-wg0-f54.google.com with SMTP id fg15so9948762wgb.21 for ; Mon, 07 Jan 2013 13:52:50 -0800 (PST) X-Received: by 10.194.87.200 with SMTP id ba8mr98266247wjb.22.1357595570921; Mon, 07 Jan 2013 13:52:50 -0800 (PST) Received: from localhost ([2.26.203.77]) by mx.google.com with ESMTPS id s16sm14501678wii.0.2013.01.07.13.52.48 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Mon, 07 Jan 2013 13:52:49 -0800 (PST) From: Richard Sandiford To: =?utf-8?Q?J=C3=BCrgen_Urban?= Mail-Followup-To: =?utf-8?Q?J=C3=BCrgen_Urban?= ,gcc-patches@gcc.gnu.org, rdsandiford@googlemail.com Cc: gcc-patches@gcc.gnu.org Subject: Re: Support for MIPS r5900 In-Reply-To: <20130106225645.190700@gmx.net> (=?utf-8?Q?=22J=C3=BCrgen?= Urban"'s message of "Sun, 06 Jan 2013 23:56:45 +0100") References: <20130106225645.190700@gmx.net> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) Date: Mon, 07 Jan 2013 21:52:00 -0000 Message-ID: <87y5g43bkf.fsf@talisman.default> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2013-01/txt/msg00365.txt.bz2 "J=C3=BCrgen Urban" writes: > ll, sc, dmult, ddiv, cvt.w.s, 64 bit FPU instructions. > ll and sc is disabled with "-mno-llsc" and works. > cvt.w.s is replaced by trunc.w.s. This seems to work. Probably showing my ignorance, but I couldn't see this in the patch. > I disabled 64 bit FPU instructions by "-msoft-float". This works, but > using "-msingle-float" fails. This would be the better > configuration. There are still 64 bit FPU instructions used (e.g. "dmfc1 > $2,$f0" when using "long double" multiplication). So "-msingle-float" > doesn't seem to work on generic mips64-linux-gnu. Right. That combination hasn't really been defined. What happens for plain doubles? Do you pass those in FPRs or GPRs? > I tried to disable dmult and ddiv (see mips.md). Disabling worked, but > now muldi3 calls itself in libgcc2. I thought this should work, because > I got this working with GCC 4.3, but the latest GCC version is a > problem. multi3 is calling muldi3, so that muldi3 should be able to use > mulsi3, because it is the same C code in libgcc2. Can someone get me > some hints or comments? How can this be debugged? Not sure, sorry. > Does someone know how to enable TImode in MIPS ABI o32 (this doesn't > need to use the 128 bit instructions at the moment)? There is some old > code for the Playstation 2 which needs this. I know that TImode is > supported in ABI n32, but some code uses also the 32 bit FPU and FPU > registers are not available with "-msoft-float" in inline assembler. The n32 TImode support you mention uses pairs of GPRs, whereas I imagine you'd eventually want to use a single 128-bit GPR. Is that right? TImode in the current n32 sense doesn't really make practical sense for 32-bit targets. We'd be dealing with quad registers in that case. I think it would only make sense with TImode registers. ISTR the TImode registers being a can of worms, especially with the optimisation to only store the lower 64 bits if the upper 64 weren't used. (Am I remembering that right?) When you submit the TImode register support, please make the support itself a separate patch from the ABI changes. I.e. one patch to add TImode registers, one to add TImode to o32, one to add single-GPR TImode to n32, etc. For the record, I think all those patches would be too invasive this late into the 4.8 cycle so would have to wait for 4.9. Also, any ABI changes should be conditional on a new flag rather than keyed off the architecture. That flag would then be the default for your new configuration. > What is the best way to change the alignment to 128 bit for all > structures and stack in any MIPS ABI? Much old code for the Playstation > 2 expects this. The stack is STACK_BOUNDARY (already 128 for n32). Do you mean the padding of all structure types, or just global structure variables? If the former, it sounds like ROUND_TYPE_ALIGN, but also sounds scary :-) If the latter, it's DATA_ALIGNMENT. > @@ -15801,6 +15816,11 @@ mips_reorg_process_insns (void) > if (TARGET_FIX_VR4120 || TARGET_FIX_24K) > cfun->machine->all_noreorder_p =3D false; >=20=20 > + /* Code compiled for R5900 can't be all noreorder because > + we rely on the assembler to work around some errata. */ > + if (TARGET_MIPS5900) > + cfun->machine->all_noreorder_p =3D false; > + > /* The same is true for -mfix-vr4130 if we might generate MFLO or > MFHI instructions. Note that we avoid using MFLO and MFHI if > the VR4130 MACC and DMACC instructions are available instead; Please fold this into the clause above it. > +/* Target supports instructions dmult and dmultu (integer). */ > +#define TARGET_HAS_DMULT (TARGET_64BIT \ > + && !TARGET_MIPS5900) Please use ISA_HAS_* for consistency with other macros. I think it'd be better to drop the '(integer)'. > +/* Target supports instructions mult and multu in 32 bit mode (integer).= */ > +#define TARGET_HAS_MULT (mips_isa >=3D 1) ...and here drop 'in 32 bit mode (integer)'. 32-bit-mode isn't really relevant here. > +/* Target supports instructions dmult and dmultu (integer). */ > +#define TARGET_HAS_DDIV (TARGET_64BIT \ > + && !TARGET_MIPS5900) Same as above. > +/* Target supports instructions mult and multu in 32 bit mode (integer).= */ > +#define TARGET_HAS_DIV (mips_isa >=3D 1) Here too, plus "div" rather than "mult". > @@ -841,10 +859,10 @@ struct mips_cpu_info { >=20=20 > /* ISA has the integer conditional move instructions introduced in mips4= and > ST Loongson 2E/2F. */ > -#define ISA_HAS_CONDMOVE (ISA_HAS_FP_CONDMOVE || TARGET_LOONGSON_= 2EF) > +#define ISA_HAS_CONDMOVE (ISA_HAS_FP_CONDMOVE || TARGET_LOONGSON_= 2EF || TARGET_MIPS5900) GCC has a strict 80-column limit, so please make this: #define ISA_HAS_CONDMOVE (ISA_HAS_FP_CONDMOVE \ || TARGET_LOONGSON_2EF \ || TARGET_MIPS5900) > /* ISA has LDC1 and SDC1. */ > -#define ISA_HAS_LDC1_SDC1 (!ISA_MIPS1 && !TARGET_MIPS16) > +#define ISA_HAS_LDC1_SDC1 (!ISA_MIPS1 && !TARGET_MIPS16 && !TARGET_MIPS5= 900) Same 3-line expansion here. > @@ -974,7 +993,11 @@ struct mips_cpu_info { > /* True if trunc.w.s and trunc.w.d are real (not synthetic) > instructions. Both require TARGET_HARD_FLOAT, and trunc.w.d > also requires TARGET_DOUBLE_FLOAT. */ > -#define ISA_HAS_TRUNC_W (!ISA_MIPS1) > +#define ISA_HAS_TRUNC_W_D (!ISA_MIPS1) > + > +/* True if trunc.w.s is real (not synthetic) instructions. > + Requires TARGET_HARD_FLOAT. */ > +#define ISA_HAS_TRUNC_W_S (ISA_HAS_TRUNC_W_D || TARGET_MIPS5900) First comment still describes both cases, so I think the second one is redundant. Just: /* True if trunc.w.s and trunc.w.d are real (not synthetic) instructions. Both require TARGET_HARD_FLOAT, and trunc.w.d also requires TARGET_DOUBLE_FLOAT. */ #define ISA_HAS_TRUNC_W_D (!ISA_MIPS1) #define ISA_HAS_TRUNC_W_S (ISA_HAS_TRUNC_W_D || TARGET_MIPS5900) > @@ -726,7 +727,7 @@ > ;; This mode iterator allows :MOVECC to be used anywhere that a > ;; conditional-move-type condition is needed. > (define_mode_iterator MOVECC [SI (DI "TARGET_64BIT") > - (CC "TARGET_HARD_FLOAT && !TARGET_LOONGSON= _2EF")]) > + (CC "TARGET_HARD_FLOAT && !TARGET_LOONGSON= _2EF && !TARGET_MIPS5900")]) Same three-line expansion here: (define_mode_iterator MOVECC [SI (DI "TARGET_64BIT") (CC "TARGET_HARD_FLOAT && !TARGET_LOONGSON_2EF && !TARGET_MIPS5900")]) > @@ -1900,7 +1901,7 @@ > [(set (match_operand:DI 0 "muldiv_target_operand" "=3Dka") > (mult:DI (any_extend:DI (match_operand:SI 1 "register_operand" "d")) > (any_extend:DI (match_operand:SI 2 "register_operand" "d"))))] > - "!TARGET_64BIT && (!TARGET_FIX_R4000 || ISA_HAS_DSP)" > + "(!TARGET_64BIT || (TARGET_64BIT && !TARGET_HAS_DMULT)) && (!TARGET_FI= X_R4000 || ISA_HAS_DSP)" > { > if (ISA_HAS_DSP_MULT) > return "mult\t%q0,%1,%2"; Just: "!ISA_HAS_DMULTU && (!TARGET_FIX_R4000 || ISA_HAS_DSP)" Please update mulsidi3_32bit_mips16 and mulsidi3_32bit_r4000 in the same way. > @@ -1927,7 +1928,7 @@ > (any_extend:DI (match_operand:SI 2 "register_operand" "d")))) > (clobber (match_scratch:TI 3 "=3Dx")) > (clobber (match_scratch:DI 4 "=3Dd"))] > - "TARGET_64BIT && !TARGET_FIX_R4000 && !ISA_HAS_DMUL3 && !TARGET_MIPS16" > + "TARGET_64BIT && !TARGET_FIX_R4000 && !ISA_HAS_DMUL3 && !TARGET_MIPS16= && TARGET_HAS_DMULT" > "#" > "&& reload_completed" > [(const_int 0)] Just: "ISA_HAS_DMULTU && !TARGET_FIX_R4000 && !ISA_HAS_DMUL3 && !TARGET_MIPS16" Please update mulsidi3_64bit_mips16 in the same way. > @@ -2105,7 +2106,7 @@ > { > rtx hilo; >=20=20 > - if (TARGET_64BIT) > + if (TARGET_64BIT && TARGET_HAS_DMULT) > { > hilo =3D gen_rtx_REG (TImode, MD_REG_FIRST); > emit_insn (gen_mulsidi3_64bit_hilo (hilo, operands[1], operands= [2])); Here too just ISA_HAS_DMULT. Several other cases later on, I won't bore you with them all :-) > @@ -2537,7 +2541,7 @@ > (set (match_operand:GPR 3 "register_operand") > (mod:GPR (match_dup 1) > (match_dup 2)))] > - "!TARGET_FIX_VR4120" > + "!TARGET_FIX_VR4120 && TARGET_HAS_DIV" > { > if (TARGET_MIPS16) > { Would prefer the ISA_HAS_DIV first. Please update the MIPS16 patterns in the same way. > @@ -1881,11 +1881,17 @@ mipsisa64sb1-*-elf* | mipsisa64sb1el-*-e > target_cpu_default=3D"MASK_64BIT|MASK_FLOAT64" > tm_defines=3D"${tm_defines} MIPS_ISA_DEFAULT=3D64 MIPS_CPU_STRING_DEFAU= LT=3D\\\"sb1\\\" MIPS_ABI_DEFAULT=3DABI_O64" > ;; > -mips-*-elf* | mipsel-*-elf*) > +mips-*-elf* | mipsel-*-elf* | mipsr5900-*-elf* | mipsr5900el-*-elf*) > tm_file=3D"elfos.h newlib-stdint.h ${tm_file} mips/elf.h" > tmake_file=3D"mips/t-elf" > ;; > -mips64-*-elf* | mips64el-*-elf*) > +mips64r5900-*-elf* | mips64r5900el-*-elf*) > + tm_file=3D"elfos.h newlib-stdint.h ${tm_file} mips/elf.h" > + tmake_file=3D"mips/t-elf" > + target_cpu_default=3D"MASK_64BIT" > + tm_defines=3D"${tm_defines} MIPS_ISA_DEFAULT=3D3 MIPS_ABI_DEFAULT=3DABI= _N32" > + ;; > +mips64-*-elf* | mips64el-*-elf* | mips64r5900-*-elf* | mips64r5900el-*-e= lf*) > tm_file=3D"elfos.h newlib-stdint.h ${tm_file} mips/elf.h" > tmake_file=3D"mips/t-elf" > target_cpu_default=3D"MASK_64BIT|MASK_FLOAT64" The change to the "mips64-*-elf* | mips64el-*-elf*)" line looks unnecessary. > @@ -3374,7 +3400,7 @@ case "${target}" in > supported_defaults=3D"abi arch arch_32 arch_64 float tune tune_32 tune= _64 divide llsc mips-plt synci" >=20=20 > case ${with_float} in > - "" | soft | hard) > + "" | soft | hard | single | double) > # OK > ;; > *) Please leave this out for now and add it with the ABI changes mentioned above. I can't approve the libgcc bits, but I'm afraid they probably tip the balance against this being acceptable for 4.8. Richard