Re: [PATCH, aarch64] Fix 70048
On Tue, Jun 07, 2016 at 11:50:11AM +0100, Renlin Li wrote:
> Hi Richard,
>
> On 16/03/16 21:25, Richard Henderson wrote:
> >This fixes only the regression described in the PR.
> >
> >There was quite a bit of follow-up that points to new work that ought to
> >be done during the gcc7 cycle, but isn't really appropriate now.
> >
> >Tested on aarch64-linux; committed as reviewed in the PR.
> >
> >
> >r~
>
> >@@ -4953,74 +4963,43 @@ aarch64_legitimize_address (rtx x, rtx /* orig_x
> >*/, machine_mode mode)
> >
> > if (GET_CODE (x) == PLUS && CONST_INT_P (XEXP (x, 1)))
> > {
> >- HOST_WIDE_INT offset = INTVAL (XEXP (x, 1));
> >- HOST_WIDE_INT base_offset;
> >+ rtx base = XEXP (x, 0);
> >+ rtx offset_rtx XEXP (x, 1);
>
>
>
> I recently read the aarch64_legitimize_address function, and find a
> suspicious line of code in the above change.
>
> >> + rtx offset_rtx XEXP (x, 1);
>
> It's committed by you. It looks like a typo, and an assignment seems
> missing?
>
> James suggests me this is c++ initialization. Ah, yes it is! But I
> believe this is an coincident?
> As you have different initialization code above.
>
> I made an obvious patch to make it looks more intuitive, is it Okay?
Yeah, there's nothing syntactically invalid about this, but it is
inconsistent with the rest of the file, so please go ahead and apply
the fixup.
Thanks,
James
> gcc/changelog:
>
> 2016-06-06 renlin li
>
> * config/aarch64/aarch64.c (aarch64_legitimize_address): Add assignment.
Re: [PATCH, aarch64] Fix 70048
Hi Richard,
On 16/03/16 21:25, Richard Henderson wrote:
This fixes only the regression described in the PR.
There was quite a bit of follow-up that points to new work that ought to
be done during the gcc7 cycle, but isn't really appropriate now.
Tested on aarch64-linux; committed as reviewed in the PR.
r~
@@ -4953,74 +4963,43 @@ aarch64_legitimize_address (rtx x, rtx /* orig_x */,
machine_mode mode)
if (GET_CODE (x) == PLUS && CONST_INT_P (XEXP (x, 1)))
{
- HOST_WIDE_INT offset = INTVAL (XEXP (x, 1));
- HOST_WIDE_INT base_offset;
+ rtx base = XEXP (x, 0);
+ rtx offset_rtx XEXP (x, 1);
I recently read the aarch64_legitimize_address function, and find a
suspicious line of code in the above change.
>> + rtx offset_rtx XEXP (x, 1);
It's committed by you. It looks like a typo, and an assignment seems
missing?
James suggests me this is c++ initialization. Ah, yes it is! But I
believe this is an coincident?
As you have different initialization code above.
I made an obvious patch to make it looks more intuitive, is it Okay?
Regards,
Renlin Li
gcc/changelog:
2016-06-06 renlin li
* config/aarch64/aarch64.c (aarch64_legitimize_address): Add assignment.
commit 1fd77baf4ca918ed25dbce4678d7be7b7cd51be2
Author: Renlin Li
Date: Mon Jun 6 11:24:39 2016 +0100
fix type
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index ad07fe1..54e6813 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -4949,7 +4949,7 @@ aarch64_legitimize_address (rtx x, rtx /* orig_x */, machine_mode mode)
if (GET_CODE (x) == PLUS && CONST_INT_P (XEXP (x, 1)))
{
rtx base = XEXP (x, 0);
- rtx offset_rtx XEXP (x, 1);
+ rtx offset_rtx = XEXP (x, 1);
HOST_WIDE_INT offset = INTVAL (offset_rtx);
if (GET_CODE (base) == PLUS)
[PATCH, aarch64] Fix 70048
This fixes only the regression described in the PR.
There was quite a bit of follow-up that points to new work that ought to be
done during the gcc7 cycle, but isn't really appropriate now.
Tested on aarch64-linux; committed as reviewed in the PR.
r~
PR target/70048
* config/aarch64/aarch64.c (virt_or_elim_regno_p): New.
(aarch64_classify_address): Use it.
(aarch64_legitimize_address): Force all subexpressions of PLUS
into registers. Simplify as (sfp+const)+reg or (reg+reg)+const.
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index cf1239d..12e498d 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -3847,6 +3847,18 @@ aarch64_mode_valid_for_sched_fusion_p (machine_mode mode)
&& GET_MODE_SIZE (mode) == 8);
}
+/* Return true if REGNO is a virtual pointer register, or an eliminable
+ "soft" frame register. Like REGNO_PTR_FRAME_P except that we don't
+ include stack_pointer or hard_frame_pointer. */
+static bool
+virt_or_elim_regno_p (unsigned regno)
+{
+ return ((regno >= FIRST_VIRTUAL_REGISTER
+ && regno <= LAST_VIRTUAL_POINTER_REGISTER)
+ || regno == FRAME_POINTER_REGNUM
+ || regno == ARG_POINTER_REGNUM);
+}
+
/* Return true if X is a valid address for machine mode MODE. If it is,
fill in INFO appropriately. STRICT_P is true if REG_OK_STRICT is in
effect. OUTER_CODE is PARALLEL for a load/store pair. */
@@ -3890,9 +3902,7 @@ aarch64_classify_address (struct aarch64_address_info
*info,
if (! strict_p
&& REG_P (op0)
- && (op0 == virtual_stack_vars_rtx
- || op0 == frame_pointer_rtx
- || op0 == arg_pointer_rtx)
+ && virt_or_elim_regno_p (REGNO (op0))
&& CONST_INT_P (op1))
{
info->type = ADDRESS_REG_IMM;
@@ -4953,74 +4963,43 @@ aarch64_legitimize_address (rtx x, rtx /* orig_x */,
machine_mode mode)
if (GET_CODE (x) == PLUS && CONST_INT_P (XEXP (x, 1)))
{
- HOST_WIDE_INT offset = INTVAL (XEXP (x, 1));
- HOST_WIDE_INT base_offset;
+ rtx base = XEXP (x, 0);
+ rtx offset_rtx XEXP (x, 1);
+ HOST_WIDE_INT offset = INTVAL (offset_rtx);
- if (GET_CODE (XEXP (x, 0)) == PLUS)
+ if (GET_CODE (base) == PLUS)
{
- rtx op0 = XEXP (XEXP (x, 0), 0);
- rtx op1 = XEXP (XEXP (x, 0), 1);
+ rtx op0 = XEXP (base, 0);
+ rtx op1 = XEXP (base, 1);
- /* Address expressions of the form Ra + Rb + CONST.
+ /* Force any scaling into a temp for CSE. */
+ op0 = force_reg (Pmode, op0);
+ op1 = force_reg (Pmode, op1);
-If CONST is within the range supported by the addressing
-mode "reg+offset", do not split CONST and use the
-sequence
- Rt = Ra + Rb;
- addr = Rt + CONST. */
- if (REG_P (op0) && REG_P (op1))
- {
- machine_mode addr_mode = GET_MODE (x);
- rtx base = gen_reg_rtx (addr_mode);
- rtx addr = plus_constant (addr_mode, base, offset);
+ /* Let the pointer register be in op0. */
+ if (REG_POINTER (op1))
+ std::swap (op0, op1);
- if (aarch64_legitimate_address_hook_p (mode, addr, false))
- {
- emit_insn (gen_adddi3 (base, op0, op1));
- return addr;
- }
- }
- /* Address expressions of the form Ra + Rb< 16
|| mode == TImode)
base_offset = ((offset + 64 * GET_MODE_SIZE (mode))
@@ -5032,15 +5011,12 @@ aarch64_legitimize_address (rtx x, rtx /* orig_x */,
machine_mode mode)
else
base_offset = offset & ~0xfff;
- if (base_offset == 0)
- return x;
-
- offset -= base_offset;
- rtx base_reg = gen_reg_rtx (Pmode);
- rtx val = force_operand (plus_constant (Pmode, XEXP (x, 0), base_offset),
- NULL_RTX);
- emit_move_insn (base_reg, val);
- x = plus_constant (Pmode, base_reg, offset);
+ if (base_offset != 0)
+ {
+ base = plus_constant (Pmode, base, base_offset);
+ base = force_operand (base, NULL_RTX);
+ return plus_constant (Pmode, base, offset - base_offset);
+ }
}
return x;
Re: [PATCH, aarch64] Fix 70048
On Wed, Mar 16, 2016 at 02:25:27PM -0700, Richard Henderson wrote:
> PR target/70048
> * config/aarch64/aarch64.c (virt_or_elim_regno_p): New.
> (aarch64_classify_address): Use it.
> (aarch64_legitimize_address): Force all subexpressions of PLUS
> into registers. Simplify as (sfp+const)+reg or (reg+reg)+const.
>
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index cf1239d..12e498d 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -3847,6 +3847,18 @@ aarch64_mode_valid_for_sched_fusion_p (machine_mode
> mode)
>&& GET_MODE_SIZE (mode) == 8);
> }
>
> +/* Return true if REGNO is a virtual pointer register, or an eliminable
> + "soft" frame register. Like REGNO_PTR_FRAME_P except that we don't
> + include stack_pointer or hard_frame_pointer. */
In that case, do we want to write this as:
return REGNO_PTR_FRAME_P (regno)
&& regno != STACK_POINTER_REGNUM
&& regno != HARD_FRAME_POINTER_REGNUM;
for clarity?
> +static bool
> +virt_or_elim_regno_p (unsigned regno)
Most functions in here get the "aarch64" in their name even if they are
static.
> +{
> + return ((regno >= FIRST_VIRTUAL_REGISTER
> +&& regno <= LAST_VIRTUAL_POINTER_REGISTER)
> + || regno == FRAME_POINTER_REGNUM
> + || regno == ARG_POINTER_REGNUM);
> +}
Otherwise, this looks OK to me. Thanks for the fix.
James
