diff mbox

[aarch64] Fix 70048

Message ID 5756A6E3.3090105@foss.arm.com
State New
Headers show

Commit Message

Renlin Li June 7, 2016, 10:50 a.m. UTC 
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  <renlin.li@arm.com>

	* config/aarch64/aarch64.c (aarch64_legitimize_address): Add assignment.

Comments

James Greenhalgh June 14, 2016, 9:52 a.m. UTC | #1 
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  <renlin.li@arm.com>
> 
> 	* config/aarch64/aarch64.c (aarch64_legitimize_address): Add assignment.
diff mbox

Patch

commit 1fd77baf4ca918ed25dbce4678d7be7b7cd51be2
Author: Renlin Li <renlin.li@arm.com>
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)