* [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).