Re: [patch 79279] combine/simplify_set issue
Hi Segher, This patch fixes a combiner bug in simplify_set which calls CANNOT_CHANGE_MODE_CLASS with wrong mode params. It occurs when trying to simplify paradoxical subregs of hw regs (whose natural mode is lower than a word). In fact, changing from (set x:m1 (subreg:m1 (op:m2))) to (set (subreg:m2 x) op2) is valid if REG_CANNOT_CHANGE_MODE_P (x, m1, m2) is false and REG_CANNOT_CHANGE_MODE_P (x, GET_MODE (src), GET_MODE (SUBREG_REG (src)) >>> r62212 (in 2003) changed it to what we have now, it used to be what you >>> want to change it back to. >>> >>> You say m1 > m2, which means you have WORD_REGISTER_OPERATIONS defined. >> No, just some hard regs whose natural mode size is 2 and UNIT_PER_WORD >> is 4... > You said it is a paradoxical subreg? Or do you mean the result is > a paradoxical subreg? I mean that the result is a paradoxical subreg: (subreg:SI (reg:HI r0) 0) = (reg:SI r0) whith r0 is a HI reg. >>> Where does this transformation go wrong? Why is the resulting insn >>> recognised at all? For example, general_operand should refuse it. >>> Maybe you have custom *_operand that do not handle subreg correctly? >>> >>> The existing code looks correct: what we want to know is if an m2 >>> interpreted as an m1 yields the correct value. We might *also* want >>> your condition, but I'm not sure about that. >> OK, looks like both m1->m2 & m2 -> m1 checks would be needed, but the m1 >> -> m2 should be filtererd by valid predicates (general_operand). >> Sorry about that. > Hrm, maybe you can show the RTL before and after this transform? RTL before combine: (set (reg:SI 31 (lshiftt:SI (reg:SI 29) (const_int 16 (set (reg:HI 1 "r1") (reg:HI 25)) (set (reg:HI 0 "r0") (subreg:HI (reg:SI 31) 0)); LE target r0 and r1 are HI regs RTL after combining 1 --> 3: (set (reg:HI 1 "r1") (reg:HI 25)) (set (reg:SI 0 "r0") (lshift:SI (reg:SI 29) (const_int 16))) and r1 is clobbered by last insn. Thanks, Aurélien
Re: [patch 79279] combine/simplify_set issue
On 31/01/2017 22:15, Segher Boessenkool wrote: > Hello, > > On Mon, Jan 30, 2017 at 10:43:23AM +0100, Aurelien Buhrig wrote: >> This patch fixes a combiner bug in simplify_set which calls >> CANNOT_CHANGE_MODE_CLASS with wrong mode params. >> It occurs when trying to simplify paradoxical subregs of hw regs (whose >> natural mode is lower than a word). >> >> In fact, changing from (set x:m1 (subreg:m1 (op:m2))) to (set (subreg:m2 >> x) op2) is valid if REG_CANNOT_CHANGE_MODE_P (x, m1, m2) is false >> and REG_CANNOT_CHANGE_MODE_P (x, GET_MODE (src), GET_MODE (SUBREG_REG >> (src)) > r62212 (in 2003) changed it to what we have now, it used to be what you > want to change it back to. > > You say m1 > m2, which means you have WORD_REGISTER_OPERATIONS defined. No, just some hard regs whose natural mode size is 2 and UNIT_PER_WORD is 4... > > Where does this transformation go wrong? Why is the resulting insn > recognised at all? For example, general_operand should refuse it. > Maybe you have custom *_operand that do not handle subreg correctly? > > The existing code looks correct: what we want to know is if an m2 > interpreted as an m1 yields the correct value. We might *also* want > your condition, but I'm not sure about that. OK, looks like both m1->m2 & m2 -> m1 checks would be needed, but the m1 -> m2 should be filtererd by valid predicates (general_operand). Sorry about that. >>See: >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79279 >> >> Validated on a private target, >> bootstraped on x86_64-pc-linux-gnu, but I'm not sure which target is the >> most relevant for this patch though ... >> >> OK to commit? > Sorry, no. We're currently in development stage 4, and this is not a > regression (see <https://gcc.gnu.org/develop.html#stage4>). But we can > of course discuss this further, and you can repost the patch when stage 1 > opens (a few months from now) if you still want it. OK, but not sure if it needs to be patched any more. Aurélien
[patch 79279] combine/simplify_set issue
Hi, This patch fixes a combiner bug in simplify_set which calls CANNOT_CHANGE_MODE_CLASS with wrong mode params. It occurs when trying to simplify paradoxical subregs of hw regs (whose natural mode is lower than a word). In fact, changing from (set x:m1 (subreg:m1 (op:m2))) to (set (subreg:m2 x) op2) is valid if REG_CANNOT_CHANGE_MODE_P (x, m1, m2) is false and REG_CANNOT_CHANGE_MODE_P (x, GET_MODE (src), GET_MODE (SUBREG_REG (src)) See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79279 Validated on a private target, bootstraped on x86_64-pc-linux-gnu, but I'm not sure which target is the most relevant for this patch though ... OK to commit? Aurélien Changelog: 2017-01-27 Aurelien Buhrig <aurelien.buhrig@gmail.com> * combine.c (simplify_set): Fix call to REG_CANNOT_CHANGE_MODE_CLASS_P Index: gcc/combine.c === --- gcc/combine.c (revision 244978) +++ gcc/combine.c (working copy) @@ -6796,8 +6796,8 @@ #ifdef CANNOT_CHANGE_MODE_CLASS && ! (REG_P (dest) && REGNO (dest) < FIRST_PSEUDO_REGISTER && REG_CANNOT_CHANGE_MODE_P (REGNO (dest), -GET_MODE (SUBREG_REG (src)), -GET_MODE (src))) +GET_MODE (src), +GET_MODE (SUBREG_REG (src #endif && (REG_P (dest) || (GET_CODE (dest) == SUBREG
Re: [PATCH] Bug fix in store_bit_field_1 for big endian targets (issue 51893)
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
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,
[PATCH] Bug fix in store_bit_field_1 for big endian targets (issue 51893)
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
--- gcc-4.6.1.orig/gcc/expmed.c 2011-05-22 21:02:59.0 +0200
+++ src/gcc/expmed.c2012-01-19 09:32:04.0 +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: Bug store_bit_field_1 + patch
Le 21/01/2012 03:37, Alan Modra a écrit : The problem occurs when value is a REG and bitsize BITS_PER_WORD. This is because wordnum, which is used to get the subword of value, is incorrectly computed, in BIG_ENDIAN, wrt the number of words needed by bitsize instead of the number of words needed by the integer mode of the field, which is the mode used to compute subwords. That doesn't sound right. wordnum is passed to operand_subword as its offset param, which counts in words of word_mode size. I think you have a problem elsewhere. The offset is wrong. I filed a bug. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51893
Bug store_bit_field_1 + patch
Hi,
I've found a bug in store_bit_field_1 for BIG_ENDIAN targets whose words
are HI. The testcase is execute.exp=bitfld-3.c for my target (which is
not public).
It occurs on 4.6.1 release, but seem to be present in trunk (looking at
the code, not executed).
The problem occurs when value is a REG and bitsize BITS_PER_WORD. This
is because wordnum, which is used to get the subword of value, is
incorrectly computed, in BIG_ENDIAN, wrt the number of words needed by
bitsize instead of the number of words needed by the integer mode of the
field, which is the mode used to compute subwords.
For instance, for a 40bit field to be set by a DI reg (with HI words),
the offset of the LSWord of the DI reg should be 3, not 2 as currently
computed.
The following patch seems to solve the issue. Tested on the C testsuite
without any regression (for my target only).
Hope it helps,
Aurélien
--- expmed.c.orig 2012-01-18 11:48:21.0 +0100
+++ expmed.c2012-01-18 11:49:19.0 +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,
