* [Patch, tentative, reload] Make push_reload work with more types of subregs?
@ 2016-07-28 7:34 Senthil Kumar Selvaraj
2016-07-29 11:28 ` Bernd Schmidt
0 siblings, 1 reply; 6+ messages in thread
From: Senthil Kumar Selvaraj @ 2016-07-28 7:34 UTC (permalink / raw)
To: gcc-patches, Bernd Schmidt
Hi,
When analyzing PR 71873 (ICE in push_reload), I found that that code
in push_reload that recursively calls push_reload for subreg
expressions doesn't correctly set subreg_in_class for a few cases.
Specifically, reload_inner_reg_of_subreg returns true if SUBREG_REG(x)
is CONSTANT_P or if it's code is PLUS. The code that tries to find a
valid class (before recursively calling push_reload), however, only
does that if SUBREG_REG is REG_P or if it's a SYMBOL_REF. For the
other cases, subreg_in_class is set to the default NO_REGS, and this
triggers the rclass != NO_REGS assert just before find_reusable_reload.
For PR 71873, reload sees
(set (reg/f:HI 87)
(const:HI (plus:HI (symbol_ref:HI ("a") <var_decl 0x7ff218a11bd0 a>)
(const_int 1 [0x1])))) ../test.c:24 83 {*movhi}
(expr_list:REG_EQUIV (const:HI (plus:HI (symbol_ref:HI ("a") <var_decl 0x7ff218a11bd0 a>)
(const_int 1 [0x1])))
and
(set (mem:QI (post_dec:HI (reg/f:HI 32 __SP_L__)) [0 S1 A8])
(subreg:QI (reg/f:HI 87) 1))
and decides to replace pseudo reg 87 in the latter insn with the
REG_EQUIV it found in the former. The resulting RTL expression
(subreg:QI (const:HI (plus:HI (symbol_ref:HI ("a") <var_decl 0x7ffff7ff5bd0 a>)
(const_int 1 [0x1]))) 1)
does not match any of the conditions that handle subregs because
subreg_low_part is false and the inner expr is not a REG or a SYMBOL_REF.
Is there a reason why only REG and SYMBOL_REFs get valid
subreg_in_class? I tried extending it handle constants and PLUS
expressions, and it fixes PR 71873. It also fixes a another
bug that was a work around for the reload failure (PR 64452) - that
had a plus expression instead of the const.
Reg testing on avr and x86_64 did not show any new failures. Is this
the right way to fix this?
Regards
Senthil
diff --git gcc/reload.c gcc/reload.c
index 06426d9..f80d849 100644
--- gcc/reload.c
+++ gcc/reload.c
@@ -1141,7 +1141,9 @@ push_reload (rtx in, rtx out, rtx *inloc, rtx *outloc,
SUBREG_BYTE (in),
GET_MODE (in)),
REGNO (SUBREG_REG (in)));
- else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF)
+ else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF
+ || CONSTANT_P (SUBREG_REG (in))
+ || GET_CODE (SUBREG_REG (in)) == PLUS)
subreg_in_class = find_valid_class_1 (inmode,
GET_MODE (SUBREG_REG (in)),
rclass);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch, tentative, reload] Make push_reload work with more types of subregs?
2016-07-28 7:34 [Patch, tentative, reload] Make push_reload work with more types of subregs? Senthil Kumar Selvaraj
@ 2016-07-29 11:28 ` Bernd Schmidt
2016-08-08 15:27 ` Senthil Kumar Selvaraj
0 siblings, 1 reply; 6+ messages in thread
From: Bernd Schmidt @ 2016-07-29 11:28 UTC (permalink / raw)
To: Senthil Kumar Selvaraj, gcc-patches
On 07/28/2016 09:33 AM, Senthil Kumar Selvaraj wrote:
>
> Is there a reason why only REG and SYMBOL_REFs get valid
> subreg_in_class? I tried extending it handle constants and PLUS
> expressions, and it fixes PR 71873. It also fixes a another
> bug that was a work around for the reload failure (PR 64452) - that
> had a plus expression instead of the const.
>
> Reg testing on avr and x86_64 did not show any new failures. Is this
> the right way to fix this?
I think it looks quite plausible. Note that testing x86_64 on trunk will
not do anything - it is no longer using reload. Could you go back to an
older branch (4.7 I think is the last one using reload) and retest
x86_64 with that, for better test coverage?
Bernd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch, tentative, reload] Make push_reload work with more types of subregs?
2016-07-29 11:28 ` Bernd Schmidt
@ 2016-08-08 15:27 ` Senthil Kumar Selvaraj
2016-08-09 12:14 ` Bernd Schmidt
0 siblings, 1 reply; 6+ messages in thread
From: Senthil Kumar Selvaraj @ 2016-08-08 15:27 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: gcc-patches
Bernd Schmidt writes:
> On 07/28/2016 09:33 AM, Senthil Kumar Selvaraj wrote:
>>
>> Is there a reason why only REG and SYMBOL_REFs get valid
>> subreg_in_class? I tried extending it handle constants and PLUS
>> expressions, and it fixes PR 71873. It also fixes a another
>> bug that was a work around for the reload failure (PR 64452) - that
>> had a plus expression instead of the const.
>>
>> Reg testing on avr and x86_64 did not show any new failures. Is this
>> the right way to fix this?
>
> I think it looks quite plausible. Note that testing x86_64 on trunk will
> not do anything - it is no longer using reload. Could you go back to an
> older branch (4.7 I think is the last one using reload) and retest
> x86_64 with that, for better test coverage?
I picked out the commit where you'd added SYMBOL_REF handling (rev
#190252), and patched that with the below code. Bootstrapped and
regtested on x86_64-pc-linux - the results were identical with and
without the patch. Is this good enough for trunk?
Regards
Senthil
gcc/ChangeLog:
2016-08-08 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com>
PR reload/71873
* reload.c (push_reload): Compute subreg_in_class for
subregs of constants and plus expressions.
gcc/testsuite/ChangeLog:
2016-08-08 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com>
* gcc.target/avr/pr71873.c: New test.
Index: gcc/reload.c
===================================================================
--- gcc/reload.c (revision 239239)
+++ gcc/reload.c (working copy)
@@ -1141,7 +1141,9 @@
SUBREG_BYTE (in),
GET_MODE (in)),
REGNO (SUBREG_REG (in)));
- else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF)
+ else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF
+ || CONSTANT_P (SUBREG_REG (in))
+ || GET_CODE (SUBREG_REG (in)) == PLUS)
subreg_in_class = find_valid_class_1 (inmode,
GET_MODE (SUBREG_REG (in)),
rclass);
Index: gcc/testsuite/gcc.target/avr/pr71873.c
===================================================================
--- gcc/testsuite/gcc.target/avr/pr71873.c (nonexistent)
+++ gcc/testsuite/gcc.target/avr/pr71873.c (working copy)
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -fcaller-saves" } */
+
+#include <stdint.h>
+
+typedef struct {
+ uint8_t x;
+ uint32_t y;
+} A;
+
+A a;
+
+extern int bar(int);
+extern int foo (char *s, ...);
+
+extern uint8_t param;
+extern uint8_t h,m,s,ld,lm;
+extern uint16_t d;
+
+void gps_parse_string(int z)
+{
+ while (bar(z))
+ {
+ switch (param)
+ {
+ case 0: foo("a", &h, &m, &s, &d); break;
+ case 1: foo("d", &ld, &lm, &a.y); break;
+ }
+ }
+}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch, tentative, reload] Make push_reload work with more types of subregs?
2016-08-08 15:27 ` Senthil Kumar Selvaraj
@ 2016-08-09 12:14 ` Bernd Schmidt
2016-08-10 12:41 ` Senthil Kumar Selvaraj
0 siblings, 1 reply; 6+ messages in thread
From: Bernd Schmidt @ 2016-08-09 12:14 UTC (permalink / raw)
To: Senthil Kumar Selvaraj; +Cc: gcc-patches
On 08/08/2016 05:26 PM, Senthil Kumar Selvaraj wrote:
> I picked out the commit where you'd added SYMBOL_REF handling (rev
> #190252), and patched that with the below code. Bootstrapped and
> regtested on x86_64-pc-linux - the results were identical with and
> without the patch. Is this good enough for trunk?
> - else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF)
> + else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF
> + || CONSTANT_P (SUBREG_REG (in))
> + || GET_CODE (SUBREG_REG (in)) == PLUS)
> subreg_in_class = find_valid_class_1 (inmode,
> GET_MODE (SUBREG_REG (in)),
> rclass);
Actually SYMBOL_REF should also be CONSTANT_P. For integers it looks
like it'll pass VOIDmode to find_valid_class_1 and just return NO_REGS.
which I suppose is OK.
Would you mind removing the SYMBOL_REF test and retesting? Ok with that
change.
Bernd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch, tentative, reload] Make push_reload work with more types of subregs?
2016-08-09 12:14 ` Bernd Schmidt
@ 2016-08-10 12:41 ` Senthil Kumar Selvaraj
2016-08-11 10:25 ` Eric Botcazou
0 siblings, 1 reply; 6+ messages in thread
From: Senthil Kumar Selvaraj @ 2016-08-10 12:41 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: gcc-patches
Bernd Schmidt writes:
> On 08/08/2016 05:26 PM, Senthil Kumar Selvaraj wrote:
>
>> I picked out the commit where you'd added SYMBOL_REF handling (rev
>> #190252), and patched that with the below code. Bootstrapped and
>> regtested on x86_64-pc-linux - the results were identical with and
>> without the patch. Is this good enough for trunk?
>
>> - else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF)
>> + else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF
>> + || CONSTANT_P (SUBREG_REG (in))
>> + || GET_CODE (SUBREG_REG (in)) == PLUS)
>> subreg_in_class = find_valid_class_1 (inmode,
>> GET_MODE (SUBREG_REG (in)),
>> rclass);
>
> Actually SYMBOL_REF should also be CONSTANT_P. For integers it looks
> like it'll pass VOIDmode to find_valid_class_1 and just return NO_REGS.
> which I suppose is OK.
>
> Would you mind removing the SYMBOL_REF test and retesting? Ok with that
> change.
Bootstrapped and reg tested below patch with same setup as above - no
regressions showed up.
Committed patch to trunk. Ok for backport to 6.x and 5.x branches as
well?
Regards
Senthil
gcc/ChangeLog
2016-08-10 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com>
PR target/71873
* reload.c (push_reload): Compute subreg_in_class for
subregs of constants and plus expressions. Remove special
handling of SYMBOL_REFs.
gcc/testsuite/ChangeLog
2016-08-10 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com>
PR target/71873
* gcc.target/avr/pr71873.c: New test.
Index: gcc/reload.c
===================================================================
--- gcc/reload.c (revision 239318)
+++ gcc/reload.c (working copy)
@@ -1141,7 +1141,8 @@
SUBREG_BYTE (in),
GET_MODE (in)),
REGNO (SUBREG_REG (in)));
- else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF)
+ else if (CONSTANT_P (SUBREG_REG (in))
+ || GET_CODE (SUBREG_REG (in)) == PLUS)
subreg_in_class = find_valid_class_1 (inmode,
GET_MODE (SUBREG_REG (in)),
rclass);
Index: gcc/testsuite/gcc.target/avr/pr71873.c
===================================================================
--- gcc/testsuite/gcc.target/avr/pr71873.c (nonexistent)
+++ gcc/testsuite/gcc.target/avr/pr71873.c (working copy)
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -fcaller-saves" } */
+
+#include <stdint.h>
+
+typedef struct {
+ uint8_t x;
+ uint32_t y;
+} A;
+
+A a;
+
+extern int bar(int);
+extern int foo (char *s, ...);
+
+extern uint8_t param;
+extern uint8_t h,m,s,ld,lm;
+extern uint16_t d;
+
+void gps_parse_string(int z)
+{
+ while (bar(z))
+ {
+ switch (param)
+ {
+ case 0: foo("a", &h, &m, &s, &d); break;
+ case 1: foo("d", &ld, &lm, &a.y); break;
+ }
+ }
+}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch, tentative, reload] Make push_reload work with more types of subregs?
2016-08-10 12:41 ` Senthil Kumar Selvaraj
@ 2016-08-11 10:25 ` Eric Botcazou
0 siblings, 0 replies; 6+ messages in thread
From: Eric Botcazou @ 2016-08-11 10:25 UTC (permalink / raw)
To: Senthil Kumar Selvaraj; +Cc: gcc-patches, Bernd Schmidt
> Committed patch to trunk. Ok for backport to 6.x and 5.x branches as
> well?
No, we really avoid touching reload on release branches, unless there is a
very good reason to do it, like a regression on a primary platform.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-08-11 10:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-28 7:34 [Patch, tentative, reload] Make push_reload work with more types of subregs? Senthil Kumar Selvaraj
2016-07-29 11:28 ` Bernd Schmidt
2016-08-08 15:27 ` Senthil Kumar Selvaraj
2016-08-09 12:14 ` Bernd Schmidt
2016-08-10 12:41 ` Senthil Kumar Selvaraj
2016-08-11 10:25 ` Eric Botcazou
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).