Re: [ARM/FDPIC 10/21] [ARM] FDPIC: Implement legitimize_tls_address_fdpic

[Christophe Lyon <christophe dot lyon at linaro dot org>]

On 8 June 2018 at 12:41, Kyrill  Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
> Hi Christophe,
>
>
> On 25/05/18 09:03, Christophe Lyon wrote:
>>
>> Support additional relocations: TLS_GD32_FDPIC, TLS_LDM32_FDPIC, and
>> TLS_IE32_FDPIC.
>>
>> We do not support the GNU2 TLS dialect.
>>
>> 2018-XX-XX  Christophe Lyon  <christophe.lyon@st.com>
>>         Mickaël Guêné <mickael.guene@st.com>
>>
>>         gcc/
>>         * config/arm/arm.c (tls_reloc): Add TLS_GD32_FDPIC,
>>         TLS_LDM32_FDPIC and TLS_IE32_FDPIC.
>>         (arm_call_tls_get_addr_fdpic): New.
>>         (legitimize_tls_address_fdpic): New.
>>         (legitimize_tls_address_not_fdpic): New.
>>         (legitimize_tls_address): Add FDPIC support.
>>         (arm_emit_tls_decoration): Likewise.
>>
>> Change-Id: I4ea5034ff654540c4658d0a79fb92f70550cdf4a
>>
>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>> index 2434602..20b8f66 100644
>> --- a/gcc/config/arm/arm.c
>> +++ b/gcc/config/arm/arm.c
>> @@ -2373,9 +2373,12 @@ char arm_arch_name[] = "__ARM_ARCH_PROFILE__";
>>
>>  enum tls_reloc {
>>    TLS_GD32,
>> +  TLS_GD32_FDPIC,
>>    TLS_LDM32,
>> +  TLS_LDM32_FDPIC,
>>    TLS_LDO32,
>>    TLS_IE32,
>> +  TLS_IE32_FDPIC,
>>    TLS_LE32,
>>    TLS_DESCSEQ  /* GNU scheme */
>>  };
>> @@ -8681,6 +8684,30 @@ load_tls_operand (rtx x, rtx reg)
>>  }
>>
>>  static rtx_insn *
>> +arm_call_tls_get_addr_fdpic (rtx x, rtx reg, rtx *valuep, int reloc)
>> +{
>
>
> Please add a function comment
>
>> +  rtx sum;
>> +
>> +  gcc_assert (reloc != TLS_DESCSEQ);
>> +  start_sequence ();
>> +
>> +  sum = gen_rtx_UNSPEC (Pmode,
>> +                       gen_rtvec (2, x, GEN_INT (reloc)),
>> +                       UNSPEC_TLS);
>> +  reg = load_tls_operand (sum, reg);
>> +  emit_insn (gen_addsi3 (reg, reg, gen_rtx_REG (Pmode, 9)));
>> +
>> +  *valuep = emit_library_call_value (get_tls_get_addr (), NULL_RTX,
>> +                                    LCT_PURE, /* LCT_CONST?  */
>> +                                    Pmode, reg, Pmode);
>
>
> I prefer to avoid comments with such question marks. Is there some ambiguity
> on which should be used?
>
>> +
>> +  rtx_insn *insns = get_insns ();
>> +  end_sequence ();
>> +
>> +  return insns;
>> +}
>> +
>> +static rtx_insn *
>>  arm_call_tls_get_addr (rtx x, rtx reg, rtx *valuep, int reloc)
>>  {
>>    rtx label, labelno, sum;
>> @@ -8736,8 +8763,84 @@ arm_tls_descseq_addr (rtx x, rtx reg)
>>    return reg;
>>  }
>>
>> -rtx
>> -legitimize_tls_address (rtx x, rtx reg)
>> +static rtx
>> +legitimize_tls_address_fdpic (rtx x, rtx reg)
>> +{
>
>
> Please add a function comment, even if it's just a simple one.
>
>> +    rtx dest, ret, eqv, addend, sum, tp;
>> +    rtx_insn *insns;
>> +    unsigned int model = SYMBOL_REF_TLS_MODEL (x);
>> +
>> +    switch (model)
>> +      {
>> +      case TLS_MODEL_GLOBAL_DYNAMIC:
>> +       if (TARGET_GNU2_TLS)
>> +         {
>> +           abort ();
>> +         }
>
>
> Use gcc_unreachable ().
>
>> +       else
>> +         {
>> +           /* Original scheme.  */
>> +           insns = arm_call_tls_get_addr_fdpic (x, reg, &ret,
>> TLS_GD32_FDPIC);
>> +           dest = gen_reg_rtx (Pmode);
>> +           emit_libcall_block (insns, dest, ret, x);
>> +         }
>> +       return dest;
>> +       break;
>> +
>> +      case TLS_MODEL_LOCAL_DYNAMIC:
>> +       if (TARGET_GNU2_TLS)
>> +         {
>> +           abort ();
>> +         }
>
>
> Likewise.
>
>> +       else
>> +         {
>> +           insns = arm_call_tls_get_addr_fdpic (x, reg, &ret,
>> TLS_LDM32_FDPIC);
>> +           /* Attach a unique REG_EQUIV, to allow the RTL optimizers to
>> +              share the LDM result with other LD model accesses.  */
>> +           eqv = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, const1_rtx),
>> +                                 UNSPEC_TLS);
>> +           dest = gen_reg_rtx (Pmode);
>> +           emit_libcall_block (insns, dest, ret, eqv);
>> +
>> +           /* Load the addend.  */
>> +           addend = gen_rtx_UNSPEC (Pmode,
>> +                                    gen_rtvec (2, x, GEN_INT
>> (TLS_LDO32)),
>> +                                    UNSPEC_TLS);
>> +           addend = force_reg (SImode, gen_rtx_CONST (SImode, addend));
>
>
> Nit I'm guessing, but I think this SImode should be Pmode.
>
>> +           dest = gen_rtx_PLUS (Pmode, dest, addend);
>> +         }
>> +       return dest;
>> +       break;
>> +
>> +      case TLS_MODEL_INITIAL_EXEC:
>> +       sum = gen_rtx_UNSPEC (Pmode,
>> +                             gen_rtvec (2, x, GEN_INT (TLS_IE32_FDPIC)),
>> +                             UNSPEC_TLS);
>> +       reg = load_tls_operand (sum, reg);
>> +       /* FIXME: optimize below? */
>
>
> Not a fan of such FIXMEs. Let's either optimise it now or leave the comment
> out.
>
>> +       emit_insn (gen_addsi3 (reg, reg, gen_rtx_REG (Pmode, 9)));
>> +       emit_insn (gen_movsi (reg, gen_rtx_MEM (Pmode, reg)));
>
>
> You can use the shorter emit_move_insn (). I think there are other places in
> the series
> where you can do that as well.
>
>> +       tp = arm_load_tp (NULL_RTX);
>> +
>> +       return gen_rtx_PLUS (Pmode, tp, reg);
>> +       break;
>> +
>> +      case TLS_MODEL_LOCAL_EXEC:
>> +       tp = arm_load_tp (NULL_RTX);
>> +       reg = gen_rtx_UNSPEC (Pmode,
>> +                             gen_rtvec (2, x, GEN_INT (TLS_LE32)),
>> +                             UNSPEC_TLS);
>> +       reg = force_reg (SImode, gen_rtx_CONST (SImode, reg));
>> +
>> +       return gen_rtx_PLUS (Pmode, tp, reg);
>> +
>> +      default:
>> +       abort ();
>> +      }
>> +}
>> +
>> +static rtx
>> +legitimize_tls_address_not_fdpic (rtx x, rtx reg)
>>  {
>
>
> Likewise, function comment please.
>
>>    rtx dest, tp, label, labelno, sum, ret, eqv, addend;
>>    rtx_insn *insns;
>> @@ -8831,6 +8934,15 @@ legitimize_tls_address (rtx x, rtx reg)
>>      }
>>  }
>>
>> +rtx
>> +legitimize_tls_address (rtx x, rtx reg)
>> +{
>
>
> And here.
>
>> +    if (TARGET_FDPIC)
>> +       return legitimize_tls_address_fdpic (x, reg);
>> +    else
>> +       return legitimize_tls_address_not_fdpic (x, reg);
>> +}
>
>
> And this is my main gripe.
> The new fdpic-specific function looks like it could be integrated with the
> existing legitimize_tls_address
> function. The structure is the same (switch on model construct and emit some
> RTXes).
> Can you look to merge the fdpic code more organically rather than splitting
> them off?
>

OK, I think it was easier to split them during development, I'll merge them.

> Thanks,
> Kyrill
>
>
>> +
>>  /* Try machine-dependent ways of modifying an illegitimate address
>>     to be legitimate.  If we find one, return the new, valid address.  */
>>  rtx
>> @@ -28022,15 +28134,24 @@ arm_emit_tls_decoration (FILE *fp, rtx x)
>>      case TLS_GD32:
>>        fputs ("(tlsgd)", fp);
>>        break;
>> +    case TLS_GD32_FDPIC:
>> +      fputs ("(tlsgd_fdpic)", fp);
>> +      break;
>>      case TLS_LDM32:
>>        fputs ("(tlsldm)", fp);
>>        break;
>> +    case TLS_LDM32_FDPIC:
>> +      fputs ("(tlsldm_fdpic)", fp);
>> +      break;
>>      case TLS_LDO32:
>>        fputs ("(tlsldo)", fp);
>>        break;
>>      case TLS_IE32:
>>        fputs ("(gottpoff)", fp);
>>        break;
>> +    case TLS_IE32_FDPIC:
>> +      fputs ("(gottpoff_fdpic)", fp);
>> +      break;
>>      case TLS_LE32:
>>        fputs ("(tpoff)", fp);
>>        break;
>> --
>> 2.6.3
>>
>