mips tls with -mlong64/-mgp32

mips tls with -mlong64/-mgp32

[DJ Delorie]

If TLS pointers are 64 bits but GP registers are 32 bits, the TLS load
needs to be split.  This means we need to support offsetted GOT
addresses, at least for TLS (perhaps for more, but this is the only
one that causes a failure in the testsuite - gcc.dg/tls/nonpic.c)

This results in code like this, which the assembler seems to be ok
with:

        lw      $3,%tprel_lo(e1+4)($2)
        lw      $2,%tprel_lo(e1)($2)

This is similar to the change I made to s390 last October.  Only
lightly tested (else you'd have to wait a week or so for dg results).
Ok?  Should other symbols be similarly offsettable?

	* config/mips/mips.c (mips_symbolic_constant_p): Allow TPREL
	offset-by-4 so we can split DImode GOT loads.

Index: mips.c
===================================================================
--- mips.c	(revision 124522)
+++ mips.c	(working copy)
@@ -1413,20 +1413,23 @@ mips_symbolic_constant_p (rtx x, enum mi
 	 address, and we will apply a 16-bit offset after loading it.
 	 If the symbol is local, the linker should provide enough local
 	 GOT entries for a 16-bit offset, but larger offsets may lead
 	 to GOT overflow.  */
       return SMALL_INT (offset);
 
+    case SYMBOL_TPREL:
+      if (GET_CODE (offset) == CONST_INT
+	  && INTVAL (offset) == 4)
+	return true;
     case SYMBOL_GOT_DISP:
     case SYMBOL_GOTOFF_DISP:
     case SYMBOL_GOTOFF_CALL:
     case SYMBOL_GOTOFF_LOADGP:
     case SYMBOL_TLSGD:
     case SYMBOL_TLSLDM:
     case SYMBOL_DTPREL:
-    case SYMBOL_TPREL:
     case SYMBOL_GOTTPREL:
     case SYMBOL_TLS:
     case SYMBOL_HALF:
       return false;
     }
   gcc_unreachable ();

Re: mips tls with -mlong64/-mgp32

[Daniel Jacobowitz]

On Thu, May 10, 2007 at 04:46:44PM -0400, DJ Delorie wrote:
> 
> If TLS pointers are 64 bits but GP registers are 32 bits, the TLS load
> needs to be split.  This means we need to support offsetted GOT
> addresses, at least for TLS (perhaps for more, but this is the only
> one that causes a failure in the testsuite - gcc.dg/tls/nonpic.c)
> 
> This results in code like this, which the assembler seems to be ok
> with:
> 
>         lw      $3,%tprel_lo(e1+4)($2)
>         lw      $2,%tprel_lo(e1)($2)

Maybe this is a silly question, but what the heck is going on here?

     Pointers are the same size as `long's, or the same size
     as integer registers, whichever is smaller.

Shouldn't the tprel offset be pointer-sized then?  Doesn't seem much
point in loading a bigger offset than fits in a register.

-- 
Daniel Jacobowitz
CodeSourcery

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

> Maybe this is a silly question, but what the heck is going on here?
> 
>      Pointers are the same size as `long's, or the same size
>      as integer registers, whichever is smaller.
> 
> Shouldn't the tprel offset be pointer-sized then?  Doesn't seem much
> point in loading a bigger offset than fits in a register.

Hence, it fails with -mlong64/-mgp32 ;-)

oh, and -mabi=eabi.  Sorry about omitting that.  Anyway, the GOT has
64 bit values in it, but registers are 32 bits.

Re: mips tls with -mlong64/-mgp32

[Daniel Jacobowitz]

On Thu, May 10, 2007 at 05:27:38PM -0400, DJ Delorie wrote:
> oh, and -mabi=eabi.  Sorry about omitting that.  Anyway, the GOT has
> 64 bit values in it, but registers are 32 bits.

Then there's no way the upper half of the offset you just loaded will
ever be used, is there?

-- 
Daniel Jacobowitz
CodeSourcery

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

> Then there's no way the upper half of the offset you just loaded will
> ever be used, is there?

Ok, I'm not a mips expert, I'm just going by what's failing.  It's
trying to load a 64 bit pointer from the GOT.  Don't know why, don't
care ;-) The 64 bit load gets split into two 32 bit loads.  I can only
assume from this, that pointers are 64 bits in this case, even though
registers are 32 bits.

./cc1 -Os nonpic-1.i -meb -quiet -mabi=eabi -mlong64 -mgp32 -ftls-model=initial-exec

Re: mips tls with -mlong64/-mgp32

[Daniel Jacobowitz]

On Thu, May 10, 2007 at 09:07:17PM -0400, DJ Delorie wrote:
> 
> > Then there's no way the upper half of the offset you just loaded will
> > ever be used, is there?
> 
> Ok, I'm not a mips expert, I'm just going by what's failing.  It's
> trying to load a 64 bit pointer from the GOT.  Don't know why, don't
> care ;-) The 64 bit load gets split into two 32 bit loads.  I can only
> assume from this, that pointers are 64 bits in this case, even though
> registers are 32 bits.

That's the point where I'll punt over to Richard and Eric, but if you
need to load it into $2 and $3, there's no way you're going to load
from it :-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: mips tls with -mlong64/-mgp32

[Rask Ingemann Lambertsen]

On Thu, May 10, 2007 at 09:07:17PM -0400, DJ Delorie wrote:
> 
> > Then there's no way the upper half of the offset you just loaded will
> > ever be used, is there?
> 
> Ok, I'm not a mips expert, I'm just going by what's failing.  It's
> trying to load a 64 bit pointer from the GOT.  Don't know why, don't
> care ;-)

   I'm pretty sure some targets have the ability to place small objects
directly in the GOT. Google turned up empty, though. Do you know that it is
a pointer rather than e.g. a 64-bit integer?

-- 
Rask Ingemann Lambertsen

Re: mips tls with -mlong64/-mgp32

[Andrew Pinski]

On 5/11/07, Rask Ingemann Lambertsen <rask@sygehus.dk> wrote:
>    I'm pretty sure some targets have the ability to place small objects
> directly in the GOT. Google turned up empty, though. Do you know that it is
> a pointer rather than e.g. a 64-bit integer?

If you consider the TOC in the PowerOpen ABI (the powerPC AIX ABI and
powerpc64 linux abi is based on that ABI)  the same as the GOT, then
yes.

--Pinski

Re: mips tls with -mlong64/-mgp32

[Daniel Jacobowitz]

On Fri, May 11, 2007 at 09:22:00AM +0200, Rask Ingemann Lambertsen wrote:
>    I'm pretty sure some targets have the ability to place small objects
> directly in the GOT. Google turned up empty, though. Do you know that it is
> a pointer rather than e.g. a 64-bit integer?

It's a TLS offset.  If it's not pointer-sized, that probably means I
goofed when implementing MIPS TLS.

DJ, does this go in 32-bit ELF or 64-bit ELF?

-- 
Daniel Jacobowitz
CodeSourcery

Re: mips tls with -mlong64/-mgp32

[Richard Sandiford]

DJ Delorie <dj@redhat.com> writes:
>> Maybe this is a silly question, but what the heck is going on here?
>> 
>>      Pointers are the same size as `long's, or the same size
>>      as integer registers, whichever is smaller.
>> 
>> Shouldn't the tprel offset be pointer-sized then?  Doesn't seem much
>> point in loading a bigger offset than fits in a register.
>
> Hence, it fails with -mlong64/-mgp32 ;-)

I don't understand.  Like Dan says, pointers are 32 bits wide for -mgp32,
regardless of whether you use -mlong64 or not.  This is an IP32, L64 ABI.

> oh, and -mabi=eabi.  Sorry about omitting that.  Anyway, the GOT has
> 64 bit values in it, but registers are 32 bits.

Hmm, which GOT values do you mean?  Pointers in the GOT ought to be 32 bits.

Richard

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

> DJ, does this go in 32-bit ELF or 64-bit ELF?

mips64vrel-elf

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

> Hmm, which GOT values do you mean?  Pointers in the GOT ought to be 32 bits.

mips64vrel-elf
gcc.dg/tls/nonpic-1.c
./cc1 nonpic-1.i -meb -quiet -mabi=eabi -mlong64 -mgp32 -ftls-model=initial-exec

Trimmed test case:

extern __thread long e1;
long ge1 (void)
{
  return e1;
}

Re: mips tls with -mlong64/-mgp32

[Richard Sandiford]

DJ Delorie <dj@redhat.com> writes:
>> Hmm, which GOT values do you mean?  Pointers in the GOT ought to be 32 bits.
>
> mips64vrel-elf
> gcc.dg/tls/nonpic-1.c
> ./cc1 nonpic-1.i -meb -quiet -mabi=eabi -mlong64 -mgp32 -ftls-model=initial-exec
>
> Trimmed test case:
>
> extern __thread long e1;
> long ge1 (void)
> {
>   return e1;
> }

That doesn't really answer the question though. ;)  I don't have a
mips64vr-elf compiler handy, so it'd be helpful if you could just
tell me what sort of GOT entry is 64 bits wide and how it ends up
so wide.

Richard

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

The original instruction is this one:

(insn 8 7 19 nonpic-1.i:5 (set (reg:DI 2 $2 [orig:193 D.1563 ] [193])
        (mem/c/i:DI (lo_sum:SI (reg:SI 2 $2 [196])
                (const:SI (unspec:SI [
                            (symbol_ref:SI ("e1") [flags 0x58] <var_decl 0xb7f4a000 e1>)
                        ] 114))) [0 e1+0 S8 A64])) 208 {*movdi_32bit} (nil)
    (nil))

With my patch, the resulting .s file contains this (minus overhead):

ge1:
        addiu   $sp,$sp,-8
        sw      $fp,0($sp)
        move    $fp,$sp
        rdhwr   $3,$29
        lui     $2,%tprel_hi(e1)
        addu    $2,$2,$3
        lw      $3,%tprel_lo(e1+4)($2)
        lw      $2,%tprel_lo(e1)($2)
        move    $sp,$fp
        lw      $fp,0($sp)
        addiu   $sp,$sp,8
        j       $31
        nop

Hmmm... looking at it given what you've all said, I guess this isn't a
GOT access at all, but a TLS data access?

(That's why it's important to run the test case, and not rely on DJ's
incoherent babbling ;)

Re: mips tls with -mlong64/-mgp32

[Paolo Bonzini]

> ge1:
>         addiu   $sp,$sp,-8
>         sw      $fp,0($sp)
>         move    $fp,$sp
>         rdhwr   $3,$29
>         lui     $2,%tprel_hi(e1)
>         addu    $2,$2,$3
>         lw      $3,%tprel_lo(e1+4)($2)
>         lw      $2,%tprel_lo(e1)($2)
>         move    $sp,$fp
>         lw      $fp,0($sp)
>         addiu   $sp,$sp,8
>         j       $31
>         nop
> 
> Hmmm... looking at it given what you've all said, I guess this isn't a
> GOT access at all, but a TLS data access?

Definitely :-P

It's just reading the 8 bytes at HWR29+%tprel_lo(e1)+$tprel_hi(e1)<<16 
into the ($2,$3) pair, right?

Paolo

Re: mips tls with -mlong64/-mgp32

[Richard Sandiford]

DJ Delorie <dj@redhat.com> writes:
> Hmmm... looking at it given what you've all said, I guess this isn't a
> GOT access at all, but a TLS data access?

Ah.  Yes, indeed. ;)

So there's nothing special about an offset of 4.  I think you should
just be able to make mips_symbolic_constant_p handle SYMBOL_DTPREL
and SYMBOL_TPREL like SYMBOL_GENERAL.

Richard

Re: mips tls with -mlong64/-mgp32

[Daniel Jacobowitz]

On Fri, May 11, 2007 at 02:19:13PM +0100, Richard Sandiford wrote:
> DJ Delorie <dj@redhat.com> writes:
> > Hmmm... looking at it given what you've all said, I guess this isn't a
> > GOT access at all, but a TLS data access?
> 
> Ah.  Yes, indeed. ;)
> 
> So there's nothing special about an offset of 4.  I think you should
> just be able to make mips_symbolic_constant_p handle SYMBOL_DTPREL
> and SYMBOL_TPREL like SYMBOL_GENERAL.

Isn't there an overflow problem here?  If we use %tprel_hi(e1) and
%tprel_lo(e1) nothing guarantees that %tprel_lo(e1+4) fits in the load.
I don't think you can offset this at all.

-- 
Daniel Jacobowitz
CodeSourcery

Re: mips tls with -mlong64/-mgp32

[Richard Sandiford]

Daniel Jacobowitz <drow@false.org> writes:
> On Fri, May 11, 2007 at 02:19:13PM +0100, Richard Sandiford wrote:
>> DJ Delorie <dj@redhat.com> writes:
>> > Hmmm... looking at it given what you've all said, I guess this isn't a
>> > GOT access at all, but a TLS data access?
>> 
>> Ah.  Yes, indeed. ;)
>> 
>> So there's nothing special about an offset of 4.  I think you should
>> just be able to make mips_symbolic_constant_p handle SYMBOL_DTPREL
>> and SYMBOL_TPREL like SYMBOL_GENERAL.
>
> Isn't there an overflow problem here?  If we use %tprel_hi(e1) and
> %tprel_lo(e1) nothing guarantees that %tprel_lo(e1+4) fits in the load.
> I don't think you can offset this at all.

The address is doubleword aligned, so it is OK.  But I'd just remembered
that the REL TLS relocs have no %hi/%lo addend pairing, so my suggestion
was indeed wrong on its own.  I suppose that even if we extended the
relocs, we'd still have to cope with old binutils.  Ugh.

We should already treat DImode %tprel addresses as non-offsettable,
so with any luck, the fix should simply be to change "m" to "o"
in the d<->m !TARGET_64BIT movdi patterns.

Richard

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

> We should already treat DImode %tprel addresses as non-offsettable,
> so with any luck, the fix should simply be to change "m" to "o" in
> the d<->m !TARGET_64BIT movdi patterns.

FYI a simple m->o change doesn't fix the bug.

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

> The address is doubleword aligned, so it is OK.  But I'd just remembered
> that the REL TLS relocs have no %hi/%lo addend pairing, so my suggestion
> was indeed wrong on its own.  I suppose that even if we extended the
> relocs, we'd still have to cope with old binutils.  Ugh.

I tested my original patch (with DTPREL) with 80 different mips
multilibs (gcc's testsuite), and it seems to work OK.  The m->o patch
didn't.

Re: mips tls with -mlong64/-mgp32

[Richard Sandiford]

DJ Delorie <dj@redhat.com> writes:
>> The address is doubleword aligned, so it is OK.  But I'd just remembered
>> that the REL TLS relocs have no %hi/%lo addend pairing, so my suggestion
>> was indeed wrong on its own.  I suppose that even if we extended the
>> relocs, we'd still have to cope with old binutils.  Ugh.
>
> I tested my original patch (with DTPREL) with 80 different mips
> multilibs (gcc's testsuite), and it seems to work OK.  The m->o patch
> didn't.

But the original patch was wrong.  You can't assume anything +4 is
valid.  It is OK in your particular case because the decl you're
using it for is 8-byte aligned, but not all TLS data is 8-byte aligned.

I suppose that the m->o thing didn't work because the address isn't
considered to be mode-dependent.  We should either make it mode-dependent,
or check whether the offset is within the alignment of the SYMBOL_REF_DECL.
The latter should be enough for TLS symbols, and is probably better.

richard

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

> But the original patch was wrong.

I think the original patch was right (or at least right-ish), but
caused other functions to have mis-assumptions and thus become wrong.
We certainly *can* support offsetted TPREL and DTPREL symbols, as the
assembler, linker, and chip allow them.  Whether that causes other
functions to do the wrong thing *semantically* is a different issue.

Recall the function was mips_symbolic_constant_p().

> You can't assume anything +4 is valid.  It is OK in your particular
> case because the decl you're using it for is 8-byte aligned, but not
> all TLS data is 8-byte aligned.

Anything with lesser alignment could still be offset by 4 and still be
valid, assuming we don't care about "user stupidity" in addressing
beyond the end of memory.

Besides, why is this different than with non-tls symbols?
SYMBOL_SMALL_DATA allows for arbitrary offsets within an object, TLS
data is no different.  Perhaps we should figure out how to use
offset_within_block_p() ?

> I suppose that the m->o thing didn't work because the address isn't
> considered to be mode-dependent.  We should either make it mode-dependent,
> or check whether the offset is within the alignment of the SYMBOL_REF_DECL.
> The latter should be enough for TLS symbols, and is probably better.

Unfortunately, mips_symbolic_constant_p() doesn't know what type of
memory reference (or if there's a memory reference at all) will be
done.  No matter how we decide that a split is valid, we still need to
allow the offsetted tls symbol.

Re: mips tls with -mlong64/-mgp32

[Richard Sandiford]

DJ Delorie <dj@redhat.com> writes:
>> But the original patch was wrong.
>
> I think the original patch was right (or at least right-ish), but
> caused other functions to have mis-assumptions and thus become wrong.
> We certainly *can* support offsetted TPREL and DTPREL symbols, as the
> assembler, linker, and chip allow them.  Whether that causes other
> functions to do the wrong thing *semantically* is a different issue.

Unfortunately, the problem is that the linker _doesn't_ allow them.
The relocs aren't defined that way.  That's what I meant by:

Richard Sandiford writes:
> But I'd just remembered that the REL TLS relocs have no %hi/%lo addend
> pairing, so my suggestion was indeed wrong on its own.  I suppose that
> even if we extended the relocs, we'd still have to cope with old
> binutils.  Ugh.

In other words, you cannot have %tprel_hi(foo+4) and %tprel_lo(foo+4) if
foo as only 4-byte-aligned, as the REL relocs are not defined to carry
the LO addition to the HI addition.  (In REL relocs, the low 16 bits
of the addend are stored with the %tprel_lo and the high 16 bits with
the %tprel_hi.)

Consider foo=0x7ff8, for example.  We want %tprel_hi(foo+4) to be 0x8000
and %tprel_lo(foo+4) to be 0.

This is different from normal %hi and %lo, which do define such a carry.

> Besides, why is this different than with non-tls symbols?
> SYMBOL_SMALL_DATA allows for arbitrary offsets within an object, TLS
> data is no different.  Perhaps we should figure out how to use
> offset_within_block_p() ?

Small data is also special because it uses a single relocation,
not a hi/lo pair.

>> I suppose that the m->o thing didn't work because the address isn't
>> considered to be mode-dependent.  We should either make it mode-dependent,
>> or check whether the offset is within the alignment of the SYMBOL_REF_DECL.
>> The latter should be enough for TLS symbols, and is probably better.
>
> Unfortunately, mips_symbolic_constant_p() doesn't know what type of
> memory reference (or if there's a memory reference at all) will be
> done.  No matter how we decide that a split is valid, we still need to
> allow the offsetted tls symbol.

It isn't the type of the memory reference that matters; it's the
alignment of the SYMBOL_REF_DECL, and mips_symbolic_constant_p does
have access to that.  If we have foo+4, and foo is 8-byte-aligned,
we know that %tprel_lo(foo+4) will not carry.

Richard.

Re: mips tls with -mlong64/-mgp32

[Richard Sandiford]

Richard Sandiford <richard@codesourcery.com> writes:
> Consider foo=0x7ff8, for example.  We want %tprel_hi(foo+4) to be 0x8000
> and %tprel_lo(foo+4) to be 0.

Er, I mean: we want %tprel_hi(foo+4) to be 1 and %tprel_lo(foo+4) to
be 0x8000.

Richard

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

> Richard Sandiford <richard@codesourcery.com> writes:
> > Consider foo=0x7ff8, for example.  We want %tprel_hi(foo+4) to be 0x8000
> > and %tprel_lo(foo+4) to be 0.
> 
> Er, I mean: we want %tprel_hi(foo+4) to be 1 and %tprel_lo(foo+4) to
> be 0x8000.

foo=0x7ffc then ;-)

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

So the solution is... what?  If we see a DImode TLS memory access and
we know we're going to split it, force the address into a register?

Re: mips tls with -mlong64/-mgp32

[Richard Sandiford]

DJ Delorie <dj@redhat.com> writes:
> So the solution is... what?  If we see a DImode TLS memory access and
> we know we're going to split it, force the address into a register?

No, the solution is to...

> check whether the offset is within the alignment of the SYMBOL_REF_DECL.

Richard

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

> It isn't the type of the memory reference that matters; it's the
> alignment of the SYMBOL_REF_DECL, and mips_symbolic_constant_p does
> have access to that.  If we have foo+4, and foo is 8-byte-aligned,
> we know that %tprel_lo(foo+4) will not carry.

Or in general, if the offset is less than the alignment?

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

> > check whether the offset is within the alignment of the SYMBOL_REF_DECL.

Like this?

Index: mips.c
===================================================================
--- mips.c	(revision 124810)
+++ mips.c	(working copy)
@@ -1413,20 +1413,39 @@ mips_symbolic_constant_p (rtx x, enum mi
 	 address, and we will apply a 16-bit offset after loading it.
 	 If the symbol is local, the linker should provide enough local
 	 GOT entries for a 16-bit offset, but larger offsets may lead
 	 to GOT overflow.  */
       return SMALL_INT (offset);
 
+    case SYMBOL_TPREL:
+    case SYMBOL_DTPREL:
+      {
+	/* If for some reason we can't get the alignment for the
+	   symbol, initializing this to zero means we won't accept any
+	   offset.  */
+	HOST_WIDE_INT align = 0;
+	tree t;
+
+	/* Get the alignment of the symbol we're referring to.  */
+	t = SYMBOL_REF_DECL (XVECEXP (x, 0, 0));
+	if (t)
+	  align = DECL_ALIGN_UNIT (t);
+
+	/* We accept offsets within the symbol's alignment, but not
+	   beyond, because the linker can't handle TPREL hi/lo relocs
+	   that involve carries.  */
+	if (GET_CODE (offset) == CONST_INT
+	    && INTVAL (offset) < align)
+	  return true;
+      }
     case SYMBOL_GOT_DISP:
     case SYMBOL_GOTOFF_DISP:
     case SYMBOL_GOTOFF_CALL:
     case SYMBOL_GOTOFF_LOADGP:
     case SYMBOL_TLSGD:
     case SYMBOL_TLSLDM:
-    case SYMBOL_DTPREL:
-    case SYMBOL_TPREL:
     case SYMBOL_GOTTPREL:
     case SYMBOL_TLS:
     case SYMBOL_HALF:
       return false;
     }
   gcc_unreachable ();

Re: mips tls with -mlong64/-mgp32

[Richard Sandiford]

DJ Delorie <dj@redhat.com> writes:
>> > check whether the offset is within the alignment of the SYMBOL_REF_DECL.
>
> Like this?
>
> Index: mips.c
> ===================================================================
> --- mips.c	(revision 124810)
> +++ mips.c	(working copy)
> @@ -1413,20 +1413,39 @@ mips_symbolic_constant_p (rtx x, enum mi
>  	 address, and we will apply a 16-bit offset after loading it.
>  	 If the symbol is local, the linker should provide enough local
>  	 GOT entries for a 16-bit offset, but larger offsets may lead
>  	 to GOT overflow.  */
>        return SMALL_INT (offset);
>  
> +    case SYMBOL_TPREL:
> +    case SYMBOL_DTPREL:
> +      {
> +	/* If for some reason we can't get the alignment for the
> +	   symbol, initializing this to zero means we won't accept any
> +	   offset.  */
> +	HOST_WIDE_INT align = 0;
> +	tree t;
> +
> +	/* Get the alignment of the symbol we're referring to.  */
> +	t = SYMBOL_REF_DECL (XVECEXP (x, 0, 0));

Although I agree that XVECEXP is right, it's a bit subtle.
Can you just change:

  if (UNSPEC_ADDRESS_P (x))
    *symbol_type = UNSPEC_ADDRESS_TYPE (x);

to:

  if (UNSPEC_ADDRESS_P (x))
    {
      *symbol_type = UNSPEC_ADDRESS_TYPE (x);
      x = UNSPEC_ADDRESS (x);
    }

instead, and use "x" here?  That'll make the whole case statement
more robust.

> +	if (t)
> +	  align = DECL_ALIGN_UNIT (t);
> +
> +	/* We accept offsets within the symbol's alignment, but not
> +	   beyond, because the linker can't handle TPREL hi/lo relocs
> +	   that involve carries.  */
> +	if (GET_CODE (offset) == CONST_INT
> +	    && INTVAL (offset) < align)
> +	  return true;
> +      }

Looks good otherwise.  Sorry to nitpick, but can you split it out into a
separate function for clarity?  Something like:

/* Return true if OFFSET is in the range [0, ALIGN), where ALIGN
   is the alignment of SYMBOL_REF X.  */

static bool
mips_offset_within_alignment_p (rtx x, HOST_WIDE_INT offset)

Can you also add a comment above the caller, like so:

/* There is no carry between the HI and LO REL relocations, so the
   offset is only valid if we know it won't lead to such a carry.  */

Also, you don't need to check whether GET_CODE (offset) == CONST_INT;
it always is.  (For avoidance of doubt, I don't think there's a strong
case for it being good defensively.  The function that provides OFFSET
is specifically defined to return a CONST_INT, and INTVAL will check
that anyway when RTL checking is enabled.)

Richard

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

Like this?

	* config/mips/mips.c (mips_offset_within_alignment_p): New.
	(mips_symbolic_constant_p): Call it for TPREL and DTPREL symbols.

Index: mips.c
===================================================================
--- mips.c	(revision 124810)
+++ mips.c	(working copy)
@@ -409,12 +409,13 @@ static rtx mips_expand_builtin_compare (
 					enum insn_code, enum mips_fp_condition,
 					rtx, tree);
 static rtx mips_expand_builtin_bposge (enum mips_builtin_type, rtx);
 static void mips_encode_section_info (tree, rtx, int);
 static void mips_extra_live_on_entry (bitmap);
 static int mips_mode_rep_extended (enum machine_mode, enum machine_mode);
+static bool mips_offset_within_alignment_p (rtx, HOST_WIDE_INT);
 
 /* Structure to be filled in by compute_frame_size with register
    save masks, and offsets for the current function.  */
 
 struct mips_frame_info GTY(())
 {
@@ -1347,24 +1348,49 @@ mips_classify_symbol (rtx x)
       return SYMBOL_GOT_PAGE_OFST;
     }
 
   return SYMBOL_GENERAL;
 }
 
+/* Returns true if OFFSET is within the range [0, ALIGN), where ALIGN
+   is the alignment (in bytes) of SYMBOL_REF X.  */
+
+static bool
+mips_offset_within_alignment_p (rtx x, HOST_WIDE_INT offset)
+{
+  /* If for some reason we can't get the alignment for the
+     symbol, initializing this to zero means we won't accept any
+     offset.  */
+  HOST_WIDE_INT align = 0;
+  tree t;
+
+  /* Get the alignment of the symbol we're referring to.  */
+  t = SYMBOL_REF_DECL (x);
+  if (t)
+    align = DECL_ALIGN_UNIT (t);
+
+  if (offset < align)
+    return true;
+  return false;
+}
+
 /* Return true if X is a symbolic constant that can be calculated in
    the same way as a bare symbol.  If it is, store the type of the
    symbol in *SYMBOL_TYPE.  */
 
 bool
 mips_symbolic_constant_p (rtx x, enum mips_symbol_type *symbol_type)
 {
   rtx offset;
 
   split_const (x, &x, &offset);
   if (UNSPEC_ADDRESS_P (x))
-    *symbol_type = UNSPEC_ADDRESS_TYPE (x);
+    {
+      *symbol_type = UNSPEC_ADDRESS_TYPE (x);
+      x = UNSPEC_ADDRESS (x);
+    }
   else if (GET_CODE (x) == SYMBOL_REF || GET_CODE (x) == LABEL_REF)
     {
       *symbol_type = mips_classify_symbol (x);
       if (*symbol_type == SYMBOL_TLS)
 	return false;
     }
@@ -1413,20 +1439,26 @@ mips_symbolic_constant_p (rtx x, enum mi
 	 address, and we will apply a 16-bit offset after loading it.
 	 If the symbol is local, the linker should provide enough local
 	 GOT entries for a 16-bit offset, but larger offsets may lead
 	 to GOT overflow.  */
       return SMALL_INT (offset);
 
+    case SYMBOL_TPREL:
+    case SYMBOL_DTPREL:
+      /* There is no carry between the HI and LO REL relocations, so the
+	 offset is only valid if we know it won't lead to such a carry.  */
+      if (mips_offset_within_alignment_p (x, INTVAL (offset)))
+	return true;
+      /* Fall through.  */
+
     case SYMBOL_GOT_DISP:
     case SYMBOL_GOTOFF_DISP:
     case SYMBOL_GOTOFF_CALL:
     case SYMBOL_GOTOFF_LOADGP:
     case SYMBOL_TLSGD:
     case SYMBOL_TLSLDM:
-    case SYMBOL_DTPREL:
-    case SYMBOL_TPREL:
     case SYMBOL_GOTTPREL:
     case SYMBOL_TLS:
     case SYMBOL_HALF:
       return false;
     }
   gcc_unreachable ();

Re: mips tls with -mlong64/-mgp32

[Richard Sandiford]

DJ Delorie <dj@redhat.com> writes:
> +/* Returns true if OFFSET is within the range [0, ALIGN), where ALIGN
> +   is the alignment (in bytes) of SYMBOL_REF X.  */

(thanks for adding the "(in bytes)" btw.)

> +static bool
> +mips_offset_within_alignment_p (rtx x, HOST_WIDE_INT offset)
> +{
> +  /* If for some reason we can't get the alignment for the
> +     symbol, initializing this to zero means we won't accept any
> +     offset.  */
> +  HOST_WIDE_INT align = 0;

I suppose with the way I've speced the function, this ought to be:

  /* If for some reason we can't get the alignment for the
     symbol, initializing this to one means we'll only accept
     a zero offset.  */
  HOST_WIDE_INT align = 1;

> +  tree t;
> +
> +  /* Get the alignment of the symbol we're referring to.  */
> +  t = SYMBOL_REF_DECL (x);
> +  if (t)
> +    align = DECL_ALIGN_UNIT (t);
> +
> +  if (offset < align)
> +    return true;
> +  return false;

This should check for negative offsets too.  Just change the last
three statements to:

   return offset >= 0 && offset < align;

> @@ -1413,20 +1439,26 @@ mips_symbolic_constant_p (rtx x, enum mi
>  	 address, and we will apply a 16-bit offset after loading it.
>  	 If the symbol is local, the linker should provide enough local
>  	 GOT entries for a 16-bit offset, but larger offsets may lead
>  	 to GOT overflow.  */
>        return SMALL_INT (offset);
>  
> +    case SYMBOL_TPREL:
> +    case SYMBOL_DTPREL:
> +      /* There is no carry between the HI and LO REL relocations, so the
> +	 offset is only valid if we know it won't lead to such a carry.  */
> +      if (mips_offset_within_alignment_p (x, INTVAL (offset)))
> +	return true;
> +      /* Fall through.  */
> +
>      case SYMBOL_GOT_DISP:
>      case SYMBOL_GOTOFF_DISP:
>      case SYMBOL_GOTOFF_CALL:
>      case SYMBOL_GOTOFF_LOADGP:
>      case SYMBOL_TLSGD:
>      case SYMBOL_TLSLDM:
> -    case SYMBOL_DTPREL:
> -    case SYMBOL_TPREL:
>      case SYMBOL_GOTTPREL:
>      case SYMBOL_TLS:
>      case SYMBOL_HALF:
>        return false;
>      }
>    gcc_unreachable ();

This isn't right, you fall through a gcc_unreachable ().
Just change it to:

      /* There is no carry between the HI and LO REL relocations, so the
	 offset is only valid if we know it won't lead to such a carry.  */
      return mips_offset_within_alignment_p (x, INTVAL (offset));

OK with those changes, if it survives testing.

Thanks,
Richard

Re: mips tls with -mlong64/-mgp32

[Richard Sandiford]

Richard Sandiford <richard@codesourcery.com> writes:
> This isn't right, you fall through a gcc_unreachable ().

Sorry, you don't of course ;)  But please still...

> Just change it to:
>
>       /* There is no carry between the HI and LO REL relocations, so the
> 	 offset is only valid if we know it won't lead to such a carry.  */
>       return mips_offset_within_alignment_p (x, INTVAL (offset));

...and kill the fallthrough.

Richard

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

> >        return false;
> >      }
> >    gcc_unreachable ();
> 
> This isn't right, you fall through a gcc_unreachable ().

Technically, I fall through to a "return false;" but I changed it
anyway.

Patch checked in.  Thanks!  Do we want to back-port it to 4.2 ?

Re: mips tls with -mlong64/-mgp32

[Richard Sandiford]

DJ Delorie <dj@redhat.com> writes:
> Patch checked in.  Thanks!  Do we want to back-port it to 4.2 ?

Unsure.  It isn't a regression AFAIK, so probably not.

Richard

Re: mips tls with -mlong64/-mgp32

[Richard Sandiford]

DJ Delorie <dj@redhat.com> writes:
> Patch checked in.  Thanks!

Looking at the commit, I noticed that you didn't make one of the other
nitpicky changes I asked for and that you didn't fully update one of the
comments.  (Perhaps you thought "zero offset" wasn't needed because you
thought zero didn't count as an offset, and that "any offset" was therefore
still right.  But I think it's confusing to say that we don't "accept
any offset" while still returning true for one value of "offset".)

I also noticed you'd changed my "Return" to "Returns".  (It was in
the patch, but I missed it.)  I think the former is more common in gcc,
and certainly the style used in the mips.c, so I changed it back while
I was there.

No big deal of course.  I feel bad writing this at all, but since we're
supposed to send each change we make to gcc-patches...  (And it took
longer to write this message than to make the change.)

Richard


gcc/
	* config/mips/mips.c (mips_offset_within_alignment_p): Tweak comment.
	Use a single return statement.

Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	(revision 124852)
+++ gcc/config/mips/mips.c	(working copy)
@@ -1351,15 +1351,15 @@ mips_classify_symbol (rtx x)
   return SYMBOL_GENERAL;
 }
 
-/* Returns true if OFFSET is within the range [0, ALIGN), where ALIGN
+/* Return true if OFFSET is within the range [0, ALIGN), where ALIGN
    is the alignment (in bytes) of SYMBOL_REF X.  */
 
 static bool
 mips_offset_within_alignment_p (rtx x, HOST_WIDE_INT offset)
 {
   /* If for some reason we can't get the alignment for the
-     symbol, initializing this to one means we won't accept any
-     offset.  */
+     symbol, initializing this to one means we will only accept
+     a zero offset.  */
   HOST_WIDE_INT align = 1;
   tree t;
 
@@ -1368,9 +1368,7 @@ mips_offset_within_alignment_p (rtx x, H
   if (t)
     align = DECL_ALIGN_UNIT (t);
 
-  if (offset >= 0 && offset < align)
-    return true;
-  return false;
+  return offset >= 0 && offset < align;
 }
 
 /* Return true if X is a symbolic constant that can be calculated in

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

> Unsure.  It isn't a regression AFAIK, so probably not.

Hmmm... we found it because it *was* a regression against our
4.1-based internal source tree.  I haven't checked the fsf releases
though.

Re: mips tls with -mlong64/-mgp32

[DJ Delorie]

> Looking at the commit, I noticed that you didn't make one of the
> other nitpicky changes I asked for and that you didn't fully update
> one of the comments.

I thought I did, but I just overlooked those changes.  Sorry.