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 > > 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) 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? > (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. > * 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. thanks Carrot
