Re: [patch] modify crt0.S for 64-bit address targets

Re: [patch] modify crt0.S for 64-bit address targets

[cgd]

(added cc: to binutils list.)

Eric,

My take on this is that the problem is **not** that addu/subu are
incorrect, but rather that the address being loaded is not what's
intended!

The intended address (K0BASE) is 0xffffffff80000000, and based on your
comments (and the detection of the UNPREDICTABLE condition), I believe
that 0x0000000080000000 is being loaded instead.

The address is a sign-extended 32-bit value, the addu/subu should be
OK.


If the simulator's actually doing the right thing, and the code really
is what your description below indicates, the changed code shouldn't
work any better.  (I forget, i have some recollection that the current
sim might always truncate addresses to 32-bits.  but it's been a while
since i looked at that.  *sigh*)


I'm probably confused/mistaken but... i thought 'la' was supposed to
generate a 32-bit address (and dla generate a 64-bit address)?  Or
maybe they always generate the address of the ABI in use?  I
forget... so much change.




chris

At Wed, 6 Apr 2005 06:28:30 +0000 (UTC), "Eric Christopher" wrote:
> So, in testing mipsisa64-elf lately we were getting a number of
> UNPREDICTABLE results because of this:
> 
> 	la      t1,K0BASE
>         addu    t0,t0,t1
>         subu    t0,t0,64
> 
> Which would assemble to this:
> 
> 	ori
> 	dsll
> 
> 	addu
> 	subu
> 
> The problem is that ABI_EABI for 64-bit targets assumes 64-bit addresses
> and the doubleword shift meant that register t0 would get something that
> isn't a word value, causing unpredictable results depending on
> processor. The mips simulator flags these. Now, shifting this to using
> 64-bit instructions for 64-bit addresses seems to be the best method for
> this. I only changed the ones that the simulator would catch thinking it
> best that we only change the ones we need to. I'm perfectly happy to
> switch it to using 64-bit instructions for everything for 64-bit, but I
> think that's probably a bit much.
> 
> Tested on mipsisa64-elf and mips64-elf.
> 
> OK?
> 
> 2005-04-05  Eric Christopher  <echristo@redhat.com>
> 
> 	* mips/crt0.S: Add macros for 64-bit add/sub.
> 	(zerobss): Use when dealing with addresses.
> 
> Index: crt0.S
> ===================================================================
> RCS file: /cvs/src/src/libgloss/mips/crt0.S,v
> retrieving revision 1.9
> diff -u -p -w -r1.9 crt0.S
> --- crt0.S 26 May 2003 20:22:16 -0000 1.9
> +++ crt0.S 6 Apr 2005 02:02:55 -0000
> @@ -41,6 +41,16 @@
> # define LA(t,x) la t,x
> #endif /* __mips_embedded_pic */
> 
> +#if !defined(__mips64)
> +  /* This machine does not support 64-bit operations.  */
> +  #define ADDU addu
> +  #define SUBU subu
> +#else
> +  /* This machine supports 64-bit operations.  */
> +  #define ADDU daddu
> +  #define SUBU dsubu
> +#endif
> + 
> .comm __memsize, 12
> .comm __lstack, STARTUP_STACK_SIZE
> 
> @@ -156,11 +166,11 @@ zerobss:
> la a0, __memsize
> lw t0,0(a0) # last address of memory available
> la t1,K0BASE # cached kernel memory
> - addu t0,t0,t1 # get the end of memory address
> + ADDU t0,t0,t1 # get the end of memory address
> /* Allocate 32 bytes for the register parameters.  Allocate 16
>    bytes for a null argv and envp.  Round the result up to 64
>    bytes to preserve alignment.  */
> - subu t0,t0,64
> + SUBU t0,t0,64
> 4:
> move sp,t0 # set stack pointer
> .end zerobss

Re: [patch] modify crt0.S for 64-bit address targets

[Maciej W. Rozycki]

On Wed, 5 Apr 2005 cgd@broadcom.com wrote:

> I'm probably confused/mistaken but... i thought 'la' was supposed to
> generate a 32-bit address (and dla generate a 64-bit address)?  Or
> maybe they always generate the address of the ABI in use?  I
> forget... so much change.

 AFAIK, the only reason for both "dla" and "la" to exist is history and 
compatibility with existing code -- there is no need to encode the address 
size in the mnemonic as its already implied by the ABI in use.  
Therefore, as far as gas is concerned, both expand to the same code -- 
it's just they generate different warning messages and for different 
conditions: "dla" complains about being used for ABIs that have 32-bit 
registers and "la" does that for ones that have 64-bit addresses.

  Maciej

Re: [patch] modify crt0.S for 64-bit address targets

[Thiemo Seufer]

Maciej W. Rozycki wrote:
> On Wed, 5 Apr 2005 cgd@broadcom.com wrote:
> 
> > I'm probably confused/mistaken but... i thought 'la' was supposed to
> > generate a 32-bit address (and dla generate a 64-bit address)?  Or
> > maybe they always generate the address of the ABI in use?  I
> > forget... so much change.
> 
>  AFAIK, the only reason for both "dla" and "la" to exist is history and 
> compatibility with existing code -- there is no need to encode the address 
> size in the mnemonic as its already implied by the ABI in use.  

This is incorrect for the (historic) no-ABI mode, as well as for e.g. a
.set mips3 ... .set mips0 sequence. la should load a sign-extended 32bit
value in that case.


Thiemo

Re: [patch] modify crt0.S for 64-bit address targets

[Maciej W. Rozycki]

On Wed, 6 Apr 2005, Thiemo Seufer wrote:

> >  AFAIK, the only reason for both "dla" and "la" to exist is history and 
> > compatibility with existing code -- there is no need to encode the address 
> > size in the mnemonic as its already implied by the ABI in use.  
> 
> This is incorrect for the (historic) no-ABI mode, as well as for e.g. a

 What's a no-ABI mode?  There's always an ABI present, whether explicit or 
not.  For the purpose of addressing, using 32-bit ELF implies 32-bit 
addresses, while using 64-bit ELF means 64-bit addresses.

> .set mips3 ... .set mips0 sequence. la should load a sign-extended 32bit
> value in that case.

 It will as these don't affect the address size -- la uses the address 
size that reflects the ABI selected, unless you restrict it with something 
like ".set mips1".  But using ".set mips1", ".set mips2" and ".set mips32" 
should probably be forbidden for ABIs that require a 64-bit ISA, just like 
option sets like "-mabi=64 -march=mips1".

  Maciej

Re: [patch] modify crt0.S for 64-bit address targets

[Eric Christopher]

On Tue, 2005-04-05 at 23:46 -0700, cgd@broadcom.com wrote:
> (added cc: to binutils list.)
> 
> Eric,
> 
> My take on this is that the problem is **not** that addu/subu are
> incorrect, but rather that the address being loaded is not what's
> intended!
> 
> The intended address (K0BASE) is 0xffffffff80000000, and based on your
> comments (and the detection of the UNPREDICTABLE condition), I believe
> that 0x0000000080000000 is being loaded instead.
> 

Assuredly this is the case. I think the problem might be from using the
ori instead of addi for the address. ori zero extends the constant, addi
sign extends. IIRC all addresses should be sign extended and as such we
should be using addi here yes? I seem to recall that we changed this a
while back because the ori was some small amount faster.

Maciej? Thiemo? This ringing any bells?

> The address is a sign-extended 32-bit value, the addu/subu should be
> OK.

You are correct.

-eric

Re: [patch] modify crt0.S for 64-bit address targets

[Paul Koning]

>>>>> "Eric" == Eric Christopher <echristo@redhat.com> writes:

 Eric> On Tue, 2005-04-05 at 23:46 -0700, cgd@broadcom.com wrote:
 >> (added cc: to binutils list.)
 >> 
 >> Eric,
 >> 
 >> My take on this is that the problem is **not** that addu/subu are
 >> incorrect, but rather that the address being loaded is not what's
 >> intended!
 >> 
 >> The intended address (K0BASE) is 0xffffffff80000000, and based on
 >> your comments (and the detection of the UNPREDICTABLE condition),
 >> I believe that 0x0000000080000000 is being loaded instead.
 >> 

 Eric> Assuredly this is the case. I think the problem might be from
 Eric> using the ori instead of addi for the address. ori zero extends
 Eric> the constant, addi sign extends. IIRC all addresses should be
 Eric> sign extended and as such we should be using addi here yes? I
 Eric> seem to recall that we changed this a while back because the
 Eric> ori was some small amount faster.

Faster?  On a RISC machine?  Seems odd.

Anyway, ori is fine.  The sequence for LA is LUI then ORI, and LUI
loads a sign-extended 32 bit value with zeroes in the low order.  So
ORI is the easy way to fill in the low order.  The LUI is what ensures
that the value is formatted correctly in 64 bit registers.

ADDI or ADDIU both sign-extend the 16-bit argument, so doing an LA
with one of those, while possible, is more trouble because the LUI
argument has to be adjusted up by 1 if the low half of the value to
load is > 7fff.  With ORI, the argument is zero-extended so it the low
bits aren't a consideration when picking the LUI argument

     paul

Re: [patch] modify crt0.S for 64-bit address targets

[Ian Lance Taylor]

Paul Koning <pkoning@equallogic.com> writes:

>  Eric> Assuredly this is the case. I think the problem might be from
>  Eric> using the ori instead of addi for the address. ori zero extends
>  Eric> the constant, addi sign extends. IIRC all addresses should be
>  Eric> sign extended and as such we should be using addi here yes? I
>  Eric> seem to recall that we changed this a while back because the
>  Eric> ori was some small amount faster.
> 
> Faster?  On a RISC machine?  Seems odd.

I think Eric is reversed, actually.  There was a MIPS processor which
had two add units but only one logical unit.  Therefore, on average,
on that processor, add was faster.  That fact led to this change:

Tue Jul 11 11:49:49 1995  Ian Lance Taylor  <ian@cygnus.com>

	* mips-opc.c (mips_opcodes): For the move pseudo-op, prefer daddu
	if ISA 3 and addu otherwise, replacing or, since some MIPS chips
	have multiple add units but only a single logical unit.

Ian

Re: [patch] modify crt0.S for 64-bit address targets

[Eric Christopher]

> Anyway, ori is fine.  The sequence for LA is LUI then ORI, and LUI
> loads a sign-extended 32 bit value with zeroes in the low order.  So
> ORI is the easy way to fill in the low order.  The LUI is what ensures
> that the value is formatted correctly in 64 bit registers.
> 

I think "the sequence" is probably a bit much. Right now we're not doing
LUI, and merely going with load register because the code sequence looks
like this:

la t1,K0BASE

which has K0BASE defined to 0x80000000, so all we're really doing is
putting a value into a register and then doing work off of that.

Really, I suppose we should just be putting the immediate in the
register, instead of treating it as an address, e.g.

li t1,K0BASE

which does cause the unpredictability problems to go away.

The code that's loading the value is load_register and it's the last set
of code in the function if you want to look.

> ADDI or ADDIU both sign-extend the 16-bit argument, so doing an LA
> with one of those, while possible, is more trouble because the LUI
> argument has to be adjusted up by 1 if the low half of the value to
> load is > 7fff.  With ORI, the argument is zero-extended so it the low
> bits aren't a consideration when picking the LUI argument

Fair enough, it was just a thought. I hate macros and so figuring out
what we really mean when "blah" is annoying.

-eric

Re: [patch] modify crt0.S for 64-bit address targets

[cgd]

> Assuredly this is the case. I think the problem might be from using the
> ori instead of addi for the address.

Perhaps.  For a 32-bit address, I'd expect the address generation to be
simply lui/addiu, or similar.  (in fact, for this, you only need the
lui.)

The only time i'd expect gas would be bothering with shifts at all
is for 64-bit addresses.  (but that's intuition speaking, rather than
code examination.)


cgd

Re: [patch] modify crt0.S for 64-bit address targets

[Eric Christopher]

On Wed, 2005-04-06 at 11:51 -0700, cgd@broadcom.com wrote:
> > Assuredly this is the case. I think the problem might be from using the
> > ori instead of addi for the address.
> 
> Perhaps.  For a 32-bit address, I'd expect the address generation to be
> simply lui/addiu, or similar.  (in fact, for this, you only need the
> lui.)
> 

Agreed.

> The only time i'd expect gas would be bothering with shifts at all
> is for 64-bit addresses.  (but that's intuition speaking, rather than
> code examination.)

Technically this is a 64-bit address target (ABI_EABI and a processor
with 64-bit registers). For this particular address I think all you need
is the addi and the shift, but...

-eric