From: Richard Earnshaw <rearnsha@arm.com>
To: Guozhi Wei <carrot@google.com>
Cc: reply@codereview.appspotmail.com, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH, ARM] PR47855 Compute attr length for thumb2 insns, 3/3 (issue4475042)
Date: Thu, 05 May 2011 09:43:00 -0000 [thread overview]
Message-ID: <1304588525.19537.67.camel@e102346-lin.cambridge.arm.com> (raw)
In-Reply-To: <20110505065105.BBA6820A24@guozhiwei.sha.corp.google.com>
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
next prev parent reply other threads:[~2011-05-05 9:42 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-05-05 7:31 Guozhi Wei
2011-05-05 9:43 ` Richard Earnshaw [this message]
2011-05-06 9:27 ` Carrot Wei
2011-06-07 10:41 Nick Clifton
2011-07-08 3:20 ` Carrot Wei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1304588525.19537.67.camel@e102346-lin.cambridge.arm.com \
--to=rearnsha@arm.com \
--cc=carrot@google.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=reply@codereview.appspotmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).