This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [ARM Patch 1/n] PR53447: optimizations of 64bit ALU operation with constant


On Thu, Jun 28, 2012 at 5:37 PM, Ramana Radhakrishnan
<ramana.radhakrishnan@linaro.org> wrote:
> On 28 June 2012 10:03, Carrot Wei <carrot@google.com> wrote:
>> Hi Ramana
>>
>> Thanks for the review, please see my inlined comments.
>>
>> On Thu, Jun 28, 2012 at 12:02 AM, Ramana Radhakrishnan
>> <ramana.radhakrishnan@linaro.org> wrote:
>>>
>>> On 8 June 2012 10:12, Carrot Wei <carrot@google.com> wrote:
>>> > Hi
>>> >
>>> > In rtl expression, substract a constant c is expressed as add a value -c, so it
>>> > is alse processed by adddi3, and I extend it more to handle a subtraction of
>>> > 64bit constant. I created an insn pattern arm_subdi3_immediate to specifically
>>> > represent substraction with 64bit constant while continue keeping the add rtl
>>> > expression.
>>> >
>>>
>>> Sorry about the time it has taken to review this patch -Thanks for
>>> tackling this but I'm not convinced that this patch is correct and
>>> definitely can be more efficient.
>>>
>>> The range of valid 64 bit constants allowed would be in my opinion are
>>> the following- obtained by dividing the 64 bit constant into 2 32 bit
>>> halves (upper32 and lower32 referred to as upper and lower below)
>>>
>>> ?arm_not_operand (upper) && arm_add_operand (lower) which boils down
>>> to the valid combination of
>>>
>>> ?adds lo : adc hi - both positive constants.
>>> ?adds lo ; sbc hi ?- lower positive, upper negative
>
>> I assume you mean "sbc -hi" or "sbc abs(hi)", similar for following instructions
>
> hi = ~upper32
>
> lower = lower 32 bits of the constant
> hi = ?~ (upper32 bits) of the constant ( bitwise twiddle not a negate :) )
>
> For e.g.
>
> unsigned long long foo4 (unsigned long long x)
> {
> ?return x - 0x25ULL;
> }
>
> should be
> subs r0, r0, #37
> sbc ? r1, r1, #0
>
> Notice that it's #0 and not 1 ..... :)
>
>
>
>>
>>>
>>> ?subs lo ; sbc hi - lower negative, upper negative
>>> ?subs lo ; adc hi ?- lower negative, upper positive
>>>

Thank you for the detailed explanation. So the four cases should be

 adds lo : adc hi - both positive constants.
 adds lo ; sbc ~hi  - lower positive, upper negative
 subs -lo ; sbc ~hi - lower negative, upper negative
 subs -lo ; adc hi  - lower negative, upper positive


>> My first version did the similar thing, but in some cases subs and
>> adds may generate different carry flag. Assume the low word is 0 and
>> high word is negative, your method will generate
>>
>> adds r0, r0, 0
>> sbc ? r1, r1, abs(hi)
>
> No it will generate
>
> adds r0, r0, #0
> sbc ? ?r1, r1, ~hi
>
> and not abs (hi)
>
>
>
>>
>> My method generates
>>
>> subs r0, r0, 0
>> sbc ? r1, r1, abs(hi)
>>
>> ARM's definition of subs is
>>
>> (result, carry, overflow) = AddWithCarry(R[n], NOT(imm32), ‘1’);
>>
>> So the subs instruction will set carry flag, but adds clear carry
>> flag, and finally generate different result in r1.
>>
>>>
>>> Therefore I'd do the following -
>>>
>>> * Don't make *arm_adddi3 a named pattern - we don't need that.
>>> * Change the *addsi3_carryin_<optab> pattern to be something like this :
>>>
>>> --- a/gcc/config/arm/arm.md
>>> +++ b/gcc/config/arm/arm.md
>>> @@ -1001,12 +1001,14 @@
>>> ?)
>>>
>>> ?(define_insn "*addsi3_carryin_<optab>"
>>> - ?[(set (match_operand:SI 0 "s_register_operand" "=r")
>>> - ? ? ? (plus:SI (plus:SI (match_operand:SI 1 "s_register_operand" "%r")
>>> - ? ? ? ? ? ? ? ? ? ? ? ? (match_operand:SI 2 "arm_rhs_operand" "rI"))
>>> + ?[(set (match_operand:SI 0 "s_register_operand" "=r,r")
>>> + ? ? ? (plus:SI (plus:SI (match_operand:SI 1 "s_register_operand" "%r,r"
>>> + ? ? ? ? ? ? ? ? ? ? ? ? (match_operand:SI 2 "arm_not_operand" "rI,K"
>>
>> Do you mean arm_add_operand?
>
> No I mean arm_not_operand and it was a deliberate choice as explained above.
>
>>
>>> ? ? ? ? ? ? ? ? (LTUGEU:SI (reg:<cnb> CC_REGNUM) (const_int 0))))]
>>> ? "TARGET_32BIT"
>>> - ?"adc%?\\t%0, %1, %2"
>>> + ?"@
>>> + ?adc%?\\t%0, %1, %2
>>> + ?sbc%?\\t%0, %1, %#n2"

Since constraint "K" is logical not, not negative, should the last
line be following?

+ ?sbc%?\\t%0, %1, #%B2"

thanks
Carrot


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]