An unreviewed ARM patch

An unreviewed ARM patch

[Kazu Hirata]

Hi,

Could someone please review the following patch?

  [patch] arm: Improve arm_rtx_costs_1 for Thumb2.
  http://gcc.gnu.org/ml/gcc-patches/2009-12/msg00958.html

Thanks,

Kazu Hirata

Re: An unreviewed ARM patch

[Richard Earnshaw]

On Fri, 2010-06-04 at 06:16 -0700, Kazu Hirata wrote:
> Hi,
> 
> Could someone please review the following patch?
> 
>   [patch] arm: Improve arm_rtx_costs_1 for Thumb2.
>   http://gcc.gnu.org/ml/gcc-patches/2009-12/msg00958.html
> 

Hmm, I agree that there's probably some code in the rest of the MINUS
case that should be shared.  I'm not convinced, however, that we should
fall through into the PLUS case: most of the code from that point on
will only apply if we have RSB (and we don't in Thumb-2).

So maybe the check should be moved to the end of the MINUS code (ie just
before the fall-through) and then simplified to remove the sub-cases
that have already been handled.

R.

Re: An unreviewed ARM patch

[Kazu Hirata]

Hi Richard,

>> Could someone please review the following patch?
>>
>>    [patch] arm: Improve arm_rtx_costs_1 for Thumb2.
>>    http://gcc.gnu.org/ml/gcc-patches/2009-12/msg00958.html
>>
>
> Hmm, I agree that there's probably some code in the rest of the MINUS
> case that should be shared.  I'm not convinced, however, that we should
> fall through into the PLUS case: most of the code from that point on
> will only apply if we have RSB (and we don't in Thumb-2).
>
> So maybe the check should be moved to the end of the MINUS code (ie just
> before the fall-through) and then simplified to remove the sub-cases
> that have already been handled.

Thank you for a review.  I'll revise the patch accordingly.

Kazu Hirata

Re: An unreviewed ARM patch

[Kazu Hirata]

Hi Richard,

>> Could someone please review the following patch?
>>
>>    [patch] arm: Improve arm_rtx_costs_1 for Thumb2.
>>    http://gcc.gnu.org/ml/gcc-patches/2009-12/msg00958.html
>>
>
> Hmm, I agree that there's probably some code in the rest of the MINUS
> case that should be shared.  I'm not convinced, however, that we should
> fall through into the PLUS case: most of the code from that point on
> will only apply if we have RSB (and we don't in Thumb-2).

Err, which form of RSB are you talking about?  IIRC, ARM has:

   RSB <Rd>, <Rn>, <shifter_operand>

Thumb-2 has:

   RSB <Rd>,<Rn>,#0
   RSB.W <Rd>,<Rn>,#<const>
   RSB <Rd>,<Rn>,<Rm>{,<shift>}

So, Thumb-2's RSB supports most of ARM's RSB, except somewhat exotic 
operands like variable shifts and rotations in <shifter_operand>.

Did you mean to say Thumb-1 above?

Thanks,

Kazu Hirata

Re: An unreviewed ARM patch

[Richard Earnshaw]

On Mon, 2010-06-07 at 22:03 -0400, Kazu Hirata wrote:
> Hi Richard,
> 
> >> Could someone please review the following patch?
> >>
> >>    [patch] arm: Improve arm_rtx_costs_1 for Thumb2.
> >>    http://gcc.gnu.org/ml/gcc-patches/2009-12/msg00958.html
> >>
> >
> > Hmm, I agree that there's probably some code in the rest of the MINUS
> > case that should be shared.  I'm not convinced, however, that we should
> > fall through into the PLUS case: most of the code from that point on
> > will only apply if we have RSB (and we don't in Thumb-2).
> 
> Err, which form of RSB are you talking about?  IIRC, ARM has:
> 
>    RSB <Rd>, <Rn>, <shifter_operand>
> 
> Thumb-2 has:
> 
>    RSB <Rd>,<Rn>,#0
>    RSB.W <Rd>,<Rn>,#<const>
>    RSB <Rd>,<Rn>,<Rm>{,<shift>}
> 
> So, Thumb-2's RSB supports most of ARM's RSB, except somewhat exotic 
> operands like variable shifts and rotations in <shifter_operand>.
> 
> Did you mean to say Thumb-1 above?
> 

Possibly :-)  But it's more likely I was thinking about the lack of RSC
in Thumb2.

In which case, the patch is OK.

R.