* [PATCH, ARM] PR47855 Compute attr length for thumb2 insns, 3/3 (issue4475042)
@ 2011-05-05 7:31 Guozhi Wei
2011-05-05 9:43 ` Richard Earnshaw
0 siblings, 1 reply; 5+ messages in thread
From: Guozhi Wei @ 2011-05-05 7:31 UTC (permalink / raw)
To: reply, gcc-patches
Hi
This is the third part of the fixing for
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47855
This patch contains the length computation/refinement for insn patterns
"*thumb2_movsi_insn", "*thumb2_cbz" and "*thumb2_cbnz".
At the same time this patch revealed two bugs. The first is the maximum offset
of cbz/cbnz, it should be 126, but it is 128 in patterns "*thumb2_cbz" and
"*thumb2_cbnz". The second is that only 2-register form of shift instructions
can be 16 bit, but 3-register form is allowed in "*thumb2_shiftsi3_short" and
related peephole2. The fix is also contained in this patch.
The patch has been tested on arm qemu.
thanks
Carrot
2011-05-05 Guozhi Wei <carrot@google.com>
PR target/47855
* config/arm/thumb2.md (thumb2_movsi_insn): Add length addtribute.
(thumb2_shiftsi3_short and peephole2): Remove 3-register case.
(thumb2_cbz): Refine length computation.
(thumb2_cbnz): Likewise.
Index: config/arm/thumb2.md
===================================================================
--- config/arm/thumb2.md (revision 173350)
+++ config/arm/thumb2.md (working copy)
@@ -165,25 +165,49 @@
;; regs. The high register alternatives are not taken into account when
;; choosing register preferences in order to reflect their expense.
(define_insn "*thumb2_movsi_insn"
- [(set (match_operand:SI 0 "nonimmediate_operand" "=rk,r,r,r,l ,*hk,m,*m")
- (match_operand:SI 1 "general_operand" "rk ,I,K,j,mi,*mi,l,*hk"))]
+ [(set (match_operand:SI 0 "nonimmediate_operand" "=l,rk,r,r,r,l ,*rk,Uu,*m")
+ (match_operand:SI 1 "general_operand" "l ,rk,I,K,j,Uu,*mi,l ,*rk"))]
"TARGET_THUMB2 && ! TARGET_IWMMXT
&& !(TARGET_HARD_FLOAT && TARGET_VFP)
&& ( register_operand (operands[0], SImode)
|| register_operand (operands[1], SImode))"
- "@
- mov%?\\t%0, %1
- mov%?\\t%0, %1
- mvn%?\\t%0, #%B1
- movw%?\\t%0, %1
- ldr%?\\t%0, %1
- ldr%?\\t%0, %1
- str%?\\t%1, %0
- str%?\\t%1, %0"
- [(set_attr "type" "*,*,*,*,load1,load1,store1,store1")
+ "*
+ switch (which_alternative)
+ {
+ case 0: return \"mov%?\\t%0, %1\";
+ case 1: return \"mov%?\\t%0, %1\";
+ case 2: return \"mov%?\\t%0, %1\";
+ case 3: return \"mvn%?\\t%0, #%B1\";
+ case 4: return \"movw%?\\t%0, %1\";
+
+ case 5:
+ if (GET_CODE (XEXP (operands[1], 0)) == POST_INC)
+ {
+ operands[1] = XEXP (XEXP (operands[1], 0), 0);
+ return \"ldm%(ia%)\t%1!, {%0}\";
+ }
+ else
+ return \"ldr%?\\t%0, %1\";
+
+ case 6: return \"ldr%?\\t%0, %1\";
+
+ case 7:
+ if (GET_CODE (XEXP (operands[0], 0)) == POST_INC)
+ {
+ operands[0] = XEXP (XEXP (operands[0], 0), 0);
+ return \"stm%(ia%)\t%0!, {%1}\";
+ }
+ else
+ return \"str%?\\t%1, %0\";
+
+ case 8: return \"str%?\\t%1, %0\";
+ default: gcc_unreachable ();
+ }"
+ [(set_attr "type" "*,*,*,*,*,load1,load1,store1,store1")
(set_attr "predicable" "yes")
- (set_attr "pool_range" "*,*,*,*,1020,4096,*,*")
- (set_attr "neg_pool_range" "*,*,*,*,0,0,*,*")]
+ (set_attr "length" "2,4,4,4,4,2,4,2,4")
+ (set_attr "pool_range" "*,*,*,*,*,1020,4096,*,*")
+ (set_attr "neg_pool_range" "*,*,*,*,*,0,0,*,*")]
)
(define_insn "tls_load_dot_plus_four"
@@ -685,7 +709,8 @@
"TARGET_THUMB2
&& peep2_regno_dead_p(0, CC_REGNUM)
&& ((GET_CODE(operands[3]) != ROTATE && GET_CODE(operands[3]) != ROTATERT)
- || REG_P(operands[2]))"
+ || REG_P(operands[2]))
+ && (CONSTANT_P (operands[2]) || (operands[0] == operands[1]))"
[(parallel
[(set (match_dup 0)
(match_op_dup 3
@@ -696,10 +721,10 @@
)
(define_insn "*thumb2_shiftsi3_short"
- [(set (match_operand:SI 0 "low_register_operand" "=l")
+ [(set (match_operand:SI 0 "low_register_operand" "=l,l")
(match_operator:SI 3 "shift_operator"
- [(match_operand:SI 1 "low_register_operand" "l")
- (match_operand:SI 2 "low_reg_or_int_operand" "lM")]))
+ [(match_operand:SI 1 "low_register_operand" "0,l")
+ (match_operand:SI 2 "low_reg_or_int_operand" "l,M")]))
(clobber (reg:CC CC_REGNUM))]
"TARGET_THUMB2 && reload_completed
&& ((GET_CODE(operands[3]) != ROTATE && GET_CODE(operands[3]) != ROTATERT)
@@ -707,7 +732,7 @@
"* return arm_output_shift(operands, 2);"
[(set_attr "predicable" "yes")
(set_attr "shift" "1")
- (set_attr "length" "2")
+ (set_attr "length" "2,2")
(set (attr "type") (if_then_else (match_operand 2 "const_int_operand" "")
(const_string "alu_shift")
(const_string "alu_shift_reg")))]
@@ -965,13 +990,23 @@
else
return \"cmp\\t%0, #0\;beq\\t%l1\";
"
- [(set (attr "length")
- (if_then_else
- (and (ge (minus (match_dup 1) (pc)) (const_int 2))
- (le (minus (match_dup 1) (pc)) (const_int 128))
- (eq (symbol_ref ("which_alternative")) (const_int 0)))
- (const_int 2)
- (const_int 8)))]
+ [(set (attr "length")
+ (if_then_else
+ (eq (symbol_ref ("which_alternative")) (const_int 0))
+ (if_then_else
+ (and (ge (minus (match_dup 1) (pc)) (const_int 2))
+ (le (minus (match_dup 1) (pc)) (const_int 126)))
+ (const_int 2)
+ (if_then_else
+ (and (ge (minus (match_dup 1) (pc)) (const_int -256))
+ (le (minus (match_dup 1) (pc)) (const_int 254)))
+ (const_int 4)
+ (const_int 6)))
+ (if_then_else
+ (and (ge (minus (match_dup 1) (pc)) (const_int -256))
+ (le (minus (match_dup 1) (pc)) (const_int 254)))
+ (const_int 6)
+ (const_int 8))))]
)
(define_insn "*thumb2_cbnz"
@@ -988,13 +1023,23 @@
else
return \"cmp\\t%0, #0\;bne\\t%l1\";
"
- [(set (attr "length")
- (if_then_else
- (and (ge (minus (match_dup 1) (pc)) (const_int 2))
- (le (minus (match_dup 1) (pc)) (const_int 128))
- (eq (symbol_ref ("which_alternative")) (const_int 0)))
- (const_int 2)
- (const_int 8)))]
+ [(set (attr "length")
+ (if_then_else
+ (eq (symbol_ref ("which_alternative")) (const_int 0))
+ (if_then_else
+ (and (ge (minus (match_dup 1) (pc)) (const_int 2))
+ (le (minus (match_dup 1) (pc)) (const_int 126)))
+ (const_int 2)
+ (if_then_else
+ (and (ge (minus (match_dup 1) (pc)) (const_int -256))
+ (le (minus (match_dup 1) (pc)) (const_int 254)))
+ (const_int 4)
+ (const_int 6)))
+ (if_then_else
+ (and (ge (minus (match_dup 1) (pc)) (const_int -256))
+ (le (minus (match_dup 1) (pc)) (const_int 254)))
+ (const_int 6)
+ (const_int 8))))]
)
;; 16-bit complement
--
This patch is available for review at http://codereview.appspot.com/4475042
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH, ARM] PR47855 Compute attr length for thumb2 insns, 3/3 (issue4475042)
2011-05-05 7:31 [PATCH, ARM] PR47855 Compute attr length for thumb2 insns, 3/3 (issue4475042) Guozhi Wei
@ 2011-05-05 9:43 ` Richard Earnshaw
2011-05-06 9:27 ` Carrot Wei
0 siblings, 1 reply; 5+ messages in thread
From: Richard Earnshaw @ 2011-05-05 9:43 UTC (permalink / raw)
To: Guozhi Wei; +Cc: reply, gcc-patches
On Thu, 2011-05-05 at 14:51 +0800, Guozhi Wei wrote:
> Hi
>
> This is the third part of the fixing for
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47855
>
> This patch contains the length computation/refinement for insn patterns
> "*thumb2_movsi_insn", "*thumb2_cbz" and "*thumb2_cbnz".
>
> At the same time this patch revealed two bugs. The first is the maximum offset
> of cbz/cbnz, it should be 126, but it is 128 in patterns "*thumb2_cbz" and
> "*thumb2_cbnz". The second is that only 2-register form of shift instructions
> can be 16 bit, but 3-register form is allowed in "*thumb2_shiftsi3_short" and
> related peephole2. The fix is also contained in this patch.
>
> The patch has been tested on arm qemu.
>
> thanks
> Carrot
>
>
> 2011-05-05 Guozhi Wei <carrot@google.com>
>
> PR target/47855
> * config/arm/thumb2.md (thumb2_movsi_insn): Add length addtribute.
> (thumb2_shiftsi3_short and peephole2): Remove 3-register case.
> (thumb2_cbz): Refine length computation.
> (thumb2_cbnz): Likewise.
>
Hmm, although these changes are all related to length calculations, they
are really three patches that are unrelated to each other. It would be
easier to review this if they were kept separate.
1) thumb2_shiftsi3_short
This appears to be a straight bug. We are putting out a 32-bit
instruction when we are claiming it to be only 16 bits. This is OK.
2) thumb2_movsi_insn
There are two things here.
a) Thumb2 has a 16-bit move instruction for all core
register-to-register transfers, so the separation of alternatives 1 and
2 is unnecessary -- just code these as "rk".
b) The ldm form does not support unaligned memory accesses. I'm aware
that work is being done to add unaligned support to GCC for ARM, so I
need to find out whether this patch will interfere with those changes.
I'll try to find out what the situation is here and get back to you.
3) thumb2_cbz and thumb2_cbnz
The range calculations look wrong here. Remember that the 'pc' as far
as GCC is concerned is the address of the start of the insn. So for a
backwards branch you need to account for all the bytes in the insn
pattern that occur before the branch instruction itself, and secondly
you also have to remember that the 'pc' that the CPU uses is the address
of the branch instruction plus 4. All these conspire to reduce the
backwards range of a short branch to several bytes less than the 256
that you currently have coded.
R.
> Index: config/arm/thumb2.md
> ===================================================================
> --- config/arm/thumb2.md (revision 173350)
> +++ config/arm/thumb2.md (working copy)
> @@ -165,25 +165,49 @@
> ;; regs. The high register alternatives are not taken into account when
> ;; choosing register preferences in order to reflect their expense.
> (define_insn "*thumb2_movsi_insn"
> - [(set (match_operand:SI 0 "nonimmediate_operand" "=rk,r,r,r,l ,*hk,m,*m")
> - (match_operand:SI 1 "general_operand" "rk ,I,K,j,mi,*mi,l,*hk"))]
> + [(set (match_operand:SI 0 "nonimmediate_operand" "=l,rk,r,r,r,l ,*rk,Uu,*m")
> + (match_operand:SI 1 "general_operand" "l ,rk,I,K,j,Uu,*mi,l ,*rk"))]
> "TARGET_THUMB2 && ! TARGET_IWMMXT
> && !(TARGET_HARD_FLOAT && TARGET_VFP)
> && ( register_operand (operands[0], SImode)
> || register_operand (operands[1], SImode))"
> - "@
> - mov%?\\t%0, %1
> - mov%?\\t%0, %1
> - mvn%?\\t%0, #%B1
> - movw%?\\t%0, %1
> - ldr%?\\t%0, %1
> - ldr%?\\t%0, %1
> - str%?\\t%1, %0
> - str%?\\t%1, %0"
> - [(set_attr "type" "*,*,*,*,load1,load1,store1,store1")
> + "*
> + switch (which_alternative)
> + {
> + case 0: return \"mov%?\\t%0, %1\";
> + case 1: return \"mov%?\\t%0, %1\";
> + case 2: return \"mov%?\\t%0, %1\";
> + case 3: return \"mvn%?\\t%0, #%B1\";
> + case 4: return \"movw%?\\t%0, %1\";
> +
> + case 5:
> + if (GET_CODE (XEXP (operands[1], 0)) == POST_INC)
> + {
> + operands[1] = XEXP (XEXP (operands[1], 0), 0);
> + return \"ldm%(ia%)\t%1!, {%0}\";
> + }
> + else
> + return \"ldr%?\\t%0, %1\";
> +
> + case 6: return \"ldr%?\\t%0, %1\";
> +
> + case 7:
> + if (GET_CODE (XEXP (operands[0], 0)) == POST_INC)
> + {
> + operands[0] = XEXP (XEXP (operands[0], 0), 0);
> + return \"stm%(ia%)\t%0!, {%1}\";
> + }
> + else
> + return \"str%?\\t%1, %0\";
> +
> + case 8: return \"str%?\\t%1, %0\";
> + default: gcc_unreachable ();
> + }"
> + [(set_attr "type" "*,*,*,*,*,load1,load1,store1,store1")
> (set_attr "predicable" "yes")
> - (set_attr "pool_range" "*,*,*,*,1020,4096,*,*")
> - (set_attr "neg_pool_range" "*,*,*,*,0,0,*,*")]
> + (set_attr "length" "2,4,4,4,4,2,4,2,4")
> + (set_attr "pool_range" "*,*,*,*,*,1020,4096,*,*")
> + (set_attr "neg_pool_range" "*,*,*,*,*,0,0,*,*")]
> )
>
> (define_insn "tls_load_dot_plus_four"
> @@ -685,7 +709,8 @@
> "TARGET_THUMB2
> && peep2_regno_dead_p(0, CC_REGNUM)
> && ((GET_CODE(operands[3]) != ROTATE && GET_CODE(operands[3]) != ROTATERT)
> - || REG_P(operands[2]))"
> + || REG_P(operands[2]))
> + && (CONSTANT_P (operands[2]) || (operands[0] == operands[1]))"
> [(parallel
> [(set (match_dup 0)
> (match_op_dup 3
> @@ -696,10 +721,10 @@
> )
>
> (define_insn "*thumb2_shiftsi3_short"
> - [(set (match_operand:SI 0 "low_register_operand" "=l")
> + [(set (match_operand:SI 0 "low_register_operand" "=l,l")
> (match_operator:SI 3 "shift_operator"
> - [(match_operand:SI 1 "low_register_operand" "l")
> - (match_operand:SI 2 "low_reg_or_int_operand" "lM")]))
> + [(match_operand:SI 1 "low_register_operand" "0,l")
> + (match_operand:SI 2 "low_reg_or_int_operand" "l,M")]))
> (clobber (reg:CC CC_REGNUM))]
> "TARGET_THUMB2 && reload_completed
> && ((GET_CODE(operands[3]) != ROTATE && GET_CODE(operands[3]) != ROTATERT)
> @@ -707,7 +732,7 @@
> "* return arm_output_shift(operands, 2);"
> [(set_attr "predicable" "yes")
> (set_attr "shift" "1")
> - (set_attr "length" "2")
> + (set_attr "length" "2,2")
> (set (attr "type") (if_then_else (match_operand 2 "const_int_operand" "")
> (const_string "alu_shift")
> (const_string "alu_shift_reg")))]
> @@ -965,13 +990,23 @@
> else
> return \"cmp\\t%0, #0\;beq\\t%l1\";
> "
> - [(set (attr "length")
> - (if_then_else
> - (and (ge (minus (match_dup 1) (pc)) (const_int 2))
> - (le (minus (match_dup 1) (pc)) (const_int 128))
> - (eq (symbol_ref ("which_alternative")) (const_int 0)))
> - (const_int 2)
> - (const_int 8)))]
> + [(set (attr "length")
> + (if_then_else
> + (eq (symbol_ref ("which_alternative")) (const_int 0))
> + (if_then_else
> + (and (ge (minus (match_dup 1) (pc)) (const_int 2))
> + (le (minus (match_dup 1) (pc)) (const_int 126)))
> + (const_int 2)
> + (if_then_else
> + (and (ge (minus (match_dup 1) (pc)) (const_int -256))
> + (le (minus (match_dup 1) (pc)) (const_int 254)))
> + (const_int 4)
> + (const_int 6)))
> + (if_then_else
> + (and (ge (minus (match_dup 1) (pc)) (const_int -256))
> + (le (minus (match_dup 1) (pc)) (const_int 254)))
> + (const_int 6)
> + (const_int 8))))]
> )
>
> (define_insn "*thumb2_cbnz"
> @@ -988,13 +1023,23 @@
> else
> return \"cmp\\t%0, #0\;bne\\t%l1\";
> "
> - [(set (attr "length")
> - (if_then_else
> - (and (ge (minus (match_dup 1) (pc)) (const_int 2))
> - (le (minus (match_dup 1) (pc)) (const_int 128))
> - (eq (symbol_ref ("which_alternative")) (const_int 0)))
> - (const_int 2)
> - (const_int 8)))]
> + [(set (attr "length")
> + (if_then_else
> + (eq (symbol_ref ("which_alternative")) (const_int 0))
> + (if_then_else
> + (and (ge (minus (match_dup 1) (pc)) (const_int 2))
> + (le (minus (match_dup 1) (pc)) (const_int 126)))
> + (const_int 2)
> + (if_then_else
> + (and (ge (minus (match_dup 1) (pc)) (const_int -256))
> + (le (minus (match_dup 1) (pc)) (const_int 254)))
> + (const_int 4)
> + (const_int 6)))
> + (if_then_else
> + (and (ge (minus (match_dup 1) (pc)) (const_int -256))
> + (le (minus (match_dup 1) (pc)) (const_int 254)))
> + (const_int 6)
> + (const_int 8))))]
> )
>
> ;; 16-bit complement
>
> --
> This patch is available for review at http://codereview.appspot.com/4475042
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH, ARM] PR47855 Compute attr length for thumb2 insns, 3/3 (issue4475042)
2011-05-05 9:43 ` Richard Earnshaw
@ 2011-05-06 9:27 ` Carrot Wei
0 siblings, 0 replies; 5+ messages in thread
From: Carrot Wei @ 2011-05-06 9:27 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: reply, gcc-patches
On Thu, May 5, 2011 at 5:42 PM, Richard Earnshaw <rearnsha@arm.com> wrote:
>
> On Thu, 2011-05-05 at 14:51 +0800, Guozhi Wei wrote:
> > Hi
> >
> > This is the third part of the fixing for
> >
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47855
> >
> > This patch contains the length computation/refinement for insn patterns
> > "*thumb2_movsi_insn", "*thumb2_cbz" and "*thumb2_cbnz".
> >
> > At the same time this patch revealed two bugs. The first is the maximum offset
> > of cbz/cbnz, it should be 126, but it is 128 in patterns "*thumb2_cbz" and
> > "*thumb2_cbnz". The second is that only 2-register form of shift instructions
> > can be 16 bit, but 3-register form is allowed in "*thumb2_shiftsi3_short" and
> > related peephole2. The fix is also contained in this patch.
> >
> > The patch has been tested on arm qemu.
> >
> > thanks
> > Carrot
> >
> >
> > 2011-05-05 Guozhi Wei <carrot@google.com>
> >
> > PR target/47855
> > * config/arm/thumb2.md (thumb2_movsi_insn): Add length addtribute.
> > (thumb2_shiftsi3_short and peephole2): Remove 3-register case.
> > (thumb2_cbz): Refine length computation.
> > (thumb2_cbnz): Likewise.
> >
>
> Hmm, although these changes are all related to length calculations, they
> are really three patches that are unrelated to each other. It would be
> easier to review this if they were kept separate.
>
> 1) thumb2_shiftsi3_short
> This appears to be a straight bug. We are putting out a 32-bit
> instruction when we are claiming it to be only 16 bits. This is OK.
>
> 2) thumb2_movsi_insn
> There are two things here.
> a) Thumb2 has a 16-bit move instruction for all core
> register-to-register transfers, so the separation of alternatives 1 and
> 2 is unnecessary -- just code these as "rk".
done.
>
> b) The ldm form does not support unaligned memory accesses. I'm aware
> that work is being done to add unaligned support to GCC for ARM, so I
> need to find out whether this patch will interfere with those changes.
> I'll try to find out what the situation is here and get back to you.
>
> 3) thumb2_cbz and thumb2_cbnz
> The range calculations look wrong here. Remember that the 'pc' as far
> as GCC is concerned is the address of the start of the insn. So for a
> backwards branch you need to account for all the bytes in the insn
> pattern that occur before the branch instruction itself, and secondly
> you also have to remember that the 'pc' that the CPU uses is the address
> of the branch instruction plus 4. All these conspire to reduce the
> backwards range of a short branch to several bytes less than the 256
> that you currently have coded.
The usage of 'pc' is more complex than I thought. I understood it after
reading the comment in file arm.md. And the description at
http://gcc.gnu.org/onlinedocs/gccint/Insn-Lengths.html#Insn-Lengths is not
right for forward branch cases. Now the ranges are modified accordingly.
It has been tested on arm qemu in thumb2 mode.
thanks
Carrot
2011-05-06 Guozhi Wei <carrot@google.com>
PR target/47855
* config/arm/thumb2.md (thumb2_movsi_insn): Add length addtribute.
(thumb2_shiftsi3_short and peephole2): Remove 3-register case.
(thumb2_cbz): Refine length computation.
(thumb2_cbnz): Likewise.
Index: config/arm/thumb2.md
===================================================================
--- config/arm/thumb2.md (revision 173350)
+++ config/arm/thumb2.md (working copy)
@@ -165,23 +165,46 @@
;; regs. The high register alternatives are not taken into account when
;; choosing register preferences in order to reflect their expense.
(define_insn "*thumb2_movsi_insn"
- [(set (match_operand:SI 0 "nonimmediate_operand" "=rk,r,r,r,l ,*hk,m,*m")
- (match_operand:SI 1 "general_operand" "rk ,I,K,j,mi,*mi,l,*hk"))]
+ [(set (match_operand:SI 0 "nonimmediate_operand" "=rk,r,r,r,l ,*rk,Uu,*m")
+ (match_operand:SI 1 "general_operand" "rk ,I,K,j,Uu,*mi,l ,*rk"))]
"TARGET_THUMB2 && ! TARGET_IWMMXT
&& !(TARGET_HARD_FLOAT && TARGET_VFP)
&& ( register_operand (operands[0], SImode)
|| register_operand (operands[1], SImode))"
- "@
- mov%?\\t%0, %1
- mov%?\\t%0, %1
- mvn%?\\t%0, #%B1
- movw%?\\t%0, %1
- ldr%?\\t%0, %1
- ldr%?\\t%0, %1
- str%?\\t%1, %0
- str%?\\t%1, %0"
+ "*
+ switch (which_alternative)
+ {
+ case 0: return \"mov%?\\t%0, %1\";
+ case 1: return \"mov%?\\t%0, %1\";
+ case 2: return \"mvn%?\\t%0, #%B1\";
+ case 3: return \"movw%?\\t%0, %1\";
+
+ case 4:
+ if (GET_CODE (XEXP (operands[1], 0)) == POST_INC)
+ {
+ operands[1] = XEXP (XEXP (operands[1], 0), 0);
+ return \"ldm%(ia%)\t%1!, {%0}\";
+ }
+ else
+ return \"ldr%?\\t%0, %1\";
+
+ case 5: return \"ldr%?\\t%0, %1\";
+
+ case 6:
+ if (GET_CODE (XEXP (operands[0], 0)) == POST_INC)
+ {
+ operands[0] = XEXP (XEXP (operands[0], 0), 0);
+ return \"stm%(ia%)\t%0!, {%1}\";
+ }
+ else
+ return \"str%?\\t%1, %0\";
+
+ case 7: return \"str%?\\t%1, %0\";
+ default: gcc_unreachable ();
+ }"
[(set_attr "type" "*,*,*,*,load1,load1,store1,store1")
(set_attr "predicable" "yes")
+ (set_attr "length" "2,4,4,4,2,4,2,4")
(set_attr "pool_range" "*,*,*,*,1020,4096,*,*")
(set_attr "neg_pool_range" "*,*,*,*,0,0,*,*")]
)
@@ -685,7 +708,8 @@
"TARGET_THUMB2
&& peep2_regno_dead_p(0, CC_REGNUM)
&& ((GET_CODE(operands[3]) != ROTATE && GET_CODE(operands[3]) != ROTATERT)
- || REG_P(operands[2]))"
+ || REG_P(operands[2]))
+ && (CONSTANT_P (operands[2]) || (operands[0] == operands[1]))"
[(parallel
[(set (match_dup 0)
(match_op_dup 3
@@ -696,10 +720,10 @@
)
(define_insn "*thumb2_shiftsi3_short"
- [(set (match_operand:SI 0 "low_register_operand" "=l")
+ [(set (match_operand:SI 0 "low_register_operand" "=l,l")
(match_operator:SI 3 "shift_operator"
- [(match_operand:SI 1 "low_register_operand" "l")
- (match_operand:SI 2 "low_reg_or_int_operand" "lM")]))
+ [(match_operand:SI 1 "low_register_operand" "0,l")
+ (match_operand:SI 2 "low_reg_or_int_operand" "l,M")]))
(clobber (reg:CC CC_REGNUM))]
"TARGET_THUMB2 && reload_completed
&& ((GET_CODE(operands[3]) != ROTATE && GET_CODE(operands[3]) != ROTATERT)
@@ -707,7 +731,7 @@
"* return arm_output_shift(operands, 2);"
[(set_attr "predicable" "yes")
(set_attr "shift" "1")
- (set_attr "length" "2")
+ (set_attr "length" "2,2")
(set (attr "type") (if_then_else (match_operand 2 "const_int_operand" "")
(const_string "alu_shift")
(const_string "alu_shift_reg")))]
@@ -965,13 +989,23 @@
else
return \"cmp\\t%0, #0\;beq\\t%l1\";
"
- [(set (attr "length")
- (if_then_else
- (and (ge (minus (match_dup 1) (pc)) (const_int 2))
- (le (minus (match_dup 1) (pc)) (const_int 128))
- (eq (symbol_ref ("which_alternative")) (const_int 0)))
- (const_int 2)
- (const_int 8)))]
+ [(set (attr "length")
+ (if_then_else
+ (eq (symbol_ref ("which_alternative")) (const_int 0))
+ (if_then_else
+ (and (ge (minus (match_dup 1) (pc)) (const_int 2))
+ (le (minus (match_dup 1) (pc)) (const_int 128)))
+ (const_int 2)
+ (if_then_else
+ (and (ge (minus (match_dup 1) (pc)) (const_int -250))
+ (le (minus (match_dup 1) (pc)) (const_int 256)))
+ (const_int 4)
+ (const_int 6)))
+ (if_then_else
+ (and (ge (minus (match_dup 1) (pc)) (const_int -248))
+ (le (minus (match_dup 1) (pc)) (const_int 256)))
+ (const_int 6)
+ (const_int 8))))]
)
(define_insn "*thumb2_cbnz"
@@ -988,13 +1022,23 @@
else
return \"cmp\\t%0, #0\;bne\\t%l1\";
"
- [(set (attr "length")
- (if_then_else
- (and (ge (minus (match_dup 1) (pc)) (const_int 2))
- (le (minus (match_dup 1) (pc)) (const_int 128))
- (eq (symbol_ref ("which_alternative")) (const_int 0)))
- (const_int 2)
- (const_int 8)))]
+ [(set (attr "length")
+ (if_then_else
+ (eq (symbol_ref ("which_alternative")) (const_int 0))
+ (if_then_else
+ (and (ge (minus (match_dup 1) (pc)) (const_int 2))
+ (le (minus (match_dup 1) (pc)) (const_int 128)))
+ (const_int 2)
+ (if_then_else
+ (and (ge (minus (match_dup 1) (pc)) (const_int -250))
+ (le (minus (match_dup 1) (pc)) (const_int 256)))
+ (const_int 4)
+ (const_int 6)))
+ (if_then_else
+ (and (ge (minus (match_dup 1) (pc)) (const_int -248))
+ (le (minus (match_dup 1) (pc)) (const_int 256)))
+ (const_int 6)
+ (const_int 8))))]
)
;; 16-bit complement
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, ARM] PR47855 Compute attr length for thumb2 insns, 3/3 (issue4475042)
@ 2011-06-07 10:41 Nick Clifton
2011-07-08 3:20 ` Carrot Wei
0 siblings, 1 reply; 5+ messages in thread
From: Nick Clifton @ 2011-06-07 10:41 UTC (permalink / raw)
To: Carrot Wei, Richard Earnshaw; +Cc: GCC Patches
Hi Carrot,
> 2011-05-06 Guozhi Wei <carrot@google.com>
>
> PR target/47855
> * config/arm/thumb2.md (thumb2_movsi_insn): Add length addtribute.
> (thumb2_shiftsi3_short and peephole2): Remove 3-register case.
> (thumb2_cbz): Refine length computation.
> (thumb2_cbnz): Likewise.
Not approved - yet.
The problem is the change to thumb2_movsi_insn. You are still adding in
the support for the STM instruction despite the fact that Richard is
still researching how this will work with unaligned addresses. Given
the fact that this change is not mentioned in the ChangeLog entry, I
will assume that you intended to remove it and just forgot.
I have no issues with the rest of your patch, so if you could submit an
updated patch I will be happy to review it again.
One small point - when/if you do resubmit the STM part of the patch, you
could make the code slightly cleaner by enclosing it in curly
parentheses, thus avoiding the need to escape the double quote marks. Ie:
+ {
+ switch (which_alternative)
+ {
+ case 0:
+ case 1:
+ return "mov%?\t%0, %1";
+
+ case 2:
+ return "mvn%?\t%0, #%B1";
+
+ case 3:
+ return "movw%?\t%0, %1";
+
+ case 4:
+ if (GET_CODE (XEXP (operands[1], 0)) == POST_INC)
+ {
+ operands[1] = XEXP (XEXP (operands[1], 0), 0);
+ return "ldm%(ia%)\t%1!, {%0}";
+ }
+ /* Fall through. */
+ case 5:
+ return "ldr%?\t%0, %1";
+
+ case 6:
+ if (GET_CODE (XEXP (operands[0], 0)) == POST_INC)
+ {
+ operands[0] = XEXP (XEXP (operands[0], 0), 0);
+ return "stm%(ia%)\t%0!, {%1}";
+ }
+ /* Fall through. */
+ case 7:
+ return "str%?\t%1, %0";
+
+ default:
+ gcc_unreachable ();
+ }
+ }
Cheers
Nick
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH, ARM] PR47855 Compute attr length for thumb2 insns, 3/3 (issue4475042)
2011-06-07 10:41 Nick Clifton
@ 2011-07-08 3:20 ` Carrot Wei
0 siblings, 0 replies; 5+ messages in thread
From: Carrot Wei @ 2011-07-08 3:20 UTC (permalink / raw)
To: Nick Clifton, Richard Earnshaw; +Cc: GCC Patches
Thanks for the review.
Richard, what's the situation of unaligned memory access and how does
it conflict with this patch?
thanks
Carrot
On Tue, Jun 7, 2011 at 6:42 PM, Nick Clifton <nickc@redhat.com> wrote:
> Hi Carrot,
>
>> 2011-05-06 Guozhi Wei <carrot@google.com>
>>
>> PR target/47855
>> * config/arm/thumb2.md (thumb2_movsi_insn): Add length addtribute.
>> (thumb2_shiftsi3_short and peephole2): Remove 3-register case.
>> (thumb2_cbz): Refine length computation.
>> (thumb2_cbnz): Likewise.
>
> Not approved - yet.
>
> The problem is the change to thumb2_movsi_insn. You are still adding in the
> support for the STM instruction despite the fact that Richard is still
> researching how this will work with unaligned addresses. Given the fact
> that this change is not mentioned in the ChangeLog entry, I will assume that
> you intended to remove it and just forgot.
>
> I have no issues with the rest of your patch, so if you could submit an
> updated patch I will be happy to review it again.
>
> One small point - when/if you do resubmit the STM part of the patch, you
> could make the code slightly cleaner by enclosing it in curly parentheses,
> thus avoiding the need to escape the double quote marks. Ie:
>
> + {
> + switch (which_alternative)
> + {
> + case 0:
> + case 1:
> + return "mov%?\t%0, %1";
> +
> + case 2:
> + return "mvn%?\t%0, #%B1";
> +
> + case 3:
> + return "movw%?\t%0, %1";
> +
> + case 4:
> + if (GET_CODE (XEXP (operands[1], 0)) == POST_INC)
> + {
> + operands[1] = XEXP (XEXP (operands[1], 0), 0);
> + return "ldm%(ia%)\t%1!, {%0}";
> + }
> + /* Fall through. */
> + case 5:
> + return "ldr%?\t%0, %1";
> +
> + case 6:
> + if (GET_CODE (XEXP (operands[0], 0)) == POST_INC)
> + {
> + operands[0] = XEXP (XEXP (operands[0], 0), 0);
> + return "stm%(ia%)\t%0!, {%1}";
> + }
> + /* Fall through. */
> + case 7:
> + return "str%?\t%1, %0";
> +
> + default:
> + gcc_unreachable ();
> + }
> + }
>
> Cheers
> Nick
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-07-08 2:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-05 7:31 [PATCH, ARM] PR47855 Compute attr length for thumb2 insns, 3/3 (issue4475042) Guozhi Wei
2011-05-05 9:43 ` Richard Earnshaw
2011-05-06 9:27 ` Carrot Wei
2011-06-07 10:41 Nick Clifton
2011-07-08 3:20 ` Carrot Wei
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).