Re: RFA: MIPS TLS support

[Richard Sandiford <rsandifo at redhat dot com>]

Daniel Jacobowitz <drow@false.org> writes:
> On Wed, Mar 09, 2005 at 10:36:30PM +0000, Richard Sandiford wrote:
>> So mips_symbolic_constant_p seems to be lying when it returns true
>> for zero-offset TLS symbols.  Why not simply:
>> 
>>   - get mips_symbolic_constant_p to return false for TLS constants
>>   - get mips_const_insns to return 0 for TLS constants
>>   - handle TLS symbols specially in mips_legitimize_move and
>>     mips_legitimize_address (as opposed to TLS constants being
>>     special cases of mips_split_p)?
>
> It took me a while to work out what you wanted me to do, I'm afraid,
> but I think I've got it now.  How about this?  Also includes improved
> configure test and untested mips16 support.

Yeah, sorry for not explaining myself very well, and thanks
for perservering!  This patch certainly looks much better to me.

Purely for the record, I was actually thinking that mips_classify_symbol
shouldn't handle TLS operands at all.  I.e. that:

  gcc_assert (GET_CODE (x) == SYMBOL_REF);

should become:

  gcc_assert (GET_CODE (x) == SYMBOL_REF && SYMBOL_REF_TLS_MODEL (x) == 0);

and that the two callers to mips_classify_symbol should deal with the
specialness of TLS symbols directly.  There would then be no need for
the SYMBOL_TLS type, which at the moment has to be handled by functions
that should never actually see it.  I can see there are arguments both
ways though (simpler interfaces, etc.) and I'm happy enough with either.
Your new SYMBOL_TLS version is fine by me.

WRT MIPS16 support: using the generic MIPS reloc machinery should make
any future MIPS16 port fairly easy, but it trivially won't work yet,
because there are no suitable MIPS16 relocs.  (Remember that in the
MIPS16 instruction encoding, 16-bit offsets are split into separate
fields and can't use the same relocs as non-MIPS16 code.)

Until MIPS16 support is added to gas, it would be better to stick
with the other plan and disable TLS for MIPS16 in override_options.
Anyone interested in MIPS16 TLS can add in support later, and can
decide then whether they want to use one of the spare opcodes (of
which there are some) to implement tls_get_tp_[sd]i.

What happens if we advertise TLS support on an OS that doesn't actually
support it (or at least not this flavour of it)?  E.g. IRIX?  IRIX
versions of gas will pass the configure-time assembler check in the
same circumstances as a GNU/Linux gas would.  I'm thinking mostly
of what happens when you use a tls-capable version of gcc to build
some autoconfed package.

I notice there are some cases of #undef HAVE_AS_TLS/#define HAVE_AS_TLS 0
for lynx.

> +#undef TARGET_CANNOT_FORCE_CONST_MEM
> +#define TARGET_CANNOT_FORCE_CONST_MEM mips_tls_operand_p

This isn't enough.  You need to handle symbols+offsets as well.
As it is, the testcase:

    extern __thread int x[2];
    int *foo (void) { return &x[1]; }

will force x+4 into the constant pool.

> +/* Return true if X is a thread-local symbol.  */
> +
> +static bool
> +mips_tls_operand_p (rtx x)
> +{
> +  if (GET_CODE (x) != SYMBOL_REF)
> +    return false;
> +
> +  return !!SYMBOL_REF_TLS_MODEL (x);
> +}

Very minor nit, but since the patch will need another revision anyway,
please write this as:

  return GET_CODE (x) == SYMBOL_REF && SYMBOL_REF_TLS_MODEL (x) == 0;

> @@ -1220,8 +1259,19 @@ mips_symbol_insns (enum mips_symbol_type
>      case SYMBOL_64_HIGH:
>      case SYMBOL_64_MID:
>      case SYMBOL_64_LOW:
> +    case SYMBOL_TPREL:
> +    case SYMBOL_DTPREL:
>        /* Check whether the offset is a 16- or 32-bit value.  */
>        return mips_split_p[type] ? 2 : 1;
> +
> +    case SYMBOL_TLS:
> +      /* We don't treat a bare TLS symbol as a constant.  */
> +      return 0;
> +
> +    case SYMBOL_GOTTPREL:
> +    case SYMBOL_TLSGD:
> +    case SYMBOL_TLSLDM:
> +      return 1;
>      }
>    gcc_unreachable ();
>  }

Please move the last three cases into the same block as the first two.
mips_split_p[] is accurate for all five.

I realise that this wasn't necessarily an RFA patch, but just to
reduce ping-pong, remember that this:

> +/* Emit a call to __tls_get_addr.  */
> [...]
> +static rtx
> +mips_call_tls_get_addr (rtx sym, enum mips_symbol_type type, rtx v0)

will need more commentary.  E.g. it will need to mention SYM, TYPE and V0,
and say what the return value is.

> +  if (Pmode == DImode)
> +    gen_adddi3 (a0, pic_offset_table_rtx, loc);
> +  else
> +    gen_addsi3 (a0, pic_offset_table_rtx, loc);

Nothing actually emits this instruction.  Missing emit_insn?

FWIW, you could use gen_add3_insn (a0, pic_offset_table_rtx, loc)
instead of the si/di conditional.  I don't mind either way.

> +  tga = gen_rtx_MEM (Pmode, mips_tls_symbol);
> +  insn = gen_call_value (v0, tga, const0_rtx, const0_rtx);
> +  insn = emit_call_insn (insn);

The last two lines might as well be combined.  (How's that for
anal micromanagement? ;)

> +/* Generate the code to access a TLS SYMBOL_REF.  */
> +
> +static rtx
> +mips_legitimize_tls_address (rtx loc)

Might as well make the comment:

  Generate code to access TLS SYMBOL_REF LOC.

so that the parameter gets a mention.  Remember to say what the function
returns (an rtx for the symbol value, the rtx being both a legitimate
address and a legitimate move_operand).

> +      eqv = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, const0_rtx),
> +			    UNSPEC_TLS_LDM);
> +      emit_libcall_block (insn, tmp1, v0, eqv);

At first, I was a bit confused about what UNSPEC_TLS_LDM was for, since
this is the only code that uses it.  In the end I assumed it was a case
of: "The libcall needs to be equivalent to _something_, so invent an
unspec to represent its value.  We don't actually care what that value
actually is, only that it is unique."  If that's right, please add a
comment to that effect, either here or above the UNSPEC_TLS_LDM definition
in mips.md.

> +      tmp2 = gen_reg_rtx (Pmode);
> +      tmp2 = mips_unspec_offset_high (tmp2, pic_offset_table_rtx,
> +				      loc, SYMBOL_DTPREL);

There's no need for the gen_reg_rtx here.  Just pass NULL as the first
argument to mips_unspec_offset_high.

> +    case TLS_MODEL_LOCAL_EXEC:
> +      tmp1 = gen_reg_rtx (Pmode);

This register is never used.

> +
> +      if (Pmode == DImode)
> +	emit_insn (gen_tls_get_tp_di (v1));
> +      else
> +	emit_insn (gen_tls_get_tp_si (v1));
> +
> +      tmp1 = gen_reg_rtx (Pmode);
> +      tmp1 = mips_unspec_offset_high (tmp1, v1, loc, SYMBOL_TPREL);

Same comment as for the other mips_unspec_offset_high call.

Otherwise this looks really good, thanks.  I'd be happy to approve
a patch with the comments above addressed. ;)

Richard