Re: [PATCH] MIPS: recoginze mipsisa64 as 64bit CPU

["Maciej W. Rozycki" <macro@orcam.me.uk>]

On Tue, 15 Aug 2023, YunQiang Su wrote:

> In GCC, mipsisa64* in triples are recoginzed as 64bit CPU.
> Let's do the same.
> 
> The default ABI is determined by the abi section of triples,
> which is same with the `mips64*' CPU:
>    -gnuabi64 and -openbsd for N64
>    otherwise, N32.

 The change description has to be explicit in that we only refer to Linux 
configurations here, as it doesn't change the semantics of `mipsisa64*' 
CPUs for other OSes.  And it's not that we don't consider the CPU 64-bit 
for `mipsisa64*-*-linux*' configurations.  We just don't use a 64-bit ABI 
by default.  Finally there's no need to repeat the rules for ABI selection 
as we just follow the existing ones for `mips64*-*-linux*' configurations.

 How about:

MIPS: Use a 64-bit ABI by default for `mipsisa64*-*-linux*' targets

Following the arrangement in GCC select a 64-bit ABI by default, either 
n32 or n64, rather than o32 for `mipsisa64*-*-linux*' targets, just as 
with the corresponding `mips64*-*-linux*' targets.

then?

 NB please remember to capitalise the sentence in change headings, whether 
it's there on its own or prefixed with a subsystem name such as "MIPS:".  
And use a hyphen in combined number-word adjectives such as "64-bit".

> diff --git a/gas/configure.ac b/gas/configure.ac
> index c3bd1178d41..617a5ca1473 100644
> --- a/gas/configure.ac
> +++ b/gas/configure.ac
> @@ -394,10 +394,10 @@ changequote([,])dnl
>  	esac
>  	# Decide which ABI to target by default.
>  	case ${target} in
> -	  mips64*-openbsd* | mips64*-linux-gnuabi64)
> +	  mips64*-openbsd* | mips64*-linux-gnuabi64 | mipsisa64*-linux-gnuabi64)
>  	    mips_default_abi=N64_ABI
>  	    ;;
> -	  mips64*-linux* | mips-sgi-irix6* | mips64*-freebsd* \
> +	  mips64*-linux* | mipsisa64*-linux* | mips-sgi-irix6* | mips64*-freebsd* \

 These lines overrun 79 columns and need to be wrapped; there's a reason 
for the existing wrapping.

> diff --git a/gold/configure.tgt b/gold/configure.tgt
> index 4b54e08d27f..d09bb76ef02 100644
> --- a/gold/configure.tgt
> +++ b/gold/configure.tgt
> @@ -153,13 +153,27 @@ aarch64*-*)
>   targ_big_endian=false
>   targ_extra_big_endian=true
>   ;;
> -mips*el*-*-*|mips*le*-*-*)
> +mips64*el*-*-* | mipsisa64*le*-*-*)
> + targ_obj=mips
> + targ_machine=EM_MIPS_RS3_LE
> + targ_size=64
> + targ_big_endian=false
> + targ_extra_big_endian=true
> + ;;
> +mips*el*-*-*)

 You are removing the `mips*le*-*-*' configuration here.  It may well be 
the right move given that no other binutils component has it, but it has 
to be a separate change.

 Also the use of EM_MIPS_RS3_LE has been deprecated since forever, so 
please don't introduce a new case.  I have no idea why the existing case 
has been accepted into GOLD in the first place as BFD has never emitted it 
and it was never intended to be used for newly-produced ELF files (the 
extra MIPS ELF machine type allocation was essentially an accident in the 
psABI design, it's not even named correctly).

 NB `mips*el*-*-*' isn't right either, it should be `mips*el-*-*' just as 
elsewhere, because we'll otherwise take a CPU name with "el" in the middle 
for the endianness.

>   targ_obj=mips
>   targ_machine=EM_MIPS_RS3_LE
>   targ_size=32
>   targ_big_endian=false
>   targ_extra_big_endian=true
>   ;;
> +mips64*-*-* | mipsisa64*-*-*)
> + targ_obj=mips
> + targ_machine=EM_MIPS
> + targ_size=64
> + targ_big_endian=true
> + targ_extra_big_endian=false
> + ;;

 Also you're adding new 64-bit MIPS support to GOLD here, so it has to be 
a separate patch too from the changes to the other binutils components.  
Is it a working configuration for GOLD even?  Doesn't it need to have 
`targ_extra_size' also set?

 Please resubmit with these issues addressed.

  Maciej