diff mbox

[RFC,MIPS] Patch to enable LRA for MIPS backend

Message ID 871tvqnai3.fsf@talisman.default
State New
Headers show

Commit Message

Richard Sandiford May 18, 2014, 7:37 p.m. UTC
Richard Sandiford <rdsandiford@googlemail.com> writes:
> I think a cleaner way of doing it would be to have helper functions
> that switch in and out of the eliminated form, storing the old form
> in fields of a new structure (either separate from address_info,
> or a local inheritance of it).  We probably also want to have arrays
> of address_infos, one for each operand, so that we don't analyse the
> same address too many times during the same insn.

In the end maintaining the array of address_infos seemed like too much
work.  It was hard to keep it up-to-date with various other changes
that can be made, including swapping commutative operands, to the point
where it wasn't obvious whether it was really an optimisation or not.

Here's a patch that does the first.  Tested on x86_64-linux-gnu.
This time I also compared the assembly output for gcc.dg, g++.dg
and gcc.c-torture at -O2 on:

  arch64-linux-gnu arm-eabi mipsisa64-sde-elf s390x-linux-gnu
  powerpc64-linux-gnu x86_64-linux-gnu

s390x in particular is very good at exposing problems with this code.
(It caught bugs in the aborted attempt to keep an array of address_infos.)

OK to install?

Thanks,
Richard


gcc/
	* lra-constraints.c (valid_address_p): Move earlier in file.
	(address_eliminator): New structure.
	(satisfies_memory_constraint_p): New function.
	(satisfies_address_constraint_p): Likewise.
	(process_alt_operands, process_address, curr_insn_transform): Use them.

Comments

Richard Sandiford June 2, 2014, 7:36 p.m. UTC | #1
Ping.  Imagination's copyright assignment has now gone through and so
in principle we're ready for the MIPS LRA switch to go in.  We need
this LRA patch as a prequisite though.

Robert: you also had an LRA change, but is it still needed after this one?
If so, could you repost it and explain the case it handles?

Thanks,
Richard

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Richard Sandiford <rdsandiford@googlemail.com> writes:
>> I think a cleaner way of doing it would be to have helper functions
>> that switch in and out of the eliminated form, storing the old form
>> in fields of a new structure (either separate from address_info,
>> or a local inheritance of it).  We probably also want to have arrays
>> of address_infos, one for each operand, so that we don't analyse the
>> same address too many times during the same insn.
>
> In the end maintaining the array of address_infos seemed like too much
> work.  It was hard to keep it up-to-date with various other changes
> that can be made, including swapping commutative operands, to the point
> where it wasn't obvious whether it was really an optimisation or not.
>
> Here's a patch that does the first.  Tested on x86_64-linux-gnu.
> This time I also compared the assembly output for gcc.dg, g++.dg
> and gcc.c-torture at -O2 on:
>
>   arch64-linux-gnu arm-eabi mipsisa64-sde-elf s390x-linux-gnu
>   powerpc64-linux-gnu x86_64-linux-gnu
>
> s390x in particular is very good at exposing problems with this code.
> (It caught bugs in the aborted attempt to keep an array of address_infos.)
>
> OK to install?
>
> Thanks,
> Richard
>
>
> gcc/
> 	* lra-constraints.c (valid_address_p): Move earlier in file.
> 	(address_eliminator): New structure.
> 	(satisfies_memory_constraint_p): New function.
> 	(satisfies_address_constraint_p): Likewise.
> 	(process_alt_operands, process_address, curr_insn_transform): Use them.
>
> Index: gcc/lra-constraints.c
> ===================================================================
> --- gcc/lra-constraints.c	2014-05-17 17:49:19.071639652 +0100
> +++ gcc/lra-constraints.c	2014-05-18 20:36:17.499181467 +0100
> @@ -317,6 +317,118 @@ in_mem_p (int regno)
>    return get_reg_class (regno) == NO_REGS;
>  }
>  
> +/* Return 1 if ADDR is a valid memory address for mode MODE in address
> +   space AS, and check that each pseudo has the proper kind of hard
> +   reg.	 */
> +static int
> +valid_address_p (enum machine_mode mode ATTRIBUTE_UNUSED,
> +		 rtx addr, addr_space_t as)
> +{
> +#ifdef GO_IF_LEGITIMATE_ADDRESS
> +  lra_assert (ADDR_SPACE_GENERIC_P (as));
> +  GO_IF_LEGITIMATE_ADDRESS (mode, addr, win);
> +  return 0;
> +
> + win:
> +  return 1;
> +#else
> +  return targetm.addr_space.legitimate_address_p (mode, addr, 0, as);
> +#endif
> +}
> +
> +namespace {
> +  /* Temporarily eliminates registers in an address (for the lifetime of
> +     the object).  */
> +  class address_eliminator {
> +  public:
> +    address_eliminator (struct address_info *ad);
> +    ~address_eliminator ();
> +
> +  private:
> +    struct address_info *m_ad;
> +    rtx *m_base_loc;
> +    rtx m_base_reg;
> +    rtx *m_index_loc;
> +    rtx m_index_reg;
> +  };
> +}
> +
> +address_eliminator::address_eliminator (struct address_info *ad)
> +  : m_ad (ad),
> +    m_base_loc (strip_subreg (ad->base_term)),
> +    m_base_reg (NULL_RTX),
> +    m_index_loc (strip_subreg (ad->index_term)),
> +    m_index_reg (NULL_RTX)
> +{
> +  if (m_base_loc != NULL)
> +    {
> +      m_base_reg = *m_base_loc;
> +      lra_eliminate_reg_if_possible (m_base_loc);
> +      if (m_ad->base_term2 != NULL)
> +	*m_ad->base_term2 = *m_ad->base_term;
> +    }
> +  if (m_index_loc != NULL)
> +    {
> +      m_index_reg = *m_index_loc;
> +      lra_eliminate_reg_if_possible (m_index_loc);
> +    }
> +}
> +
> +address_eliminator::~address_eliminator ()
> +{
> +  if (m_base_loc && *m_base_loc != m_base_reg)
> +    {
> +      *m_base_loc = m_base_reg;
> +      if (m_ad->base_term2 != NULL)
> +	*m_ad->base_term2 = *m_ad->base_term;
> +    }
> +  if (m_index_loc && *m_index_loc != m_index_reg)
> +    *m_index_loc = m_index_reg;
> +}
> +
> +/* Return true if the eliminated form of AD is a legitimate target address.  */
> +static bool
> +valid_address_p (struct address_info *ad)
> +{
> +  address_eliminator eliminator (ad);
> +  return valid_address_p (ad->mode, *ad->outer, ad->as);
> +}
> +
> +#ifdef EXTRA_CONSTRAINT_STR
> +/* Return true if the eliminated form of memory reference OP satisfies
> +   extra address constraint CONSTRAINT.  */
> +static bool
> +satisfies_memory_constraint_p (rtx op, const char *constraint)
> +{
> +  struct address_info ad;
> +
> +  decompose_mem_address (&ad, op);
> +  address_eliminator eliminator (&ad);
> +  return EXTRA_CONSTRAINT_STR (op, *constraint, constraint);
> +}
> +
> +/* Return true if the eliminated form of address AD satisfies extra
> +   address constraint CONSTRAINT.  */
> +static bool
> +satisfies_address_constraint_p (struct address_info *ad,
> +				const char *constraint)
> +{
> +  address_eliminator eliminator (ad);
> +  return EXTRA_CONSTRAINT_STR (*ad->outer, *constraint, constraint);
> +}
> +
> +/* Return true if the eliminated form of address OP satisfies extra
> +   address constraint CONSTRAINT.  */
> +static bool
> +satisfies_address_constraint_p (rtx op, const char *constraint)
> +{
> +  struct address_info ad;
> +
> +  decompose_lea_address (&ad, &op);
> +  return satisfies_address_constraint_p (&ad, constraint);
> +}
> +#endif
> +
>  /* Initiate equivalences for LRA.  As we keep original equivalences
>     before any elimination, we need to make copies otherwise any change
>     in insns might change the equivalences.  */
> @@ -1941,7 +2053,8 @@ process_alt_operands (int only_alternati
>  #ifdef EXTRA_CONSTRAINT_STR
>  		      if (EXTRA_MEMORY_CONSTRAINT (c, p))
>  			{
> -			  if (EXTRA_CONSTRAINT_STR (op, c, p))
> +			  if (MEM_P (op)
> +			      && satisfies_memory_constraint_p (op, p))
>  			    win = true;
>  			  else if (spilled_pseudo_p (op))
>  			    win = true;
> @@ -1960,7 +2073,7 @@ process_alt_operands (int only_alternati
>  			}
>  		      if (EXTRA_ADDRESS_CONSTRAINT (c, p))
>  			{
> -			  if (EXTRA_CONSTRAINT_STR (op, c, p))
> +			  if (satisfies_address_constraint_p (op, p))
>  			    win = true;
>  
>  			  /* If we didn't already win, we can reload
> @@ -2576,60 +2689,6 @@ process_alt_operands (int only_alternati
>    return ok_p;
>  }
>  
> -/* Return 1 if ADDR is a valid memory address for mode MODE in address
> -   space AS, and check that each pseudo has the proper kind of hard
> -   reg.	 */
> -static int
> -valid_address_p (enum machine_mode mode ATTRIBUTE_UNUSED,
> -		 rtx addr, addr_space_t as)
> -{
> -#ifdef GO_IF_LEGITIMATE_ADDRESS
> -  lra_assert (ADDR_SPACE_GENERIC_P (as));
> -  GO_IF_LEGITIMATE_ADDRESS (mode, addr, win);
> -  return 0;
> -
> - win:
> -  return 1;
> -#else
> -  return targetm.addr_space.legitimate_address_p (mode, addr, 0, as);
> -#endif
> -}
> -
> -/* Return whether address AD is valid.  */
> -
> -static bool
> -valid_address_p (struct address_info *ad)
> -{
> -  /* Some ports do not check displacements for eliminable registers,
> -     so we replace them temporarily with the elimination target.  */
> -  rtx saved_base_reg = NULL_RTX;
> -  rtx saved_index_reg = NULL_RTX;
> -  rtx *base_term = strip_subreg (ad->base_term);
> -  rtx *index_term = strip_subreg (ad->index_term);
> -  if (base_term != NULL)
> -    {
> -      saved_base_reg = *base_term;
> -      lra_eliminate_reg_if_possible (base_term);
> -      if (ad->base_term2 != NULL)
> -	*ad->base_term2 = *ad->base_term;
> -    }
> -  if (index_term != NULL)
> -    {
> -      saved_index_reg = *index_term;
> -      lra_eliminate_reg_if_possible (index_term);
> -    }
> -  bool ok_p = valid_address_p (ad->mode, *ad->outer, ad->as);
> -  if (saved_base_reg != NULL_RTX)
> -    {
> -      *base_term = saved_base_reg;
> -      if (ad->base_term2 != NULL)
> -	*ad->base_term2 = *ad->base_term;
> -    }
> -  if (saved_index_reg != NULL_RTX)
> -    *index_term = saved_index_reg;
> -  return ok_p;
> -}
> -
>  /* Make reload base reg + disp from address AD.  Return the new pseudo.  */
>  static rtx
>  base_plus_disp_to_reg (struct address_info *ad)
> @@ -2832,7 +2891,7 @@ process_address (int nop, rtx *before, r
>       EXTRA_CONSTRAINT_STR for the validation.  */
>    if (constraint[0] != 'p'
>        && EXTRA_ADDRESS_CONSTRAINT (constraint[0], constraint)
> -      && EXTRA_CONSTRAINT_STR (op, constraint[0], constraint))
> +      && satisfies_address_constraint_p (&ad, constraint))
>      return change_p;
>  #endif
>  
> @@ -3539,7 +3598,7 @@ curr_insn_transform (void)
>  		  break;
>  #ifdef EXTRA_CONSTRAINT_STR
>  		if (EXTRA_MEMORY_CONSTRAINT (c, constraint)
> -		    && EXTRA_CONSTRAINT_STR (tem, c, constraint))
> +		    && satisfies_memory_constraint_p (tem, constraint))
>  		  break;
>  #endif
>  	      }
Vladimir Makarov June 3, 2014, 8:50 p.m. UTC | #2
On 06/02/2014 03:36 PM, Richard Sandiford wrote:
> Ping.  Imagination's copyright assignment has now gone through and so
> in principle we're ready for the MIPS LRA switch to go in.  We need
> this LRA patch as a prequisite though.
>
> Robert: you also had an LRA change, but is it still needed after this one?
> If so, could you repost it and explain the case it handles?
>
> Thanks,
> Richard
>
> Richard Sandiford <rdsandiford@googlemail.com> writes:
>> Richard Sandiford <rdsandiford@googlemail.com> writes:
>>> I think a cleaner way of doing it would be to have helper functions
>>> that switch in and out of the eliminated form, storing the old form
>>> in fields of a new structure (either separate from address_info,
>>> or a local inheritance of it).  We probably also want to have arrays
>>> of address_infos, one for each operand, so that we don't analyse the
>>> same address too many times during the same insn.
>> In the end maintaining the array of address_infos seemed like too much
>> work.  It was hard to keep it up-to-date with various other changes
>> that can be made, including swapping commutative operands, to the point
>> where it wasn't obvious whether it was really an optimisation or not.
>>
>> Here's a patch that does the first.  Tested on x86_64-linux-gnu.
>> This time I also compared the assembly output for gcc.dg, g++.dg
>> and gcc.c-torture at -O2 on:
>>
>>   arch64-linux-gnu arm-eabi mipsisa64-sde-elf s390x-linux-gnu
>>   powerpc64-linux-gnu x86_64-linux-gnu
>>
>> s390x in particular is very good at exposing problems with this code.
>> (It caught bugs in the aborted attempt to keep an array of address_infos.)
>>
>> OK to install?
>>
Yes, Richard.  Thanks for the wide testing.  It was also a pleasure to
read this C++ code.  Really nice.
Richard Sandiford June 4, 2014, 5:44 p.m. UTC | #3
Vladimir Makarov <vmakarov@redhat.com> writes:
> On 06/02/2014 03:36 PM, Richard Sandiford wrote:
>> Ping.  Imagination's copyright assignment has now gone through and so
>> in principle we're ready for the MIPS LRA switch to go in.  We need
>> this LRA patch as a prequisite though.
>>
>> Robert: you also had an LRA change, but is it still needed after this one?
>> If so, could you repost it and explain the case it handles?
>>
>> Thanks,
>> Richard
>>
>> Richard Sandiford <rdsandiford@googlemail.com> writes:
>>> Richard Sandiford <rdsandiford@googlemail.com> writes:
>>>> I think a cleaner way of doing it would be to have helper functions
>>>> that switch in and out of the eliminated form, storing the old form
>>>> in fields of a new structure (either separate from address_info,
>>>> or a local inheritance of it).  We probably also want to have arrays
>>>> of address_infos, one for each operand, so that we don't analyse the
>>>> same address too many times during the same insn.
>>> In the end maintaining the array of address_infos seemed like too much
>>> work.  It was hard to keep it up-to-date with various other changes
>>> that can be made, including swapping commutative operands, to the point
>>> where it wasn't obvious whether it was really an optimisation or not.
>>>
>>> Here's a patch that does the first.  Tested on x86_64-linux-gnu.
>>> This time I also compared the assembly output for gcc.dg, g++.dg
>>> and gcc.c-torture at -O2 on:
>>>
>>>   arch64-linux-gnu arm-eabi mipsisa64-sde-elf s390x-linux-gnu
>>>   powerpc64-linux-gnu x86_64-linux-gnu
>>>
>>> s390x in particular is very good at exposing problems with this code.
>>> (It caught bugs in the aborted attempt to keep an array of address_infos.)
>>>
>>> OK to install?
>>>
> Yes, Richard.  Thanks for the wide testing.

Thanks Vlad, committed as r211242.  Given the problems with my first version,
I suppose it'd make sense to wait a few days to see whether there's any
fallout before applying the MIPS backend patch.

Richard
Robert Suchanek June 16, 2014, 4:12 p.m. UTC | #4
Pinging for approval.

This part of the patch will be needed for MIPS16. The second part to enable
LRA in MIPS has been already approved.

> Hi Richard, 
> 
> >> Robert: you also had an LRA change, but is it still needed after this one?
> >> If so, could you repost it and explain the case it handles? 
>
> For just turning the LRA for the MIPS backend is not needed but we have issues
> with the code size for MIPS16. LRA inserted a lot of reloads and the code size
> increased on average by about 10% IIRC. To fix this, a number of patterns 
> have to accept the stack pointer and a new class, M16_SP_REGS with 
> M16_REGS + $sp was added.
> 
> However, this triggered a reloading problem as the stack pointer was rejected
> by the back end and LRA tried to insert base+disp with the displacement not
> always present. It only affects $sp not directly accessible as in MIPS16 case.
>
> Regards,
> Robert
>         
> gcc/                                                      
> 	* lra-constraints.c (base_to_reg): New function.       
> 	(process_address): Use new function.                   
> 
> diff --git gcc/lra-constraints.c gcc/lra-constraints.c
> index 08716fe..d5ed37f 100644
> --- gcc/lra-constraints.c
> +++ gcc/lra-constraints.c
> @@ -2686,6 +2686,39 @@ process_alt_operands (int only_alternative)
>    return ok_p;
>  }
>  
> +/* Make reload base reg from address AD.  */
> +static rtx
> +base_to_reg (struct address_info *ad)
> +{
> +  enum reg_class cl;
> +  int code = -1;
> +  rtx new_inner = NULL_RTX;
> +  rtx new_reg = NULL_RTX;
> +  rtx insn;
> +  rtx last_insn = get_last_insn();
> +
> +  lra_assert (ad->base == ad->base_term && ad->disp == ad->disp_term);
> +  cl = base_reg_class (ad->mode, ad->as, ad->base_outer_code,
> +                       get_index_code (ad));
> +  new_reg = lra_create_new_reg (GET_MODE (*ad->base_term), NULL_RTX,
> +                                cl, "base");
> +  new_inner = simplify_gen_binary (PLUS, GET_MODE (new_reg), new_reg,
> +                                   ad->disp_term == NULL
> +                                   ? gen_int_mode (0, ad->mode)
> +                                   : *ad->disp_term);
> +  if (!valid_address_p (ad->mode, new_inner, ad->as))
> +    return NULL_RTX;
> +  insn = emit_insn (gen_rtx_SET (ad->mode, new_reg, *ad->base_term));
> +  code = recog_memoized (insn);
> +  if (code < 0)
> +    {
> +      delete_insns_since (last_insn);
> +      return NULL_RTX;
> +    }
> +
> +  return new_inner;
> +}
> +
>  /* Make reload base reg + disp from address AD.  Return the new pseudo.  */
>  static rtx
>  base_plus_disp_to_reg (struct address_info *ad)
> @@ -2908,6 +2941,8 @@ process_address_1 (int nop, rtx *before, rtx *after)
>  
>       3) the address is a frame address with an invalid offset.
>  
> +     4) the address is a frame address with an invalid base.
> +
>       All these cases involve a non-autoinc address, so there is no
>       point revalidating other types.  */
>    if (ad.autoinc_p || valid_address_p (&ad))
> @@ -2989,14 +3024,19 @@ process_address_1 (int nop, rtx *before, rtx *after)
>        int regno;
>        enum reg_class cl;
>        rtx set, insns, last_insn;
> +      /* Try to reload base into register only if the base is invalid
> +         for the address but with valid offset, case (4) above.  */
> +      start_sequence ();
> +      new_reg = base_to_reg (&ad);
> +
>        /* base + disp => new base, cases (1) and (3) above.  */
>        /* Another option would be to reload the displacement into an
>  	 index register.  However, postreload has code to optimize
>  	 address reloads that have the same base and different
>  	 displacements, so reloading into an index register would
>  	 not necessarily be a win.  */
> -      start_sequence ();
> -      new_reg = base_plus_disp_to_reg (&ad);
> +      if (new_reg == NULL_RTX)
> +        new_reg = base_plus_disp_to_reg (&ad);
>        insns = get_insns ();
>        last_insn = get_last_insn ();
>        /* If we generated at least two insns, try last insn source as
Vladimir Makarov June 16, 2014, 6:08 p.m. UTC | #5
On 2014-06-16, 12:12 PM, Robert Suchanek wrote:
> Pinging for approval.
>
> This part of the patch will be needed for MIPS16. The second part to enable
> LRA in MIPS has been already approved.
>

   Sorry, Robert.  I thought you are waiting for some Richard's comment 
(actually he knows the code well and wrote address decoding in rtlanal.c).

   The patch is ok for me and makes LRA even more portable as it adds a 
new profitable address transformation and the code can be useful for 
other targets too.

   Thanks.


>> Hi Richard,
>>
>>>> Robert: you also had an LRA change, but is it still needed after this one?
>>>> If so, could you repost it and explain the case it handles?
>>
>> For just turning the LRA for the MIPS backend is not needed but we have issues
>> with the code size for MIPS16. LRA inserted a lot of reloads and the code size
>> increased on average by about 10% IIRC. To fix this, a number of patterns
>> have to accept the stack pointer and a new class, M16_SP_REGS with
>> M16_REGS + $sp was added.
>>
>> However, this triggered a reloading problem as the stack pointer was rejected
>> by the back end and LRA tried to insert base+disp with the displacement not
>> always present. It only affects $sp not directly accessible as in MIPS16 case.
>>
>> Regards,
>> Robert
>>
>> gcc/
>> 	* lra-constraints.c (base_to_reg): New function.
>> 	(process_address): Use new function.
>>
>> diff --git gcc/lra-constraints.c gcc/lra-constraints.c
>> index 08716fe..d5ed37f 100644
>> --- gcc/lra-constraints.c
>> +++ gcc/lra-constraints.c
>> @@ -2686,6 +2686,39 @@ process_alt_operands (int only_alternative)
>>     return ok_p;
>>   }
>>
>> +/* Make reload base reg from address AD.  */
>> +static rtx
>> +base_to_reg (struct address_info *ad)
>> +{
>> +  enum reg_class cl;
>> +  int code = -1;
>> +  rtx new_inner = NULL_RTX;
>> +  rtx new_reg = NULL_RTX;
>> +  rtx insn;
>> +  rtx last_insn = get_last_insn();
>> +
>> +  lra_assert (ad->base == ad->base_term && ad->disp == ad->disp_term);
>> +  cl = base_reg_class (ad->mode, ad->as, ad->base_outer_code,
>> +                       get_index_code (ad));
>> +  new_reg = lra_create_new_reg (GET_MODE (*ad->base_term), NULL_RTX,
>> +                                cl, "base");
>> +  new_inner = simplify_gen_binary (PLUS, GET_MODE (new_reg), new_reg,
>> +                                   ad->disp_term == NULL
>> +                                   ? gen_int_mode (0, ad->mode)
>> +                                   : *ad->disp_term);
>> +  if (!valid_address_p (ad->mode, new_inner, ad->as))
>> +    return NULL_RTX;
>> +  insn = emit_insn (gen_rtx_SET (ad->mode, new_reg, *ad->base_term));
>> +  code = recog_memoized (insn);
>> +  if (code < 0)
>> +    {
>> +      delete_insns_since (last_insn);
>> +      return NULL_RTX;
>> +    }
>> +
>> +  return new_inner;
>> +}
>> +
>>   /* Make reload base reg + disp from address AD.  Return the new pseudo.  */
>>   static rtx
>>   base_plus_disp_to_reg (struct address_info *ad)
>> @@ -2908,6 +2941,8 @@ process_address_1 (int nop, rtx *before, rtx *after)
>>
>>        3) the address is a frame address with an invalid offset.
>>
>> +     4) the address is a frame address with an invalid base.
>> +
>>        All these cases involve a non-autoinc address, so there is no
>>        point revalidating other types.  */
>>     if (ad.autoinc_p || valid_address_p (&ad))
>> @@ -2989,14 +3024,19 @@ process_address_1 (int nop, rtx *before, rtx *after)
>>         int regno;
>>         enum reg_class cl;
>>         rtx set, insns, last_insn;
>> +      /* Try to reload base into register only if the base is invalid
>> +         for the address but with valid offset, case (4) above.  */
>> +      start_sequence ();
>> +      new_reg = base_to_reg (&ad);
>> +
>>         /* base + disp => new base, cases (1) and (3) above.  */
>>         /* Another option would be to reload the displacement into an
>>   	 index register.  However, postreload has code to optimize
>>   	 address reloads that have the same base and different
>>   	 displacements, so reloading into an index register would
>>   	 not necessarily be a win.  */
>> -      start_sequence ();
>> -      new_reg = base_plus_disp_to_reg (&ad);
>> +      if (new_reg == NULL_RTX)
>> +        new_reg = base_plus_disp_to_reg (&ad);
>>         insns = get_insns ();
>>         last_insn = get_last_insn ();
>>         /* If we generated at least two insns, try last insn source as
>
Matthew Fortune June 18, 2014, 8:51 p.m. UTC | #6
> On 2014-06-16, 12:12 PM, Robert Suchanek wrote:
> > Pinging for approval.
> >
> > This part of the patch will be needed for MIPS16. The second part to
> enable
> > LRA in MIPS has been already approved.
> >
> 
>    Sorry, Robert.  I thought you are waiting for some Richard's comment
> (actually he knows the code well and wrote address decoding in rtlanal.c).
> 
>    The patch is ok for me and makes LRA even more portable as it adds a
> new profitable address transformation and the code can be useful for
> other targets too.

Thanks.

Core LRA change committed as: r211802
MIPS LRA committed as: r211805

Matthew
diff mbox

Patch

Index: gcc/lra-constraints.c
===================================================================
--- gcc/lra-constraints.c	2014-05-17 17:49:19.071639652 +0100
+++ gcc/lra-constraints.c	2014-05-18 20:36:17.499181467 +0100
@@ -317,6 +317,118 @@  in_mem_p (int regno)
   return get_reg_class (regno) == NO_REGS;
 }
 
+/* Return 1 if ADDR is a valid memory address for mode MODE in address
+   space AS, and check that each pseudo has the proper kind of hard
+   reg.	 */
+static int
+valid_address_p (enum machine_mode mode ATTRIBUTE_UNUSED,
+		 rtx addr, addr_space_t as)
+{
+#ifdef GO_IF_LEGITIMATE_ADDRESS
+  lra_assert (ADDR_SPACE_GENERIC_P (as));
+  GO_IF_LEGITIMATE_ADDRESS (mode, addr, win);
+  return 0;
+
+ win:
+  return 1;
+#else
+  return targetm.addr_space.legitimate_address_p (mode, addr, 0, as);
+#endif
+}
+
+namespace {
+  /* Temporarily eliminates registers in an address (for the lifetime of
+     the object).  */
+  class address_eliminator {
+  public:
+    address_eliminator (struct address_info *ad);
+    ~address_eliminator ();
+
+  private:
+    struct address_info *m_ad;
+    rtx *m_base_loc;
+    rtx m_base_reg;
+    rtx *m_index_loc;
+    rtx m_index_reg;
+  };
+}
+
+address_eliminator::address_eliminator (struct address_info *ad)
+  : m_ad (ad),
+    m_base_loc (strip_subreg (ad->base_term)),
+    m_base_reg (NULL_RTX),
+    m_index_loc (strip_subreg (ad->index_term)),
+    m_index_reg (NULL_RTX)
+{
+  if (m_base_loc != NULL)
+    {
+      m_base_reg = *m_base_loc;
+      lra_eliminate_reg_if_possible (m_base_loc);
+      if (m_ad->base_term2 != NULL)
+	*m_ad->base_term2 = *m_ad->base_term;
+    }
+  if (m_index_loc != NULL)
+    {
+      m_index_reg = *m_index_loc;
+      lra_eliminate_reg_if_possible (m_index_loc);
+    }
+}
+
+address_eliminator::~address_eliminator ()
+{
+  if (m_base_loc && *m_base_loc != m_base_reg)
+    {
+      *m_base_loc = m_base_reg;
+      if (m_ad->base_term2 != NULL)
+	*m_ad->base_term2 = *m_ad->base_term;
+    }
+  if (m_index_loc && *m_index_loc != m_index_reg)
+    *m_index_loc = m_index_reg;
+}
+
+/* Return true if the eliminated form of AD is a legitimate target address.  */
+static bool
+valid_address_p (struct address_info *ad)
+{
+  address_eliminator eliminator (ad);
+  return valid_address_p (ad->mode, *ad->outer, ad->as);
+}
+
+#ifdef EXTRA_CONSTRAINT_STR
+/* Return true if the eliminated form of memory reference OP satisfies
+   extra address constraint CONSTRAINT.  */
+static bool
+satisfies_memory_constraint_p (rtx op, const char *constraint)
+{
+  struct address_info ad;
+
+  decompose_mem_address (&ad, op);
+  address_eliminator eliminator (&ad);
+  return EXTRA_CONSTRAINT_STR (op, *constraint, constraint);
+}
+
+/* Return true if the eliminated form of address AD satisfies extra
+   address constraint CONSTRAINT.  */
+static bool
+satisfies_address_constraint_p (struct address_info *ad,
+				const char *constraint)
+{
+  address_eliminator eliminator (ad);
+  return EXTRA_CONSTRAINT_STR (*ad->outer, *constraint, constraint);
+}
+
+/* Return true if the eliminated form of address OP satisfies extra
+   address constraint CONSTRAINT.  */
+static bool
+satisfies_address_constraint_p (rtx op, const char *constraint)
+{
+  struct address_info ad;
+
+  decompose_lea_address (&ad, &op);
+  return satisfies_address_constraint_p (&ad, constraint);
+}
+#endif
+
 /* Initiate equivalences for LRA.  As we keep original equivalences
    before any elimination, we need to make copies otherwise any change
    in insns might change the equivalences.  */
@@ -1941,7 +2053,8 @@  process_alt_operands (int only_alternati
 #ifdef EXTRA_CONSTRAINT_STR
 		      if (EXTRA_MEMORY_CONSTRAINT (c, p))
 			{
-			  if (EXTRA_CONSTRAINT_STR (op, c, p))
+			  if (MEM_P (op)
+			      && satisfies_memory_constraint_p (op, p))
 			    win = true;
 			  else if (spilled_pseudo_p (op))
 			    win = true;
@@ -1960,7 +2073,7 @@  process_alt_operands (int only_alternati
 			}
 		      if (EXTRA_ADDRESS_CONSTRAINT (c, p))
 			{
-			  if (EXTRA_CONSTRAINT_STR (op, c, p))
+			  if (satisfies_address_constraint_p (op, p))
 			    win = true;
 
 			  /* If we didn't already win, we can reload
@@ -2576,60 +2689,6 @@  process_alt_operands (int only_alternati
   return ok_p;
 }
 
-/* Return 1 if ADDR is a valid memory address for mode MODE in address
-   space AS, and check that each pseudo has the proper kind of hard
-   reg.	 */
-static int
-valid_address_p (enum machine_mode mode ATTRIBUTE_UNUSED,
-		 rtx addr, addr_space_t as)
-{
-#ifdef GO_IF_LEGITIMATE_ADDRESS
-  lra_assert (ADDR_SPACE_GENERIC_P (as));
-  GO_IF_LEGITIMATE_ADDRESS (mode, addr, win);
-  return 0;
-
- win:
-  return 1;
-#else
-  return targetm.addr_space.legitimate_address_p (mode, addr, 0, as);
-#endif
-}
-
-/* Return whether address AD is valid.  */
-
-static bool
-valid_address_p (struct address_info *ad)
-{
-  /* Some ports do not check displacements for eliminable registers,
-     so we replace them temporarily with the elimination target.  */
-  rtx saved_base_reg = NULL_RTX;
-  rtx saved_index_reg = NULL_RTX;
-  rtx *base_term = strip_subreg (ad->base_term);
-  rtx *index_term = strip_subreg (ad->index_term);
-  if (base_term != NULL)
-    {
-      saved_base_reg = *base_term;
-      lra_eliminate_reg_if_possible (base_term);
-      if (ad->base_term2 != NULL)
-	*ad->base_term2 = *ad->base_term;
-    }
-  if (index_term != NULL)
-    {
-      saved_index_reg = *index_term;
-      lra_eliminate_reg_if_possible (index_term);
-    }
-  bool ok_p = valid_address_p (ad->mode, *ad->outer, ad->as);
-  if (saved_base_reg != NULL_RTX)
-    {
-      *base_term = saved_base_reg;
-      if (ad->base_term2 != NULL)
-	*ad->base_term2 = *ad->base_term;
-    }
-  if (saved_index_reg != NULL_RTX)
-    *index_term = saved_index_reg;
-  return ok_p;
-}
-
 /* Make reload base reg + disp from address AD.  Return the new pseudo.  */
 static rtx
 base_plus_disp_to_reg (struct address_info *ad)
@@ -2832,7 +2891,7 @@  process_address (int nop, rtx *before, r
      EXTRA_CONSTRAINT_STR for the validation.  */
   if (constraint[0] != 'p'
       && EXTRA_ADDRESS_CONSTRAINT (constraint[0], constraint)
-      && EXTRA_CONSTRAINT_STR (op, constraint[0], constraint))
+      && satisfies_address_constraint_p (&ad, constraint))
     return change_p;
 #endif
 
@@ -3539,7 +3598,7 @@  curr_insn_transform (void)
 		  break;
 #ifdef EXTRA_CONSTRAINT_STR
 		if (EXTRA_MEMORY_CONSTRAINT (c, constraint)
-		    && EXTRA_CONSTRAINT_STR (tem, c, constraint))
+		    && satisfies_memory_constraint_p (tem, constraint))
 		  break;
 #endif
 	      }