* [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns @ 2011-03-26 15:55 Carrot Wei 2011-03-30 1:24 ` Ramana Radhakrishnan 0 siblings, 1 reply; 16+ messages in thread From: Carrot Wei @ 2011-03-26 15:55 UTC (permalink / raw) To: gcc-patches Hi As described in http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47855, there are many insn patterns don't compute attribute length correctly. This patch is the first and simplest part of the fixing. This patch has been tested on qemu. thanks Carrot ChangeLog: 2011-03-26 Wei Guozhi <carrot@google.com> PR target/47855 * config/arm/arm.md (arm_cmpsi_insn): Compute attr "length". (arm_cond_branch): Likewise. (arm_cond_branch_reversed): Likewise. (arm_jump): Likewise. (push_multi): Likewise. Index: arm.md =================================================================== --- arm.md (revision 171337) +++ arm.md (working copy) @@ -7115,7 +7115,18 @@ "@ cmp%?\\t%0, %1 cmn%?\\t%0, #%n1" - [(set_attr "conds" "set")] + [(set_attr "conds" "set") + (set (attr "length") + (if_then_else + (and (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) + (eq (symbol_ref "which_alternative") (const_int 0))) + (ior (ne (symbol_ref "REG_P (operands[1])") (const_int 0)) + (and (ne (symbol_ref "CONST_INT_P (operands[1])") (const_int 0)) + (and (ge (symbol_ref "INTVAL (operands[1])") (const_int 0)) + (le (symbol_ref "INTVAL (operands[1])") + (const_int 255)))))) + (const_int 2) + (const_int 4)))] ) (define_insn "*cmpsi_shiftsi" @@ -7286,7 +7297,14 @@ return \"b%d1\\t%l0\"; " [(set_attr "conds" "use") - (set_attr "type" "branch")] + (set_attr "type" "branch") + (set (attr "length") + (if_then_else + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) + (and (ge (minus (match_dup 0) (pc)) (const_int -250)) + (le (minus (match_dup 0) (pc)) (const_int 256)))) + (const_int 2) + (const_int 4)))] ) (define_insn "*arm_cond_branch_reversed" @@ -7305,7 +7323,14 @@ return \"b%D1\\t%l0\"; " [(set_attr "conds" "use") - (set_attr "type" "branch")] + (set_attr "type" "branch") + (set (attr "length") + (if_then_else + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) + (and (ge (minus (match_dup 0) (pc)) (const_int -250)) + (le (minus (match_dup 0) (pc)) (const_int 256)))) + (const_int 2) + (const_int 4)))] ) @@ -7757,7 +7782,14 @@ return \"b%?\\t%l0\"; } " - [(set_attr "predicable" "yes")] + [(set_attr "predicable" "yes") + (set (attr "length") + (if_then_else + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) + (and (ge (minus (match_dup 0) (pc)) (const_int -2044)) + (le (minus (match_dup 0) (pc)) (const_int 2048)))) + (const_int 2) + (const_int 4)))] ) (define_insn "*thumb_jump" @@ -10256,7 +10288,26 @@ return \"\"; }" - [(set_attr "type" "store4")] + [(set_attr "type" "store4") + (set (attr "length") + (if_then_else + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) + (ne (symbol_ref "{ + int i, regno, hi_reg; + int num_saves = XVECLEN (operands[2], 0); + regno = REGNO (operands[1]); + hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS) + && (regno != LR_REGNUM); + for (i = 1; i < num_saves; i++) + { + regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0)); + hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS) + && (regno != LR_REGNUM); + } + !hi_reg; }") + (const_int 0))) + (const_int 2) + (const_int 4)))] ) (define_insn "stack_tie" ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-03-26 15:55 [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns Carrot Wei @ 2011-03-30 1:24 ` Ramana Radhakrishnan 2011-03-30 7:35 ` Chung-Lin Tang 2011-03-31 3:34 ` Carrot Wei 0 siblings, 2 replies; 16+ messages in thread From: Ramana Radhakrishnan @ 2011-03-30 1:24 UTC (permalink / raw) To: Carrot Wei; +Cc: gcc-patches Hi Carrot, On 26/03/11 15:14, Carrot Wei wrote: > Index: arm.md > =================================================================== > --- arm.md (revision 171337) > +++ arm.md (working copy) > @@ -7115,7 +7115,18 @@ > "@ > cmp%?\\t%0, %1 > cmn%?\\t%0, #%n1" > - [(set_attr "conds" "set")] > + [(set_attr "conds" "set") > + (set (attr "length") > + (if_then_else > + (and (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) > + (eq (symbol_ref "which_alternative") (const_int 0))) > + (ior (ne (symbol_ref "REG_P (operands[1])") (const_int 0)) > + (and (ne (symbol_ref "CONST_INT_P (operands[1])") (const_int 0)) > + (and (ge (symbol_ref "INTVAL (operands[1])") (const_int 0)) > + (le (symbol_ref "INTVAL (operands[1])") > + (const_int 255)))))) > + (const_int 2) > + (const_int 4)))] > ) How about adding an alternative only enabled for T2 that uses the `l' constraint and inventing new constraints for some of the constant values that are valid for 16 bit instructions since the `I' and `L' constraints have different meanings depending on whether TARGET_32BIT is valid or not ? We could then set the value of the length attribute accordingly. I don't think we can change the meaning of the I and L constraints very easily given the amount of churn that might be needed > > (define_insn "*cmpsi_shiftsi" > @@ -7286,7 +7297,14 @@ > return \"b%d1\\t%l0\"; > " > [(set_attr "conds" "use") > - (set_attr "type" "branch")] > + (set_attr "type" "branch") > + (set (attr "length") > + (if_then_else > + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) > + (and (ge (minus (match_dup 0) (pc)) (const_int -250)) > + (le (minus (match_dup 0) (pc)) (const_int 256)))) > + (const_int 2) > + (const_int 4)))] > ) I don't think this and the other conditional branch instruction changes are correct. This could end up being the last instruction in an IT block and will automatically end up getting the 32 bit encoding and end up causing trouble with the length calculations. Remember the 16 bit encoding for the conditional instruction can't be used as the last instruction in an IT block. > @@ -10256,7 +10288,26 @@ > > return \"\"; > }" > - [(set_attr "type" "store4")] > + [(set_attr "type" "store4") > + (set (attr "length") > + (if_then_else > + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) > + (ne (symbol_ref "{ > + int i, regno, hi_reg; > + int num_saves = XVECLEN (operands[2], 0); > + regno = REGNO (operands[1]); > + hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS) > + && (regno != LR_REGNUM); > + for (i = 1; i< num_saves; i++) (i < num_saves && (hi_reg == 0)) - what's the point of going through the register list when hi_reg != 0 in this case ? A comment to explain that the length calculation *must* be kept in sync with the template above might be useful. cheers Ramana ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-03-30 1:24 ` Ramana Radhakrishnan @ 2011-03-30 7:35 ` Chung-Lin Tang 2011-03-30 12:41 ` Richard Earnshaw 2011-03-31 3:34 ` Carrot Wei 1 sibling, 1 reply; 16+ messages in thread From: Chung-Lin Tang @ 2011-03-30 7:35 UTC (permalink / raw) To: Ramana Radhakrishnan; +Cc: Carrot Wei, gcc-patches On 2011/3/30 06:35 AM, Ramana Radhakrishnan wrote: > Hi Carrot, > > On 26/03/11 15:14, Carrot Wei wrote: >> Index: arm.md >> =================================================================== >> --- arm.md (revision 171337) >> +++ arm.md (working copy) >> @@ -7115,7 +7115,18 @@ >> "@ >> cmp%?\\t%0, %1 >> cmn%?\\t%0, #%n1" >> - [(set_attr "conds" "set")] >> + [(set_attr "conds" "set") >> + (set (attr "length") >> + (if_then_else >> + (and (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) >> + (eq (symbol_ref "which_alternative") (const_int 0))) >> + (ior (ne (symbol_ref "REG_P (operands[1])") (const_int 0)) >> + (and (ne (symbol_ref "CONST_INT_P (operands[1])") (const_int 0)) >> + (and (ge (symbol_ref "INTVAL (operands[1])") (const_int 0)) >> + (le (symbol_ref "INTVAL (operands[1])") >> + (const_int 255)))))) >> + (const_int 2) >> + (const_int 4)))] >> ) > > How about adding an alternative only enabled for T2 that uses the > `l' constraint and inventing new constraints for some of the constant > values that are valid for 16 bit instructions since the `I' and `L' > constraints have different meanings depending on whether TARGET_32BIT is > valid or not ? We could then set the value of the length attribute > accordingly. I don't think we can change the meaning of the I and L > constraints very easily given the amount of churn that might be needed > > >> >> (define_insn "*cmpsi_shiftsi" >> @@ -7286,7 +7297,14 @@ >> return \"b%d1\\t%l0\"; >> " >> [(set_attr "conds" "use") >> - (set_attr "type" "branch")] >> + (set_attr "type" "branch") >> + (set (attr "length") >> + (if_then_else >> + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) >> + (and (ge (minus (match_dup 0) (pc)) (const_int -250)) >> + (le (minus (match_dup 0) (pc)) (const_int 256)))) >> + (const_int 2) >> + (const_int 4)))] >> ) > > I don't think this and the other conditional branch instruction > changes are correct. This could end up being the last instruction in an > IT block and will automatically end up getting the 32 bit encoding and > end up causing trouble with the length calculations. Remember the 16 bit > encoding for the conditional instruction can't be used as the last > instruction in an IT block. > > >> @@ -10256,7 +10288,26 @@ >> >> return \"\"; >> }" >> - [(set_attr "type" "store4")] >> + [(set_attr "type" "store4") >> + (set (attr "length") >> + (if_then_else >> + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) >> + (ne (symbol_ref "{ >> + int i, regno, hi_reg; >> + int num_saves = XVECLEN (operands[2], 0); >> + regno = REGNO (operands[1]); >> + hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS) >> + && (regno != LR_REGNUM); >> + for (i = 1; i< num_saves; i++) > > > (i < num_saves && (hi_reg == 0)) - what's the point of going through > the register list when hi_reg != 0 in this case ? A comment to explain > that the length calculation *must* be kept in sync with the template > above might be useful. I suggest abstracting all of this Thumb-2 insn length knowledge into a arm.c function, say "thumb2_16bit_insn_p()" to test for 2-byte patterns. Adding more of these quite large pieces of logic into arm.md does not look very pretty to me, and centralizing it into a C function will also make it easier to manage the cases. Chung-Lin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-03-30 7:35 ` Chung-Lin Tang @ 2011-03-30 12:41 ` Richard Earnshaw 0 siblings, 0 replies; 16+ messages in thread From: Richard Earnshaw @ 2011-03-30 12:41 UTC (permalink / raw) To: Chung-Lin Tang; +Cc: Ramana Radhakrishnan, Carrot Wei, gcc-patches On Wed, 2011-03-30 at 12:38 +0800, Chung-Lin Tang wrote: > On 2011/3/30 06:35 AM, Ramana Radhakrishnan wrote: > > Hi Carrot, > > > > On 26/03/11 15:14, Carrot Wei wrote: > >> Index: arm.md > >> =================================================================== > >> --- arm.md (revision 171337) > >> +++ arm.md (working copy) > >> @@ -7115,7 +7115,18 @@ > >> "@ > >> cmp%?\\t%0, %1 > >> cmn%?\\t%0, #%n1" > >> - [(set_attr "conds" "set")] > >> + [(set_attr "conds" "set") > >> + (set (attr "length") > >> + (if_then_else > >> + (and (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) > >> + (eq (symbol_ref "which_alternative") (const_int 0))) > >> + (ior (ne (symbol_ref "REG_P (operands[1])") (const_int 0)) > >> + (and (ne (symbol_ref "CONST_INT_P (operands[1])") (const_int 0)) > >> + (and (ge (symbol_ref "INTVAL (operands[1])") (const_int 0)) > >> + (le (symbol_ref "INTVAL (operands[1])") > >> + (const_int 255)))))) > >> + (const_int 2) > >> + (const_int 4)))] > >> ) > > > > How about adding an alternative only enabled for T2 that uses the > > `l' constraint and inventing new constraints for some of the constant > > values that are valid for 16 bit instructions since the `I' and `L' > > constraints have different meanings depending on whether TARGET_32BIT is > > valid or not ? We could then set the value of the length attribute > > accordingly. I don't think we can change the meaning of the I and L > > constraints very easily given the amount of churn that might be needed > > > > > >> > >> (define_insn "*cmpsi_shiftsi" > >> @@ -7286,7 +7297,14 @@ > >> return \"b%d1\\t%l0\"; > >> " > >> [(set_attr "conds" "use") > >> - (set_attr "type" "branch")] > >> + (set_attr "type" "branch") > >> + (set (attr "length") > >> + (if_then_else > >> + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) > >> + (and (ge (minus (match_dup 0) (pc)) (const_int -250)) > >> + (le (minus (match_dup 0) (pc)) (const_int 256)))) > >> + (const_int 2) > >> + (const_int 4)))] > >> ) > > > > I don't think this and the other conditional branch instruction > > changes are correct. This could end up being the last instruction in an > > IT block and will automatically end up getting the 32 bit encoding and > > end up causing trouble with the length calculations. Remember the 16 bit > > encoding for the conditional instruction can't be used as the last > > instruction in an IT block. > > > > > >> @@ -10256,7 +10288,26 @@ > >> > >> return \"\"; > >> }" > >> - [(set_attr "type" "store4")] > >> + [(set_attr "type" "store4") > >> + (set (attr "length") > >> + (if_then_else > >> + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) > >> + (ne (symbol_ref "{ > >> + int i, regno, hi_reg; > >> + int num_saves = XVECLEN (operands[2], 0); > >> + regno = REGNO (operands[1]); > >> + hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS) > >> + && (regno != LR_REGNUM); > >> + for (i = 1; i< num_saves; i++) > > > > > > (i < num_saves && (hi_reg == 0)) - what's the point of going through > > the register list when hi_reg != 0 in this case ? A comment to explain > > that the length calculation *must* be kept in sync with the template > > above might be useful. > > I suggest abstracting all of this Thumb-2 insn length knowledge into a > arm.c function, say "thumb2_16bit_insn_p()" to test for 2-byte patterns. > Adding more of these quite large pieces of logic into arm.md does not > look very pretty to me, and centralizing it into a C function will also > make it easier to manage the cases. > > Chung-Lin > I think Ramana's suggestion should avoid the need for any arithmetic at all here. If the constraints precisely match a 16-bit instruction (and that alternative is disbled for ARM) then we can say unconditionally that the length of the insn is 2. R. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-03-30 1:24 ` Ramana Radhakrishnan 2011-03-30 7:35 ` Chung-Lin Tang @ 2011-03-31 3:34 ` Carrot Wei 2011-03-31 16:04 ` Ramana Radhakrishnan 1 sibling, 1 reply; 16+ messages in thread From: Carrot Wei @ 2011-03-31 3:34 UTC (permalink / raw) To: Ramana Radhakrishnan; +Cc: gcc-patches Hi Ramana On Wed, Mar 30, 2011 at 6:35 AM, Ramana Radhakrishnan <ramana.radhakrishnan@linaro.org> wrote: > Hi Carrot, > > > How about adding an alternative only enabled for T2 that uses the `l' > constraint and inventing new constraints for some of the constant values > that are valid for 16 bit instructions since the `I' and `L' constraints > have different meanings depending on whether TARGET_32BIT is valid or not ? > We could then set the value of the length attribute accordingly. I don't > think we can change the meaning of the I and L constraints very easily given > the amount of churn that might be needed > You are right. Now the logic is much clearer by splitting the constraints. > > I don't think this and the other conditional branch instruction > changes are correct. This could end up being the last instruction in an IT > block and will automatically end up getting the 32 bit encoding and end up > causing trouble with the length calculations. Remember the 16 bit encoding > for the conditional instruction can't be used as the last instruction in an > IT block. > According to arm architecture reference manual, chapter A8.6.16, neither 16 bit nor 32 bit conditional branch can be used in IT block. Only unconditional branch can be used as the last instruction in IT block, and it isn't related to instruction length. So we don't need to care about branch instruction encoding in IT block. > > (i < num_saves && (hi_reg == 0)) - what's the point of going through > the register list when hi_reg != 0 in this case ? A comment to explain that > the length calculation *must* be kept in sync with the template above might > be useful. done. The following patch has been tested on arm qemu. ChangeLog: 2011-03-31 Wei Guozhi <carrot@google.com> PR target/47855 * config/arm/arm.md (arm_cmpsi_insn): Compute attr "length". (arm_cond_branch): Likewise. (arm_cond_branch_reversed): Likewise. (arm_jump): Likewise. (push_multi): Likewise. * config/arm/constraints.md (Py): New constraint. Index: constraints.md =================================================================== --- constraints.md (revision 171337) +++ constraints.md (working copy) @@ -31,7 +31,7 @@ ;; The following multi-letter normal constraints have been used: ;; in ARM/Thumb-2 state: Da, Db, Dc, Dn, Dl, DL, Dv, Dy, Di, Dz ;; in Thumb-1 state: Pa, Pb, Pc, Pd -;; in Thumb-2 state: Ps, Pt, Pu, Pv, Pw, Px +;; in Thumb-2 state: Ps, Pt, Pu, Pv, Pw, Px, Py ;; The following memory constraints have been used: ;; in ARM/Thumb-2 state: Q, Ut, Uv, Uy, Un, Um, Us @@ -189,6 +189,11 @@ (and (match_code "const_int") (match_test "TARGET_THUMB2 && ival >= -7 && ival <= -1"))) +(define_constraint "Py" + "@internal In Thumb-2 state a constant in the range 0 to 255" + (and (match_code "const_int") + (match_test "TARGET_THUMB2 && ival >= 0 && ival <= 255"))) + (define_constraint "G" "In ARM/Thumb-2 state a valid FPA immediate constant." (and (match_code "const_double") Index: arm.md =================================================================== --- arm.md (revision 171337) +++ arm.md (working copy) @@ -7109,13 +7109,21 @@ (define_insn "*arm_cmpsi_insn" [(set (reg:CC CC_REGNUM) - (compare:CC (match_operand:SI 0 "s_register_operand" "r,r") - (match_operand:SI 1 "arm_add_operand" "rI,L")))] + (compare:CC (match_operand:SI 0 "s_register_operand" "l,r,r,r") + (match_operand:SI 1 "arm_add_operand" "Py,r,I,L")))] "TARGET_32BIT" "@ cmp%?\\t%0, %1 + cmp%?\\t%0, %1 + cmp%?\\t%0, %1 cmn%?\\t%0, #%n1" - [(set_attr "conds" "set")] + [(set_attr "conds" "set") + (set (attr "length") + (if_then_else + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) + (lt (symbol_ref "which_alternative") (const_int 2))) + (const_int 2) + (const_int 4)))] ) (define_insn "*cmpsi_shiftsi" @@ -7286,7 +7294,14 @@ return \"b%d1\\t%l0\"; " [(set_attr "conds" "use") - (set_attr "type" "branch")] + (set_attr "type" "branch") + (set (attr "length") + (if_then_else + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) + (and (ge (minus (match_dup 0) (pc)) (const_int -250)) + (le (minus (match_dup 0) (pc)) (const_int 256)))) + (const_int 2) + (const_int 4)))] ) (define_insn "*arm_cond_branch_reversed" @@ -7305,7 +7320,14 @@ return \"b%D1\\t%l0\"; " [(set_attr "conds" "use") - (set_attr "type" "branch")] + (set_attr "type" "branch") + (set (attr "length") + (if_then_else + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) + (and (ge (minus (match_dup 0) (pc)) (const_int -250)) + (le (minus (match_dup 0) (pc)) (const_int 256)))) + (const_int 2) + (const_int 4)))] ) @@ -7757,7 +7779,14 @@ return \"b%?\\t%l0\"; } " - [(set_attr "predicable" "yes")] + [(set_attr "predicable" "yes") + (set (attr "length") + (if_then_else + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) + (and (ge (minus (match_dup 0) (pc)) (const_int -2044)) + (le (minus (match_dup 0) (pc)) (const_int 2048)))) + (const_int 2) + (const_int 4)))] ) (define_insn "*thumb_jump" @@ -10256,7 +10285,29 @@ return \"\"; }" - [(set_attr "type" "store4")] + [(set_attr "type" "store4") + (set (attr "length") + (if_then_else + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) + (ne (symbol_ref "{ + /* Check if there are any high register (except lr) + references in the list. KEEP the following iteration + in sync with the template above. */ + int i, regno, hi_reg; + int num_saves = XVECLEN (operands[2], 0); + regno = REGNO (operands[1]); + hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS) + && (regno != LR_REGNUM); + for (i = 1; i < num_saves && !hi_reg; i++) + { + regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0)); + hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS) + && (regno != LR_REGNUM); + } + !hi_reg; }") + (const_int 0))) + (const_int 2) + (const_int 4)))] ) (define_insn "stack_tie" ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-03-31 3:34 ` Carrot Wei @ 2011-03-31 16:04 ` Ramana Radhakrishnan 2011-04-01 6:57 ` Carrot Wei 0 siblings, 1 reply; 16+ messages in thread From: Ramana Radhakrishnan @ 2011-03-31 16:04 UTC (permalink / raw) To: Carrot Wei; +Cc: gcc-patches >> Hi Carrot, >> >> >> How about adding an alternative only enabled for T2 that uses the `l' >> constraint and inventing new constraints for some of the constant values >> that are valid for 16 bit instructions since the `I' and `L' constraints >> have different meanings depending on whether TARGET_32BIT is valid or not ? >> We could then set the value of the length attribute accordingly. I don't >> think we can change the meaning of the I and L constraints very easily given >> the amount of churn that might be needed >> > You are right. Now the logic is much clearer by splitting the constraints. Sorry I wasn't too clear . What I meant was to use the "enabled" trick and enable this only when Thumb2 is enabled in the compiler. So what you could do instead is add the alternative and then use the arch attribute to enable this only for Thumb2. i.e. >> (define_insn "*arm_cmpsi_insn" >> [(set (reg:CC CC_REGNUM) >> - (compare:CC (match_operand:SI 0 "s_register_operand" "r,r") >> - (match_operand:SI 1 "arm_add_operand" "rI,L")))] >> + (compare:CC (match_operand:SI 0 "s_register_operand" "l,r,r,r") >> + (match_operand:SI 1 "arm_add_operand" "Py,r,I,L")))] >> "TARGET_32BIT" >> "@ >> cmp%?\\t%0, %1 >> + cmp%?\\t%0, %1 >> + cmp%?\\t%0, %1 >> cmn%?\\t%0, #%n1" >> - [(set_attr "conds" "set")] >> + [(set_attr "conds" "set") (set_attr "arch" "t2, any") (set_attr "length" "2, 4")] Does that look any better ? That way you can now safely add options on a per arch basis and reduce the amount of complexity for some of these length calculations. >> >> I don't think this and the other conditional branch instruction >> changes are correct. This could end up being the last instruction in an IT >> block and will automatically end up getting the 32 bit encoding and end up >> causing trouble with the length calculations. Remember the 16 bit encoding >> for the conditional instruction can't be used as the last instruction in an >> IT block. >> > According to arm architecture reference manual, chapter A8.6.16, neither 16 bit > nor 32 bit conditional branch can be used in IT block. Only unconditional branch > can be used as the last instruction in IT block, and it isn't related > to instruction > length. So we don't need to care about branch instruction encoding in IT block. Yes I think you are right in this case. The reason I think I misinterpreted this was ofcourse the fact that conditional branches as the last instruction in the ARM-ARM. I don't know why I misread it last night. cheers Ramana ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-03-31 16:04 ` Ramana Radhakrishnan @ 2011-04-01 6:57 ` Carrot Wei 2011-04-05 13:32 ` Ramana Radhakrishnan 2011-04-07 9:32 ` Richard Sandiford 0 siblings, 2 replies; 16+ messages in thread From: Carrot Wei @ 2011-04-01 6:57 UTC (permalink / raw) To: Ramana Radhakrishnan; +Cc: gcc-patches Hi Ramana On Thu, Mar 31, 2011 at 11:57 PM, Ramana Radhakrishnan <ramana.radhakrishnan@linaro.org> wrote: >>> Hi Carrot, >>> How about adding an alternative only enabled for T2 that uses the >>> `l' >>> constraint and inventing new constraints for some of the constant values >>> that are valid for 16 bit instructions since the `I' and `L' constraints >>> have different meanings depending on whether TARGET_32BIT is valid or not >>> ? >>> We could then set the value of the length attribute accordingly. I don't >>> think we can change the meaning of the I and L constraints very easily >>> given >>> the amount of churn that might be needed >>> >> You are right. Now the logic is much clearer by splitting the constraints. > > Sorry I wasn't too clear . > > What I meant was to use the "enabled" trick and enable this only when Thumb2 > is enabled in the compiler. > > So what you could do instead is add the alternative and then use the arch > attribute to enable this only for Thumb2. > > i.e. > >>> (define_insn "*arm_cmpsi_insn" >>> [(set (reg:CC CC_REGNUM) >>> - (compare:CC (match_operand:SI 0 "s_register_operand" "r,r") >>> - (match_operand:SI 1 "arm_add_operand" "rI,L")))] >>> + (compare:CC (match_operand:SI 0 "s_register_operand" "l,r,r,r") >>> + (match_operand:SI 1 "arm_add_operand" >>> "Py,r,I,L")))] >>> "TARGET_32BIT" >>> "@ >>> cmp%?\\t%0, %1 >>> + cmp%?\\t%0, %1 >>> + cmp%?\\t%0, %1 >>> cmn%?\\t%0, #%n1" >>> - [(set_attr "conds" "set")] >>> + [(set_attr "conds" "set") > > (set_attr "arch" "t2, any") > (set_attr "length" "2, 4")] > > Does that look any better ? That way you can now safely add options on a per > arch basis and reduce the amount of complexity for some of these length > calculations. > Thank you for the knowledge, this is the first time I see an usage of attribute "arch". I modified the pattern by enabling the new alternatives for thumb2 only and set attribute "length" accordingly. The patch was tested on arm qemu without regression, OK for install? thanks Carrot ChangeLog: 2011-04-01 Wei Guozhi <carrot@google.com> PR target/47855 * config/arm/arm.md (arm_cmpsi_insn): Compute attr "length". (arm_cond_branch): Likewise. (arm_cond_branch_reversed): Likewise. (arm_jump): Likewise. (push_multi): Likewise. * config/arm/constraints.md (Py): New constraint. Index: constraints.md =================================================================== --- constraints.md (revision 171337) +++ constraints.md (working copy) @@ -31,7 +31,7 @@ ;; The following multi-letter normal constraints have been used: ;; in ARM/Thumb-2 state: Da, Db, Dc, Dn, Dl, DL, Dv, Dy, Di, Dz ;; in Thumb-1 state: Pa, Pb, Pc, Pd -;; in Thumb-2 state: Ps, Pt, Pu, Pv, Pw, Px +;; in Thumb-2 state: Ps, Pt, Pu, Pv, Pw, Px, Py ;; The following memory constraints have been used: ;; in ARM/Thumb-2 state: Q, Ut, Uv, Uy, Un, Um, Us @@ -189,6 +189,11 @@ (and (match_code "const_int") (match_test "TARGET_THUMB2 && ival >= -7 && ival <= -1"))) +(define_constraint "Py" + "@internal In Thumb-2 state a constant in the range 0 to 255" + (and (match_code "const_int") + (match_test "TARGET_THUMB2 && ival >= 0 && ival <= 255"))) + (define_constraint "G" "In ARM/Thumb-2 state a valid FPA immediate constant." (and (match_code "const_double") Index: arm.md =================================================================== --- arm.md (revision 171337) +++ arm.md (working copy) @@ -7109,13 +7109,17 @@ (define_insn "*arm_cmpsi_insn" [(set (reg:CC CC_REGNUM) - (compare:CC (match_operand:SI 0 "s_register_operand" "r,r") - (match_operand:SI 1 "arm_add_operand" "rI,L")))] + (compare:CC (match_operand:SI 0 "s_register_operand" "l,r,r,r") + (match_operand:SI 1 "arm_add_operand" "Py,r,rI,L")))] "TARGET_32BIT" "@ cmp%?\\t%0, %1 + cmp%?\\t%0, %1 + cmp%?\\t%0, %1 cmn%?\\t%0, #%n1" - [(set_attr "conds" "set")] + [(set_attr "conds" "set") + (set_attr "arch" "t2,t2,any,any") + (set_attr "length" "2,2,4,4")] ) (define_insn "*cmpsi_shiftsi" @@ -7286,7 +7290,14 @@ return \"b%d1\\t%l0\"; " [(set_attr "conds" "use") - (set_attr "type" "branch")] + (set_attr "type" "branch") + (set (attr "length") + (if_then_else + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) + (and (ge (minus (match_dup 0) (pc)) (const_int -250)) + (le (minus (match_dup 0) (pc)) (const_int 256)))) + (const_int 2) + (const_int 4)))] ) (define_insn "*arm_cond_branch_reversed" @@ -7305,7 +7316,14 @@ return \"b%D1\\t%l0\"; " [(set_attr "conds" "use") - (set_attr "type" "branch")] + (set_attr "type" "branch") + (set (attr "length") + (if_then_else + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) + (and (ge (minus (match_dup 0) (pc)) (const_int -250)) + (le (minus (match_dup 0) (pc)) (const_int 256)))) + (const_int 2) + (const_int 4)))] ) @@ -7757,7 +7775,14 @@ return \"b%?\\t%l0\"; } " - [(set_attr "predicable" "yes")] + [(set_attr "predicable" "yes") + (set (attr "length") + (if_then_else + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) + (and (ge (minus (match_dup 0) (pc)) (const_int -2044)) + (le (minus (match_dup 0) (pc)) (const_int 2048)))) + (const_int 2) + (const_int 4)))] ) (define_insn "*thumb_jump" @@ -10256,7 +10281,29 @@ return \"\"; }" - [(set_attr "type" "store4")] + [(set_attr "type" "store4") + (set (attr "length") + (if_then_else + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) + (ne (symbol_ref "{ + /* Check if there are any high register (except lr) + references in the list. KEEP the following iteration + in sync with the template above. */ + int i, regno, hi_reg; + int num_saves = XVECLEN (operands[2], 0); + regno = REGNO (operands[1]); + hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS) + && (regno != LR_REGNUM); + for (i = 1; i < num_saves && !hi_reg; i++) + { + regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0)); + hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS) + && (regno != LR_REGNUM); + } + !hi_reg; }") + (const_int 0))) + (const_int 2) + (const_int 4)))] ) (define_insn "stack_tie" ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-04-01 6:57 ` Carrot Wei @ 2011-04-05 13:32 ` Ramana Radhakrishnan 2011-04-06 2:26 ` Carrot Wei 2011-04-07 9:32 ` Richard Sandiford 1 sibling, 1 reply; 16+ messages in thread From: Ramana Radhakrishnan @ 2011-04-05 13:32 UTC (permalink / raw) To: Carrot Wei; +Cc: gcc-patches > > The patch was tested on arm qemu without regression, OK for install? Ok if no regressions. cheers Ramana > > thanks > Carrot > > > ChangeLog: > 2011-04-01 Wei Guozhi<carrot@google.com> > > PR target/47855 > * config/arm/arm.md (arm_cmpsi_insn): Compute attr "length". > (arm_cond_branch): Likewise. > (arm_cond_branch_reversed): Likewise. > (arm_jump): Likewise. > (push_multi): Likewise. > * config/arm/constraints.md (Py): New constraint. > > > Index: constraints.md > =================================================================== > --- constraints.md (revision 171337) > +++ constraints.md (working copy) > @@ -31,7 +31,7 @@ > ;; The following multi-letter normal constraints have been used: > ;; in ARM/Thumb-2 state: Da, Db, Dc, Dn, Dl, DL, Dv, Dy, Di, Dz > ;; in Thumb-1 state: Pa, Pb, Pc, Pd > -;; in Thumb-2 state: Ps, Pt, Pu, Pv, Pw, Px > +;; in Thumb-2 state: Ps, Pt, Pu, Pv, Pw, Px, Py > > ;; The following memory constraints have been used: > ;; in ARM/Thumb-2 state: Q, Ut, Uv, Uy, Un, Um, Us > @@ -189,6 +189,11 @@ > (and (match_code "const_int") > (match_test "TARGET_THUMB2&& ival>= -7&& ival<= -1"))) > > +(define_constraint "Py" > + "@internal In Thumb-2 state a constant in the range 0 to 255" > + (and (match_code "const_int") > + (match_test "TARGET_THUMB2&& ival>= 0&& ival<= 255"))) > + > (define_constraint "G" > "In ARM/Thumb-2 state a valid FPA immediate constant." > (and (match_code "const_double") > Index: arm.md > =================================================================== > --- arm.md (revision 171337) > +++ arm.md (working copy) > @@ -7109,13 +7109,17 @@ > > (define_insn "*arm_cmpsi_insn" > [(set (reg:CC CC_REGNUM) > - (compare:CC (match_operand:SI 0 "s_register_operand" "r,r") > - (match_operand:SI 1 "arm_add_operand" "rI,L")))] > + (compare:CC (match_operand:SI 0 "s_register_operand" "l,r,r,r") > + (match_operand:SI 1 "arm_add_operand" "Py,r,rI,L")))] > "TARGET_32BIT" > "@ > cmp%?\\t%0, %1 > + cmp%?\\t%0, %1 > + cmp%?\\t%0, %1 > cmn%?\\t%0, #%n1" > - [(set_attr "conds" "set")] > + [(set_attr "conds" "set") > + (set_attr "arch" "t2,t2,any,any") > + (set_attr "length" "2,2,4,4")] > ) > > (define_insn "*cmpsi_shiftsi" > @@ -7286,7 +7290,14 @@ > return \"b%d1\\t%l0\"; > " > [(set_attr "conds" "use") > - (set_attr "type" "branch")] > + (set_attr "type" "branch") > + (set (attr "length") > + (if_then_else > + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) > + (and (ge (minus (match_dup 0) (pc)) (const_int -250)) > + (le (minus (match_dup 0) (pc)) (const_int 256)))) > + (const_int 2) > + (const_int 4)))] > ) > > (define_insn "*arm_cond_branch_reversed" > @@ -7305,7 +7316,14 @@ > return \"b%D1\\t%l0\"; > " > [(set_attr "conds" "use") > - (set_attr "type" "branch")] > + (set_attr "type" "branch") > + (set (attr "length") > + (if_then_else > + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) > + (and (ge (minus (match_dup 0) (pc)) (const_int -250)) > + (le (minus (match_dup 0) (pc)) (const_int 256)))) > + (const_int 2) > + (const_int 4)))] > ) > > > > @@ -7757,7 +7775,14 @@ > return \"b%?\\t%l0\"; > } > " > - [(set_attr "predicable" "yes")] > + [(set_attr "predicable" "yes") > + (set (attr "length") > + (if_then_else > + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) > + (and (ge (minus (match_dup 0) (pc)) (const_int -2044)) > + (le (minus (match_dup 0) (pc)) (const_int 2048)))) > + (const_int 2) > + (const_int 4)))] > ) > > (define_insn "*thumb_jump" > @@ -10256,7 +10281,29 @@ > > return \"\"; > }" > - [(set_attr "type" "store4")] > + [(set_attr "type" "store4") > + (set (attr "length") > + (if_then_else > + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) > + (ne (symbol_ref "{ > + /* Check if there are any high register (except lr) > + references in the list. KEEP the following iteration > + in sync with the template above. */ > + int i, regno, hi_reg; > + int num_saves = XVECLEN (operands[2], 0); > + regno = REGNO (operands[1]); > + hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS) > + && (regno != LR_REGNUM); > + for (i = 1; i< num_saves&& !hi_reg; i++) > + { > + regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0)); > + hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS) > + && (regno != LR_REGNUM); > + } > + !hi_reg; }") > + (const_int 0))) > + (const_int 2) > + (const_int 4)))] > ) > > (define_insn "stack_tie" ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-04-05 13:32 ` Ramana Radhakrishnan @ 2011-04-06 2:26 ` Carrot Wei 0 siblings, 0 replies; 16+ messages in thread From: Carrot Wei @ 2011-04-06 2:26 UTC (permalink / raw) To: Ramana Radhakrishnan; +Cc: gcc-patches Committed as r172017. thanks Carrot On Tue, Apr 5, 2011 at 9:32 PM, Ramana Radhakrishnan <ramana.radhakrishnan@linaro.org> wrote: > >> >> The patch was tested on arm qemu without regression, OK for install? > > Ok if no regressions. > > cheers > Ramana > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-04-01 6:57 ` Carrot Wei 2011-04-05 13:32 ` Ramana Radhakrishnan @ 2011-04-07 9:32 ` Richard Sandiford 2011-04-07 11:09 ` Carrot Wei 1 sibling, 1 reply; 16+ messages in thread From: Richard Sandiford @ 2011-04-07 9:32 UTC (permalink / raw) To: Carrot Wei; +Cc: Ramana Radhakrishnan, gcc-patches Hi Carrot, Sorry if this has already been reported, but the patch breaks bootstrap of arm-linux-gnueabi (or cross builds with --enable-werror). The problem is that this... Carrot Wei <carrot@google.com> writes: > @@ -10256,7 +10281,29 @@ > > return \"\"; > }" > - [(set_attr "type" "store4")] > + [(set_attr "type" "store4") > + (set (attr "length") > + (if_then_else > + (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) > + (ne (symbol_ref "{ > + /* Check if there are any high register (except lr) > + references in the list. KEEP the following iteration > + in sync with the template above. */ > + int i, regno, hi_reg; > + int num_saves = XVECLEN (operands[2], 0); > + regno = REGNO (operands[1]); > + hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS) > + && (regno != LR_REGNUM); > + for (i = 1; i < num_saves && !hi_reg; i++) > + { > + regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0)); > + hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS) > + && (regno != LR_REGNUM); > + } > + !hi_reg; }") > + (const_int 0))) > + (const_int 2) > + (const_int 4)))] uses a statement expression -- i.e. ({ code; result; }) -- which is a GNU extension. I suppose a simple fix would be to put the code into an arm.c function. Richard ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-04-07 9:32 ` Richard Sandiford @ 2011-04-07 11:09 ` Carrot Wei 2011-04-07 11:31 ` Ramana Radhakrishnan 0 siblings, 1 reply; 16+ messages in thread From: Carrot Wei @ 2011-04-07 11:09 UTC (permalink / raw) To: Carrot Wei, Ramana Radhakrishnan, gcc-patches, richard.sandiford On Thu, Apr 7, 2011 at 5:31 PM, Richard Sandiford <richard.sandiford@linaro.org> wrote: > Hi Carrot, > > Sorry if this has already been reported, but the patch breaks bootstrap > of arm-linux-gnueabi (or cross builds with --enable-werror). The problem > is that this... > > uses a statement expression -- i.e. ({ code; result; }) -- which is > a GNU extension. > > I suppose a simple fix would be to put the code into an arm.c function. > Thank you for the report, I will add this fix to the second part of the patch. Carrot ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-04-07 11:09 ` Carrot Wei @ 2011-04-07 11:31 ` Ramana Radhakrishnan 2011-04-08 7:36 ` Carrot Wei 0 siblings, 1 reply; 16+ messages in thread From: Ramana Radhakrishnan @ 2011-04-07 11:31 UTC (permalink / raw) To: Carrot Wei; +Cc: gcc-patches, richard.sandiford On 07/04/11 12:08, Carrot Wei wrote: > On Thu, Apr 7, 2011 at 5:31 PM, Richard Sandiford > <richard.sandiford@linaro.org> wrote: >> Hi Carrot, >> >> Sorry if this has already been reported, but the patch breaks bootstrap >> of arm-linux-gnueabi (or cross builds with --enable-werror). The problem >> is that this... >> >> uses a statement expression -- i.e. ({ code; result; }) -- which is >> a GNU extension. >> >> I suppose a simple fix would be to put the code into an arm.c function. >> > > Thank you for the report, I will add this fix to the second part of the patch. It would be worth unbreaking bootstrap as a separate patch and not conflating it with a different set of improvements. Ramana > > Carrot ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-04-07 11:31 ` Ramana Radhakrishnan @ 2011-04-08 7:36 ` Carrot Wei 2011-04-08 8:36 ` Ramana Radhakrishnan 0 siblings, 1 reply; 16+ messages in thread From: Carrot Wei @ 2011-04-08 7:36 UTC (permalink / raw) To: Ramana Radhakrishnan; +Cc: gcc-patches, richard.sandiford Hi This patch moves the length computation of push_multi into a C function to avoid the usage of GNU extension. Tested on qemu without regression. OK to install? thanks Carrot ChangeLog: 2011-04-08 Wei Guozhi <carrot@google.com> PR target/47855 * config/arm/arm-protos.h (arm_attr_length_push_multi): New prototype. * config/arm/arm.c (arm_attr_length_push_multi): New function. * config/arm/arm.md (*push_multi): Change the length computation to call a C function. Index: arm.c =================================================================== --- arm.c (revision 172158) +++ arm.c (working copy) @@ -23696,4 +23696,30 @@ arm_preferred_rename_class (reg_class_t return NO_REGS; } +/* Compute the atrribute "length" of insn "*push_multi". + So this function MUST be kept in sync with that insn pattern. */ +int +arm_attr_length_push_multi(rtx parallel_op, rtx first_op) +{ + int i, regno, hi_reg; + int num_saves = XVECLEN (parallel_op, 0); + + /* ARM mode. */ + if (TARGET_ARM) + return 4; + + /* Thumb2 mode. */ + regno = REGNO (first_op); + hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS) && (regno != LR_REGNUM); + for (i = 1; i < num_saves && !hi_reg; i++) + { + regno = REGNO (XEXP (XVECEXP (parallel_op, 0, i), 0)); + hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS) && (regno != LR_REGNUM); + } + + if (!hi_reg) + return 2; + return 4; +} + #include "gt-arm.h" Index: arm-protos.h =================================================================== --- arm-protos.h (revision 172158) +++ arm-protos.h (working copy) @@ -152,6 +152,7 @@ extern void arm_expand_sync (enum machin extern const char *arm_output_memory_barrier (rtx *); extern const char *arm_output_sync_insn (rtx, rtx *); extern unsigned int arm_sync_loop_insns (rtx , rtx *); +extern int arm_attr_length_push_multi(rtx, rtx); #if defined TREE_CODE extern void arm_init_cumulative_args (CUMULATIVE_ARGS *, tree, rtx, tree); Index: arm.md =================================================================== --- arm.md (revision 172158) +++ arm.md (working copy) @@ -10290,27 +10290,7 @@ }" [(set_attr "type" "store4") (set (attr "length") - (if_then_else - (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) - (ne (symbol_ref "{ - /* Check if there are any high register (except lr) - references in the list. KEEP the following iteration - in sync with the template above. */ - int i, regno, hi_reg; - int num_saves = XVECLEN (operands[2], 0); - regno = REGNO (operands[1]); - hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS) - && (regno != LR_REGNUM); - for (i = 1; i < num_saves && !hi_reg; i++) - { - regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0)); - hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS) - && (regno != LR_REGNUM); - } - !hi_reg; }") - (const_int 0))) - (const_int 2) - (const_int 4)))] + (symbol_ref "arm_attr_length_push_multi (operands[2], operands[1])"))] ) (define_insn "stack_tie" On Thu, Apr 7, 2011 at 7:30 PM, Ramana Radhakrishnan <ramana.radhakrishnan@linaro.org> wrote: > On 07/04/11 12:08, Carrot Wei wrote: >> >> On Thu, Apr 7, 2011 at 5:31 PM, Richard Sandiford >> <richard.sandiford@linaro.org> wrote: >>> >>> Hi Carrot, >>> >>> Sorry if this has already been reported, but the patch breaks bootstrap >>> of arm-linux-gnueabi (or cross builds with --enable-werror). The problem >>> is that this... >>> >>> uses a statement expression -- i.e. ({ code; result; }) -- which is >>> a GNU extension. >>> >>> I suppose a simple fix would be to put the code into an arm.c function. >>> >> >> Thank you for the report, I will add this fix to the second part of the >> patch. > > It would be worth unbreaking bootstrap as a separate patch and not > conflating it with a different set of improvements. > > Ramana > >> >> Carrot > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-04-08 7:36 ` Carrot Wei @ 2011-04-08 8:36 ` Ramana Radhakrishnan 2011-04-08 8:52 ` Carrot Wei 0 siblings, 1 reply; 16+ messages in thread From: Ramana Radhakrishnan @ 2011-04-08 8:36 UTC (permalink / raw) To: Carrot Wei; +Cc: gcc-patches, richard.sandiford On 08/04/11 08:36, Carrot Wei wrote: > Hi > > This patch moves the length computation of push_multi into a C > function to avoid the usage of GNU extension. Please put the comment regarding the calculation of the length attribute along with the pattern in thumb2.md as well. > > Tested on qemu without regression. OK to install? Ok with that change. Thanks for fixing this up. cheers Ramana > > thanks > Carrot > > ChangeLog: > 2011-04-08 Wei Guozhi<carrot@google.com> > > PR target/47855 > * config/arm/arm-protos.h (arm_attr_length_push_multi): New prototype. > * config/arm/arm.c (arm_attr_length_push_multi): New function. > * config/arm/arm.md (*push_multi): Change the length computation to > call a C function. > > > Index: arm.c > =================================================================== > --- arm.c (revision 172158) > +++ arm.c (working copy) > @@ -23696,4 +23696,30 @@ arm_preferred_rename_class (reg_class_t > return NO_REGS; > } > > +/* Compute the atrribute "length" of insn "*push_multi". > + So this function MUST be kept in sync with that insn pattern. */ > +int > +arm_attr_length_push_multi(rtx parallel_op, rtx first_op) > +{ > + int i, regno, hi_reg; > + int num_saves = XVECLEN (parallel_op, 0); > + > + /* ARM mode. */ > + if (TARGET_ARM) > + return 4; > + > + /* Thumb2 mode. */ > + regno = REGNO (first_op); > + hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)&& (regno != LR_REGNUM); > + for (i = 1; i< num_saves&& !hi_reg; i++) > + { > + regno = REGNO (XEXP (XVECEXP (parallel_op, 0, i), 0)); > + hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)&& (regno != LR_REGNUM); > + } > + > + if (!hi_reg) > + return 2; > + return 4; > +} > + > #include "gt-arm.h" > Index: arm-protos.h > =================================================================== > --- arm-protos.h (revision 172158) > +++ arm-protos.h (working copy) > @@ -152,6 +152,7 @@ extern void arm_expand_sync (enum machin > extern const char *arm_output_memory_barrier (rtx *); > extern const char *arm_output_sync_insn (rtx, rtx *); > extern unsigned int arm_sync_loop_insns (rtx , rtx *); > +extern int arm_attr_length_push_multi(rtx, rtx); > > #if defined TREE_CODE > extern void arm_init_cumulative_args (CUMULATIVE_ARGS *, tree, rtx, tree); > Index: arm.md > =================================================================== > --- arm.md (revision 172158) > +++ arm.md (working copy) > @@ -10290,27 +10290,7 @@ > }" > [(set_attr "type" "store4") > (set (attr "length") > - (if_then_else > - (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) > - (ne (symbol_ref "{ > - /* Check if there are any high register (except lr) > - references in the list. KEEP the following iteration > - in sync with the template above. */ > - int i, regno, hi_reg; > - int num_saves = XVECLEN (operands[2], 0); > - regno = REGNO (operands[1]); > - hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS) > - && (regno != LR_REGNUM); > - for (i = 1; i< num_saves&& !hi_reg; i++) > - { > - regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0)); > - hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS) > - && (regno != LR_REGNUM); > - } > - !hi_reg; }") > - (const_int 0))) > - (const_int 2) > - (const_int 4)))] > + (symbol_ref "arm_attr_length_push_multi (operands[2], operands[1])"))] > ) > > (define_insn "stack_tie" > > On Thu, Apr 7, 2011 at 7:30 PM, Ramana Radhakrishnan > <ramana.radhakrishnan@linaro.org> wrote: >> On 07/04/11 12:08, Carrot Wei wrote: >>> >>> On Thu, Apr 7, 2011 at 5:31 PM, Richard Sandiford >>> <richard.sandiford@linaro.org> wrote: >>>> >>>> Hi Carrot, >>>> >>>> Sorry if this has already been reported, but the patch breaks bootstrap >>>> of arm-linux-gnueabi (or cross builds with --enable-werror). The problem >>>> is that this... >>>> >>>> uses a statement expression -- i.e. ({ code; result; }) -- which is >>>> a GNU extension. >>>> >>>> I suppose a simple fix would be to put the code into an arm.c function. >>>> >>> >>> Thank you for the report, I will add this fix to the second part of the >>> patch. >> >> It would be worth unbreaking bootstrap as a separate patch and not >> conflating it with a different set of improvements. >> >> Ramana >> >>> >>> Carrot >> >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-04-08 8:36 ` Ramana Radhakrishnan @ 2011-04-08 8:52 ` Carrot Wei 2011-04-08 9:01 ` Ramana Radhakrishnan 0 siblings, 1 reply; 16+ messages in thread From: Carrot Wei @ 2011-04-08 8:52 UTC (permalink / raw) To: Ramana Radhakrishnan; +Cc: gcc-patches, richard.sandiford Sorry, which pattern in thumb2.md? thanks Carrot On Fri, Apr 8, 2011 at 4:35 PM, Ramana Radhakrishnan <ramana.radhakrishnan@linaro.org> wrote: > On 08/04/11 08:36, Carrot Wei wrote: >> >> Hi >> >> This patch moves the length computation of push_multi into a C >> function to avoid the usage of GNU extension. > > Please put the comment regarding the calculation of the length attribute > along with the pattern in thumb2.md as well. > >> >> Tested on qemu without regression. OK to install? > > Ok with that change. > > Thanks for fixing this up. > > cheers > Ramana >> >> thanks >> Carrot >> >> ChangeLog: >> 2011-04-08 Wei Guozhi<carrot@google.com> >> >> PR target/47855 >> * config/arm/arm-protos.h (arm_attr_length_push_multi): New >> prototype. >> * config/arm/arm.c (arm_attr_length_push_multi): New function. >> * config/arm/arm.md (*push_multi): Change the length computation >> to >> call a C function. >> >> >> Index: arm.c >> =================================================================== >> --- arm.c (revision 172158) >> +++ arm.c (working copy) >> @@ -23696,4 +23696,30 @@ arm_preferred_rename_class (reg_class_t >> return NO_REGS; >> } >> >> +/* Compute the atrribute "length" of insn "*push_multi". >> + So this function MUST be kept in sync with that insn pattern. */ >> +int >> +arm_attr_length_push_multi(rtx parallel_op, rtx first_op) >> +{ >> + int i, regno, hi_reg; >> + int num_saves = XVECLEN (parallel_op, 0); >> + >> + /* ARM mode. */ >> + if (TARGET_ARM) >> + return 4; >> + >> + /* Thumb2 mode. */ >> + regno = REGNO (first_op); >> + hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)&& (regno != LR_REGNUM); >> + for (i = 1; i< num_saves&& !hi_reg; i++) >> + { >> + regno = REGNO (XEXP (XVECEXP (parallel_op, 0, i), 0)); >> + hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)&& (regno != >> LR_REGNUM); >> + } >> + >> + if (!hi_reg) >> + return 2; >> + return 4; >> +} >> + >> #include "gt-arm.h" >> Index: arm-protos.h >> =================================================================== >> --- arm-protos.h (revision 172158) >> +++ arm-protos.h (working copy) >> @@ -152,6 +152,7 @@ extern void arm_expand_sync (enum machin >> extern const char *arm_output_memory_barrier (rtx *); >> extern const char *arm_output_sync_insn (rtx, rtx *); >> extern unsigned int arm_sync_loop_insns (rtx , rtx *); >> +extern int arm_attr_length_push_multi(rtx, rtx); >> >> #if defined TREE_CODE >> extern void arm_init_cumulative_args (CUMULATIVE_ARGS *, tree, rtx, >> tree); >> Index: arm.md >> =================================================================== >> --- arm.md (revision 172158) >> +++ arm.md (working copy) >> @@ -10290,27 +10290,7 @@ >> }" >> [(set_attr "type" "store4") >> (set (attr "length") >> - (if_then_else >> - (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) >> - (ne (symbol_ref "{ >> - /* Check if there are any high register (except lr) >> - references in the list. KEEP the following >> iteration >> - in sync with the template above. */ >> - int i, regno, hi_reg; >> - int num_saves = XVECLEN (operands[2], 0); >> - regno = REGNO (operands[1]); >> - hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS) >> - && (regno != LR_REGNUM); >> - for (i = 1; i< num_saves&& !hi_reg; i++) >> - { >> - regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), >> 0)); >> - hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS) >> - && (regno != LR_REGNUM); >> - } >> - !hi_reg; }") >> - (const_int 0))) >> - (const_int 2) >> - (const_int 4)))] >> + (symbol_ref "arm_attr_length_push_multi (operands[2], >> operands[1])"))] >> ) >> >> (define_insn "stack_tie" >> >> On Thu, Apr 7, 2011 at 7:30 PM, Ramana Radhakrishnan >> <ramana.radhakrishnan@linaro.org> wrote: >>> >>> On 07/04/11 12:08, Carrot Wei wrote: >>>> >>>> On Thu, Apr 7, 2011 at 5:31 PM, Richard Sandiford >>>> <richard.sandiford@linaro.org> wrote: >>>>> >>>>> Hi Carrot, >>>>> >>>>> Sorry if this has already been reported, but the patch breaks bootstrap >>>>> of arm-linux-gnueabi (or cross builds with --enable-werror). The >>>>> problem >>>>> is that this... >>>>> >>>>> uses a statement expression -- i.e. ({ code; result; }) -- which is >>>>> a GNU extension. >>>>> >>>>> I suppose a simple fix would be to put the code into an arm.c function. >>>>> >>>> >>>> Thank you for the report, I will add this fix to the second part of the >>>> patch. >>> >>> It would be worth unbreaking bootstrap as a separate patch and not >>> conflating it with a different set of improvements. >>> >>> Ramana >>> >>>> >>>> Carrot >>> >>> > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns 2011-04-08 8:52 ` Carrot Wei @ 2011-04-08 9:01 ` Ramana Radhakrishnan 0 siblings, 0 replies; 16+ messages in thread From: Ramana Radhakrishnan @ 2011-04-08 9:01 UTC (permalink / raw) To: Carrot Wei; +Cc: gcc-patches, richard.sandiford On 8 April 2011 09:52, Carrot Wei <carrot@google.com> wrote: > Sorry, which pattern in thumb2.md? Sorry, I meant to say push_multi in arm.md . Not enough coffee yet this morning. cheers Ramana > > thanks > Carrot > > On Fri, Apr 8, 2011 at 4:35 PM, Ramana Radhakrishnan > <ramana.radhakrishnan@linaro.org> wrote: >> On 08/04/11 08:36, Carrot Wei wrote: >>> >>> Hi >>> >>> This patch moves the length computation of push_multi into a C >>> function to avoid the usage of GNU extension. >> >> Please put the comment regarding the calculation of the length attribute >> along with the pattern in thumb2.md as well. >> >>> >>> Tested on qemu without regression. OK to install? >> >> Ok with that change. >> >> Thanks for fixing this up. >> >> cheers >> Ramana >>> >>> thanks >>> Carrot >>> >>> ChangeLog: >>> 2011-04-08 Wei Guozhi<carrot@google.com> >>> >>> PR target/47855 >>> * config/arm/arm-protos.h (arm_attr_length_push_multi): New >>> prototype. >>> * config/arm/arm.c (arm_attr_length_push_multi): New function. >>> * config/arm/arm.md (*push_multi): Change the length computation >>> to >>> call a C function. >>> >>> >>> Index: arm.c >>> =================================================================== >>> --- arm.c (revision 172158) >>> +++ arm.c (working copy) >>> @@ -23696,4 +23696,30 @@ arm_preferred_rename_class (reg_class_t >>> return NO_REGS; >>> } >>> >>> +/* Compute the atrribute "length" of insn "*push_multi". >>> + So this function MUST be kept in sync with that insn pattern. */ >>> +int >>> +arm_attr_length_push_multi(rtx parallel_op, rtx first_op) >>> +{ >>> + int i, regno, hi_reg; >>> + int num_saves = XVECLEN (parallel_op, 0); >>> + >>> + /* ARM mode. */ >>> + if (TARGET_ARM) >>> + return 4; >>> + >>> + /* Thumb2 mode. */ >>> + regno = REGNO (first_op); >>> + hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)&& (regno != LR_REGNUM); >>> + for (i = 1; i< num_saves&& !hi_reg; i++) >>> + { >>> + regno = REGNO (XEXP (XVECEXP (parallel_op, 0, i), 0)); >>> + hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)&& (regno != >>> LR_REGNUM); >>> + } >>> + >>> + if (!hi_reg) >>> + return 2; >>> + return 4; >>> +} >>> + >>> #include "gt-arm.h" >>> Index: arm-protos.h >>> =================================================================== >>> --- arm-protos.h (revision 172158) >>> +++ arm-protos.h (working copy) >>> @@ -152,6 +152,7 @@ extern void arm_expand_sync (enum machin >>> extern const char *arm_output_memory_barrier (rtx *); >>> extern const char *arm_output_sync_insn (rtx, rtx *); >>> extern unsigned int arm_sync_loop_insns (rtx , rtx *); >>> +extern int arm_attr_length_push_multi(rtx, rtx); >>> >>> #if defined TREE_CODE >>> extern void arm_init_cumulative_args (CUMULATIVE_ARGS *, tree, rtx, >>> tree); >>> Index: arm.md >>> =================================================================== >>> --- arm.md (revision 172158) >>> +++ arm.md (working copy) >>> @@ -10290,27 +10290,7 @@ >>> }" >>> [(set_attr "type" "store4") >>> (set (attr "length") >>> - (if_then_else >>> - (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) >>> - (ne (symbol_ref "{ >>> - /* Check if there are any high register (except lr) >>> - references in the list. KEEP the following >>> iteration >>> - in sync with the template above. */ >>> - int i, regno, hi_reg; >>> - int num_saves = XVECLEN (operands[2], 0); >>> - regno = REGNO (operands[1]); >>> - hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS) >>> - && (regno != LR_REGNUM); >>> - for (i = 1; i< num_saves&& !hi_reg; i++) >>> - { >>> - regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), >>> 0)); >>> - hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS) >>> - && (regno != LR_REGNUM); >>> - } >>> - !hi_reg; }") >>> - (const_int 0))) >>> - (const_int 2) >>> - (const_int 4)))] >>> + (symbol_ref "arm_attr_length_push_multi (operands[2], >>> operands[1])"))] >>> ) >>> >>> (define_insn "stack_tie" >>> >>> On Thu, Apr 7, 2011 at 7:30 PM, Ramana Radhakrishnan >>> <ramana.radhakrishnan@linaro.org> wrote: >>>> >>>> On 07/04/11 12:08, Carrot Wei wrote: >>>>> >>>>> On Thu, Apr 7, 2011 at 5:31 PM, Richard Sandiford >>>>> <richard.sandiford@linaro.org> wrote: >>>>>> >>>>>> Hi Carrot, >>>>>> >>>>>> Sorry if this has already been reported, but the patch breaks bootstrap >>>>>> of arm-linux-gnueabi (or cross builds with --enable-werror). The >>>>>> problem >>>>>> is that this... >>>>>> >>>>>> uses a statement expression -- i.e. ({ code; result; }) -- which is >>>>>> a GNU extension. >>>>>> >>>>>> I suppose a simple fix would be to put the code into an arm.c function. >>>>>> >>>>> >>>>> Thank you for the report, I will add this fix to the second part of the >>>>> patch. >>>> >>>> It would be worth unbreaking bootstrap as a separate patch and not >>>> conflating it with a different set of improvements. >>>> >>>> Ramana >>>> >>>>> >>>>> Carrot >>>> >>>> >> >> > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2011-04-08 9:01 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-03-26 15:55 [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns Carrot Wei 2011-03-30 1:24 ` Ramana Radhakrishnan 2011-03-30 7:35 ` Chung-Lin Tang 2011-03-30 12:41 ` Richard Earnshaw 2011-03-31 3:34 ` Carrot Wei 2011-03-31 16:04 ` Ramana Radhakrishnan 2011-04-01 6:57 ` Carrot Wei 2011-04-05 13:32 ` Ramana Radhakrishnan 2011-04-06 2:26 ` Carrot Wei 2011-04-07 9:32 ` Richard Sandiford 2011-04-07 11:09 ` Carrot Wei 2011-04-07 11:31 ` Ramana Radhakrishnan 2011-04-08 7:36 ` Carrot Wei 2011-04-08 8:36 ` Ramana Radhakrishnan 2011-04-08 8:52 ` Carrot Wei 2011-04-08 9:01 ` Ramana Radhakrishnan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).