| Message ID | CACUk7=U=aaYmKf+r2cOyYCkzcyT_Aw8uTxskU6jHt5T1gNXo_w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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 > > 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
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 >> > 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
On 28/06/12 10:03, Carrot Wei 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 > No, it's sbc ~hi -- bitwise inversion It all falls out from the specification, where adc == X + Y + C and sbc == X + ~Y + C. Hence the need to use arm_not_operand. R.
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
> subs -lo ; sbc ~hi - lower negative, upper negative > subs -lo ; adc hi - lower negative, upper positive Yes. <snip> >> >>> >>>> (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" Indeed that was a typo on my part. Sorry about that. Ramana
--- 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" (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")] )