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
>>
> 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"
>> [(set_attr "conds" "use")]
>> )
>>
>> * I'd like a new const_ok_for_dimode_op function that dealt with each
>> of these operations, thus your plus operation with a DImode constant
>> would just be a check similar to what I've said above.
>
> Good idea, it will make the interface cleaner. I will do it later.
I think it should help with a clean interface for all the operations
you plan to add.
>
>> * You then don't need the new subdi3_immediate pattern and the split
>> can happen after reload. Adjust predicates and constraints
>> accordingly, delete it. Also please use CONST_INT_P instead of
>
> Even if I delete subdi3_immediate pattern, we still need the
> predicates and constraints to represent the negative di numbers in
> other patterns.
I agree you need the predicate - I suspect you can get away with a
single constraint for all valid add immediate DImode operands
especially if you are splitting it later to the constituent forms.
regards,
Ramana
>
> thanks
> Carrot
