From: Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>
To: Jiong Wang <jiong.wang@foss.arm.com>,
GCC Patches <gcc-patches@gcc.gnu.org>
Cc: Wilco Dijkstra <Wilco.Dijkstra@arm.com>,
Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
Subject: Re: [Patch, ARM] PR71061, length pop* pattern in epilogue correctly
Date: Tue, 31 May 2016 15:06:00 -0000 [thread overview]
Message-ID: <574D9C6F.4020306@foss.arm.com> (raw)
In-Reply-To: <573D7707.5090608@foss.arm.com>
Hi Jiong,
On 19/05/16 09:19, Jiong Wang wrote:
>
>
> On 13/05/16 14:54, Jiong Wang wrote:
>> For thumb mode, this is causing wrong size calculation and may affect
>> some rtl pass, for example bb-order where copy_bb_p needs accurate insn
>> length info.
>>
>> This have eventually part of the reason for
>> https://gcc.gnu.org/ml/gcc-patches/2016-05/msg00639.html where bb-order
>> failed to do the bb copy.
>>
>> For the fix, I think we should extend arm_attr_length_push_multi to pop*
>> pattern.
>>
>> OK for trunk?
>>
>> 2016-05-13 Jiong. Wang <jiong.wang@arm.com>
>>
>> gcc/
>> PR target/71061
>> * config/arm/arm-protos.h (arm_attr_length_push_multi): Rename to
>> "arm_attr_length_pp_multi". Add one parameter "first_index".
>> * config/arm/arm.md (*push_multi): Use new function.
>> (*load_multiple_with_writeback): Set "length" attribute.
>> (*pop_multiple_with_writeback_and_return): Likewise.
>> (*pop_multiple_with_return): Likewise.
>>
>
> Wilco pointed out offline the new length function is wrong as for pop we
> should check PC_REG instead of LR_REG, meanwhile these pop patterns may
> allow ldm thus base register needs to be checked also.
>
> I updated this patch to address these issues.
>
> OK for trunk ?
>
I agree with the idea of the patch, but I have some comments inline.
> gcc/
>
> 2016-05-19 Jiong Wang <jiong.wang@arm.com>
>
> PR target/71061
> * config/arm/arm-protos.h (arm_attr_length_pop_multi): New declaration.
> * config/arm/arm.c (arm_attr_length_pop_multi): New function to return
> length for pop patterns.
> * config/arm/arm.md (*load_multiple_with_writeback): Set "length" attribute.
> (*pop_multiple_with_writeback_and_return): Likewise.
> (*pop_multiple_with_return): Likewise.
>
>
diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
index d8179c4..5dd3e0d 100644
--- a/gcc/config/arm/arm-protos.h
+++ b/gcc/config/arm/arm-protos.h
@@ -163,6 +163,7 @@ extern const char *arm_output_iwmmxt_shift_immediate (const char *, rtx *, bool)
extern const char *arm_output_iwmmxt_tinsr (rtx *);
extern unsigned int arm_sync_loop_insns (rtx , rtx *);
extern int arm_attr_length_push_multi(rtx, rtx);
+extern int arm_attr_length_pop_multi(rtx *, bool, bool);
extern void arm_expand_compare_and_swap (rtx op[]);
extern void arm_split_compare_and_swap (rtx op[]);
extern void arm_split_atomic_op (enum rtx_code, rtx, rtx, rtx, rtx, rtx, rtx);
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index c3f74dc..4d19d3a 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -27812,6 +27812,65 @@ arm_attr_length_push_multi(rtx parallel_op, rtx first_op)
return 4;
}
+/* Compute the atrribute "length" of insn. Currently, this function is used
+ for "*load_multiple_with_writeback", "*pop_multiple_with_return" and
+ "*pop_multiple_with_writeback_and_return". */
+
s/atrribute/attribute/
Also, please add a description of the function arguments to the function comment.
+int
+arm_attr_length_pop_multi(rtx *operands, bool return_pc, bool write_back_p)
+{
space between function name and '('.
+ /* ARM mode. */
+ if (TARGET_ARM)
+ return 4;
+ /* Thumb1 mode. */
+ if (TARGET_THUMB1)
+ return 2;
+
+ /* For POP/PUSH/LDM/STM under Thumb2 mode, we can use 16-bit Thumb encodings
+ if the register list is 8-bit. Normally this means all registers in the
+ list must be LO_REGS, that is (R0 -R7). If any HI_REGS used, then we must
+ use 32-bit encodings. But there are two exceptions to this rule where
+ special HI_REGS used in PUSH/POP.
+
+ 1. 16-bit Thumb encoding POP can use an 8-bit register list, and an
+ additional bit, P, that corresponds to the PC. This means it can access
+ any of {PC, R7-R0}.
It took me a bit to figure it out but this bit 'P' you're talking about is a just a bit
in the illustration in the ARM Architecture Reference Manual (section A8.8.131).
I don't think it's useful to refer to it.
This would be better phrased as "The encoding is 16 bits wide if the register list contains
only the registers in {R0 - R7} or the PC".
+ 2. 16-bit Thumb encoding PUSH can use an 8-bit register list, and an
+ additional bit, M , that corresponds to the LR. This means it can
+ access any of {LR, R7-R0}. */
+
This function only deals with POP instructions, so talking about PUSH encodings
is confusing. I suggest you drop point 2).
+ rtx parallel_op = operands[0];
+ /* Initialize to elements number of PARALLEL. */
+ unsigned indx = XVECLEN (parallel_op, 0) - 1;
+ /* Initialize the value to base register. */
+ unsigned regno = REGNO (operands[1]);
+ /* Skip return and write back pattern.
+ We only need register pop pattern for later analysis. */
+ unsigned first_indx = 0;
+ first_indx += return_pc ? 1 : 0;
+ first_indx += write_back_p ? 1 : 0;
+
+ /* A pop operation can be done through LDM or POP. If the base register is SP
+ and if it's with write back, then a LDM will be alias of POP. */
+ bool pop_p = (regno == SP_REGNUM && write_back_p);
+ bool ldm_p = !pop_p;
+
+ /* Check base register for LDM. */
+ if (ldm_p && REGNO_REG_CLASS (regno) == HI_REGS)
+ return 4;
+
+ /* Check each register in the list. */
+ for (; indx >= first_indx; indx--)
+ {
+ regno = REGNO (XEXP (XVECEXP (parallel_op, 0, indx), 0));
+ if (REGNO_REG_CLASS (regno) == HI_REGS
+ && (regno != PC_REGNUM || ldm_p))
+ return 4;
+ }
+
+ return 2;
+}
+
next prev parent reply other threads:[~2016-05-31 14:15 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-13 13:55 Jiong Wang
2016-05-19 8:19 ` Jiong Wang
2016-05-31 15:06 ` Kyrill Tkachov [this message]
2016-06-02 14:45 ` Jiong Wang
2016-06-09 11:09 ` Kyrill Tkachov
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=574D9C6F.4020306@foss.arm.com \
--to=kyrylo.tkachov@foss.arm.com \
--cc=Wilco.Dijkstra@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jiong.wang@foss.arm.com \
--cc=ramana.radhakrishnan@arm.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).