On Thu, Jun 28, 2012 at 5:37 PM, Ramana Radhakrishnan
<ramana.radhakrish...@linaro.org> wrote:
> On 28 June 2012 10:03, Carrot Wei <car...@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.radhakrish...@linaro.org> wrote:
>>>
>>> On 8 June 2012 10:12, Carrot Wei <car...@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
