Aw: Re: [PATCH] Basic support for MIPS r5900

[Richard Sandiford]

"Jürgen Urban" <JuergenUrban@gmx.de> writes:
>> > How much other changes will be currently accepted here? There is other
>> > stuff which I want to prepare and submit here, e.g.:
>> > 1. disable use of dmult and ddiv (ABI n32).
>> > 2. use trunc.w.s instead of cvt.w.s (to get single float working for
>> > normal range calculations; i.e. calculating without inf or nan).
>> > 3. fix use of ll/sc in libgomp, either increase mips ISA level or use
>> > syscall (which is broken in Linux 2.6.35.4).
>> > 4. fix libgcc to build a real muldi3 function for ABI n32 (not the
>> > multi3 function which is stored in muldi3.o file).
>> > 5. add support for configure parameters --float=single and
>> > --float=double in addition to --float=soft and --float=hard.
>> > 6. rework floating point to support single float with ABI n32 (either
>> > break the ABI or store floating point values in general purpose
>> > registers like soft float).
>> > 7. change libgcc or mips.md in way so that the non IEEE 754 compatible
>> > FPU of the r5900 gets compatible.
>>
>> Well, I'm afraid that's hard to say in advance.  It really depends
>> on what the changes look like.  (1) and (2) sound harmless enough,
>> although (1) should probably only be done in conjunction with (4).
>> I'm not sure what (3) involves.  (5) sounds like a good idea.
>> (6) is worth doing, but anything ABI-related gets extra-paranoid
>> treatment. :-)
>
> The attached patch fixes (1) and (4). This makes mips64r5900el usable
> with r5900. If (4) is a problem (i.e. patching libgcc/Makefile.in), it
> would be good if at least (1) is accepted.

I can't approve the Makefile.in bits.  I've cc'ed Ian, who's the libgcc
maintainer.  Ian: the problem is that "_muldi3.o" on 64-bit targets
is actually an implementation of __multi3.  Jürgen wants to have a
__muldi3 too, with the same implementation as on 32-bit targets.

I think (1) and (4) should go in together though.  (1) doesn't make much
sense without a libgcc function to back it up.

> The patch for mips.md after line 1992 (adds TARGET_64BIT) is a more
> general fix. This is not needed for r5900 support, but I think this
> should be fixed.
> The same applies for patch after 2233 (adds ISA_HAS_DMULT). The fix here
> would be also adding TARGET_64BIT, but for r5900 we need ISA_HAS_DMULT
> here.

The current state is actually deliberate.  define_expand conditions are
only ever used in HAVE_* macros, so whatever we put there will not get
tested.  I think it's less confusing to have no test than an unused one,
just like we try not to have constraints in define_expands.

The other bits of the config/mips patch look good, thanks.  A couple of
formatting niggles:

> +/* ISA supports instructions dmult and dmultu. */
> +#define ISA_HAS_DMULT           (TARGET_64BIT				\
> +				 && !TARGET_MIPS5900)
> +
> +/* ISA supports instructions mult and multu.
> +   This always supported, but the macro is needed for ISA_HAS_<D>MULT
> +   in mips.md.  */
> +#define ISA_HAS_MULT		(1)
> +
> +/* ISA supports instructions ddiv and ddivu. */
> +#define ISA_HAS_DDIV            (TARGET_64BIT				\
> +				 && !TARGET_MIPS5900)

Please keep ISA_HAS_DMULT and ISA_HAS_DDIV on one line while they fit.
I prefer caps for insn names in the comments, but the code isn't yet
as consistent as it should be, sorry...

> +/* ISA supports instructions div and divu.
> +   This always supported, but the macro is needed for ISA_HAS_<D>DIV
> +   in mips.md.  */
> +#define ISA_HAS_DIV		(1)
> +
> +

Excess blank line here.

Thanks,
Richard