[PATCH] Bug fix in store_bit_field_1 for big endian targets (issue 51893)

[PATCH] Bug fix in store_bit_field_1 for big endian targets (issue 51893)

[Aurelien Buhrig]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

Hi,

This patch (for 4.6) fixes a wrong subword index computation in
store_bit_field_1 for big endian targets when value is at least 4 times
bigger than a word (DI REG value with HI words).

It fixes a regression on gcc.c-torture/execute/bitfld-3.c for my current
backend port.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51893

OK to commit?

Aurélien

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 661 bytes --]

--- gcc-4.6.1.orig/gcc/expmed.c	2011-05-22 21:02:59.000000000 +0200
+++ src/gcc/expmed.c	2012-01-19 09:32:04.000000000 +0100
@@ -589,7 +589,10 @@
 	{
 	  /* If I is 0, use the low-order word in both field and target;
 	     if I is 1, use the next to lowest word; and so on.  */
-	  unsigned int wordnum = (backwards ? nwords - i - 1 : i);
+	  unsigned int wordnum = (backwards
+                                  ? GET_MODE_SIZE(fieldmode)/UNITS_PER_WORD
+                                        - i - 1 
+                                  : i);
 	  unsigned int bit_offset = (backwards
 				     ? MAX ((int) bitsize - ((int) i + 1)
 					    * BITS_PER_WORD,

Re: [PATCH] Bug fix in store_bit_field_1 for big endian targets (issue 51893)

[Eric Botcazou]

> This patch (for 4.6) fixes a wrong subword index computation in
> store_bit_field_1 for big endian targets when value is at least 4 times
> bigger than a word (DI REG value with HI words).
>
> It fixes a regression on gcc.c-torture/execute/bitfld-3.c for my current
> backend port.
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51893
>
> OK to commit?

It is OK for mainline on principle but there is no ChangeLog entry and the 
patch doesn't follow the GNU Coding Style: TABs instead of spaces, spaces 
before parentheses, etc.  See the equivalent code in extract_bit_field_1.
Moreover you need to properly test it on a mainstream big-endian platform.

See http://gcc.gnu.org/contribute.html for a more complete reference.

-- 
Eric Botcazou

Re: [PATCH] Bug fix in store_bit_field_1 for big endian targets (issue 51893)

[Alan Modra]

On Wed, Mar 21, 2012 at 10:53:07PM +0100, Eric Botcazou wrote:
> > This patch (for 4.6) fixes a wrong subword index computation in
> > store_bit_field_1 for big endian targets when value is at least 4 times
> > bigger than a word (DI REG value with HI words).
> >
> > It fixes a regression on gcc.c-torture/execute/bitfld-3.c for my current
> > backend port.
> >
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51893
> >
> > OK to commit?
> 
> It is OK for mainline on principle but there is no ChangeLog entry and the 
> patch doesn't follow the GNU Coding Style: TABs instead of spaces, spaces 
> before parentheses, etc.  See the equivalent code in extract_bit_field_1.
> Moreover you need to properly test it on a mainstream big-endian platform.

Passes bootstrap and regression test powerpc64-linux.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Bug fix in store_bit_field_1 for big endian targets (issue 51893)

[Eric Botcazou]

> Passes bootstrap and regression test powerpc64-linux.

Thanks a lot, Alan!

So, Aurelien, you only need to adjust the formatting of the patch and post a 
ChangeLog entry along with it.  TIA.

-- 
Eric Botcazou

Re: [PATCH] Bug fix in store_bit_field_1 for big endian targets (issue 51893)

[Aurelien Buhrig]

[-- Attachment #1: Type: text/plain, Size: 499 bytes --]


>> Passes bootstrap and regression test powerpc64-linux.
> 
> Thanks a lot, Alan!
> 
> So, Aurelien, you only need to adjust the formatting of the patch and post a 
> ChangeLog entry along with it.  TIA.
> 

Thanks Alan!
Bootstrap and regression test for m68k-elf ok, but I have trouble cross
compiling trunk for powerpc64-linux target...

Changelog:
* expmed.c (store_bit_field_1): Fix wordnum value for big endian targets

Will this fix be backported to 4.6 branch for next release?

Aurélien



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 603 bytes --]

Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c	(revision 185732)
+++ gcc/expmed.c	(working copy)
@@ -550,7 +550,10 @@
 	{
 	  /* If I is 0, use the low-order word in both field and target;
 	     if I is 1, use the next to lowest word; and so on.  */
-	  unsigned int wordnum = (backwards ? nwords - i - 1 : i);
+	  unsigned int wordnum = (backwards
+				  ? GET_MODE_SIZE (fieldmode) / UNITS_PER_WORD
+				  - i - 1
+				  : i);
 	  unsigned int bit_offset = (backwards
 				     ? MAX ((int) bitsize - ((int) i + 1)
 					    * BITS_PER_WORD,

Re: [PATCH] Bug fix in store_bit_field_1 for big endian targets (issue 51893)

[Eric Botcazou]

> Changelog:
> * expmed.c (store_bit_field_1): Fix wordnum value for big endian targets

The author line was missing so I put:

2012-03-27  Aurelien Buhrig  <aurelien.buhrig.gcc@gmail.com>

	PR middle-end/51893
	* expmed.c (store_bit_field_1): Fix wordnum value for big-endian
	targets.

> Will this fix be backported to 4.6 branch for next release?

The policy is to put only fixes for regressions on release branches, and this 
is obviously not a regression.  I think we might want to put it on the 4.7 
branch though, because it's early in the release cycle and we fixed a twin bug 
on that branch (PR middle-end/50325).  But it's up to the RMs, now CCed.

-- 
Eric Botcazou

Re: [PATCH] Bug fix in store_bit_field_1 for big endian targets (issue 51893)

[Richard Guenther]

On Tue, 27 Mar 2012, Eric Botcazou wrote:

> > Changelog:
> > * expmed.c (store_bit_field_1): Fix wordnum value for big endian targets
> 
> The author line was missing so I put:
> 
> 2012-03-27  Aurelien Buhrig  <aurelien.buhrig.gcc@gmail.com>
> 
> 	PR middle-end/51893
> 	* expmed.c (store_bit_field_1): Fix wordnum value for big-endian
> 	targets.
> 
> > Will this fix be backported to 4.6 branch for next release?
> 
> The policy is to put only fixes for regressions on release branches, and this 
> is obviously not a regression.  I think we might want to put it on the 4.7 
> branch though, because it's early in the release cycle and we fixed a twin bug 
> on that branch (PR middle-end/50325).  But it's up to the RMs, now CCed.

Yes, it's ok for the 4.7 branch.

Richard.