* [PATCH, PR66444] Handle -fipa-ra in reload_combine @ 2015-06-08 12:05 Tom de Vries 2015-06-08 15:37 ` Jakub Jelinek 0 siblings, 1 reply; 4+ messages in thread From: Tom de Vries @ 2015-06-08 12:05 UTC (permalink / raw) To: gcc-patches; +Cc: Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 1360 bytes --] Hi, this patch fixes PR66444, a problem with -fipa-ra in reload_combine. The problem is that for the test-case, reload_combine combines these two insns: ... (insn 13 12 14 2 (parallel [ (set (reg/v/f:DI 37 r8 [orig:92 xD.1858 ] [92]) (plus:DI (reg:DI 37 r8 [96]) (reg:DI 0 ax [orig:95 D.1884 ] [95]))) (clobber (reg:CC 17 flags)) ]) (expr_list:REG_EQUAL (plus:DI (reg:DI 0 ax [orig:95 D.1884 ] [95]) (const_int 962072674304 [0xe000000000])) (nil))) (insn 14 13 15 2 (set (reg:DI 5 di) (reg/v/f:DI 37 r8 [orig:92 xD.1858 ] [92])) (nil)) ... into this insn: ... (insn 14 12 15 2 (set (reg:DI 5 di) (plus:DI (reg:DI 37 r8 [96]) (reg:DI 0 ax [orig:95 D.1884 ] [95]))) (nil)) ... That removes the set of r8 by insn 13. And that set of r8 is used by insn 16: .... (call_insn 15 ...) (insn 16 15 17 2 (set (reg:DI 5 di) (reg/v/f:DI 37 r8 [orig:92 x ] [92])) test.c:33 85 {*movdi_internal} (nil)) ... But reload_combine doesn't acknowledge that use, because it considers that r8 is killed by call_insn 15. The patch fixes the problem by using get_call_reg_set_usage to find out that r8 is actually not killed by the call_insn. Bootstrapped and reg-tested on x86_64 on top of trunk. OK for trunk and gcc-5-branch? Thanks, - Tom [-- Attachment #2: 0001-Handle-fipa-ra-in-reload_combine.patch --] [-- Type: text/x-patch, Size: 1977 bytes --] Handle -fipa-ra in reload_combine 2015-06-08 Tom de Vries <tom@codesourcery.com> PR rtl-optimization/66444 * postreload.c (reload_combine): Use get_call_reg_set_usage instead of call_used_regs. * gcc.dg/pr66444.c: New test. --- gcc/postreload.c | 5 +++- gcc/testsuite/gcc.dg/pr66444.c | 58 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/gcc.dg/pr66444.c diff --git a/gcc/postreload.c b/gcc/postreload.c index 7ecca15..1cc7b14 100644 --- a/gcc/postreload.c +++ b/gcc/postreload.c @@ -1352,9 +1352,12 @@ reload_combine (void) if (CALL_P (insn)) { rtx link; + HARD_REG_SET used_regs; + + get_call_reg_set_usage (insn, &used_regs, call_used_reg_set); for (r = 0; r < FIRST_PSEUDO_REGISTER; r++) - if (call_used_regs[r]) + if (TEST_HARD_REG_BIT (used_regs, r)) { reg_state[r].use_index = RELOAD_COMBINE_MAX_USES; reg_state[r].store_ruid = reload_combine_ruid; diff --git a/gcc/testsuite/gcc.dg/pr66444.c b/gcc/testsuite/gcc.dg/pr66444.c new file mode 100644 index 0000000..93a7644 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr66444.c @@ -0,0 +1,58 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -fipa-ra" } */ + +extern void abort (void); + +int __attribute__((noinline, noclone)) +bar (void) +{ + return 1; +} + +struct S +{ + unsigned long p, q, r; + void *v; +}; + +struct S *s1; +struct S *s2; + +void __attribute__((noinline, noclone)) +fn2 (struct S *x) +{ + s2 = x; +} + +__attribute__((noinline, noclone)) void * +fn1 (struct S *x) +{ + /* Just a statement to make it a non-const function. */ + s1 = x; + + return (void *)0; +} + +int __attribute__((noinline, noclone)) +baz (void) +{ + struct S *x = (struct S *) 0xe0000000U; + + x += bar (); + + fn1 (x); + fn2 (x); + + return 0; +} + +int +main (void) +{ + baz (); + + if (s2 != (((struct S *) 0xe0000000U) + 1)) + abort (); + + return 0; +} -- 1.9.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH, PR66444] Handle -fipa-ra in reload_combine 2015-06-08 12:05 [PATCH, PR66444] Handle -fipa-ra in reload_combine Tom de Vries @ 2015-06-08 15:37 ` Jakub Jelinek 2015-06-08 17:47 ` Tom de Vries 2015-06-08 17:52 ` Tom de Vries 0 siblings, 2 replies; 4+ messages in thread From: Jakub Jelinek @ 2015-06-08 15:37 UTC (permalink / raw) To: Tom de Vries; +Cc: gcc-patches On Mon, Jun 08, 2015 at 02:04:12PM +0200, Tom de Vries wrote: > this patch fixes PR66444, a problem with -fipa-ra in reload_combine. > > The problem is that for the test-case, reload_combine combines these two > insns: Please work out with Vlad whether reload_cse_move2add doesn't need similar fix (and check other spots too). > 2015-06-08 Tom de Vries <tom@codesourcery.com> > > PR rtl-optimization/66444 > * postreload.c (reload_combine): Use get_call_reg_set_usage instead of > call_used_regs. LGTM. > * gcc.dg/pr66444.c: New test. > +int __attribute__((noinline, noclone)) > +baz (void) > +{ > + struct S *x = (struct S *) 0xe0000000U; I'm still afraid this will not really work on s390-linux (which has only 31-bit pointers) and will not work on 16-bit int targets either (some have say 24-bit pointers etc., not really familiar with the embedded world). So, I'd suggest use a macro for the address, so you don't need to duplicate it, and define it to say ((struct S *) 0x8000UL), if it reproduces even with that change without your reload_combine fix. Ok for trunk and 5.2 with that change. Jakub ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH, PR66444] Handle -fipa-ra in reload_combine 2015-06-08 15:37 ` Jakub Jelinek @ 2015-06-08 17:47 ` Tom de Vries 2015-06-08 17:52 ` Tom de Vries 1 sibling, 0 replies; 4+ messages in thread From: Tom de Vries @ 2015-06-08 17:47 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 1607 bytes --] On 08/06/15 17:31, Jakub Jelinek wrote: > On Mon, Jun 08, 2015 at 02:04:12PM +0200, Tom de Vries wrote: >> this patch fixes PR66444, a problem with -fipa-ra in reload_combine. >> >> The problem is that for the test-case, reload_combine combines these two >> insns: > > Please work out with Vlad whether reload_cse_move2add doesn't need similar > fix (and check other spots too). > >> 2015-06-08 Tom de Vries <tom@codesourcery.com> >> >> PR rtl-optimization/66444 >> * postreload.c (reload_combine): Use get_call_reg_set_usage instead of >> call_used_regs. > > LGTM. > >> * gcc.dg/pr66444.c: New test. > >> +int __attribute__((noinline, noclone)) >> +baz (void) >> +{ >> + struct S *x = (struct S *) 0xe0000000U; > > I'm still afraid this will not really work on s390-linux (which has only > 31-bit pointers) and will not work on 16-bit int targets either > (some have say 24-bit pointers etc., not really familiar with the embedded > world). > So, I'd suggest use a macro for the address, so you don't need to duplicate > it, Used a macro CONST_PTR. > and define it to say ((struct S *) 0x8000UL), if it reproduces > even with that change without your reload_combine fix. Unfortunately, it didn't. So I used __SIZEOF_POINTER__ to produce a valid constant pointer for the 16-31 bit pointer-size cases, while still triggering the original problem for x86_64 -m64 case using a larger pointer. Furthermore, I used __SIZEOF_POINTER__ to ensured a valid pointer constant suffix. > > Ok for trunk and 5.2 with that change. Committed to trunk and backported gcc-5-branch as attached. Thanks, - Tom [-- Attachment #2: 0001-Handle-fipa-ra-in-reload_combine.patch --] [-- Type: text/x-patch, Size: 2733 bytes --] Handle -fipa-ra in reload_combine 2015-06-08 Tom de Vries <tom@codesourcery.com> PR rtl-optimization/66444 * postreload.c (reload_combine): Use get_call_reg_set_usage instead of call_used_regs. * gcc.dg/pr66444.c: New test. --- gcc/postreload.c | 5 ++- gcc/testsuite/gcc.dg/pr66444.c | 79 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/gcc.dg/pr66444.c diff --git a/gcc/postreload.c b/gcc/postreload.c index 7ecca15..1cc7b14 100644 --- a/gcc/postreload.c +++ b/gcc/postreload.c @@ -1352,9 +1352,12 @@ reload_combine (void) if (CALL_P (insn)) { rtx link; + HARD_REG_SET used_regs; + + get_call_reg_set_usage (insn, &used_regs, call_used_reg_set); for (r = 0; r < FIRST_PSEUDO_REGISTER; r++) - if (call_used_regs[r]) + if (TEST_HARD_REG_BIT (used_regs, r)) { reg_state[r].use_index = RELOAD_COMBINE_MAX_USES; reg_state[r].store_ruid = reload_combine_ruid; diff --git a/gcc/testsuite/gcc.dg/pr66444.c b/gcc/testsuite/gcc.dg/pr66444.c new file mode 100644 index 0000000..3f92a5c --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr66444.c @@ -0,0 +1,79 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -fipa-ra" } */ + +extern void abort (void); + +#if (__SIZEOF_LONG_LONG__ == __SIZEOF_POINTER__) +#define ADD_SUFFIX(a) a ## ULL +#elif (__SIZEOF_LONG__ == __SIZEOF_POINTER__) +#define ADD_SUFFIX(a) a ## UL +#elif (__SIZEOF_INT__ == __SIZEOF_POINTER__) +#define ADD_SUFFIX(a) a ## U +#else +#error Add target support here +#endif + +#if __SIZEOF_POINTER__ <= 4 +/* Use a 16 bit pointer to have a valid pointer for 16-bit to 31-bit pointer + architectures. Using sizeof, we cannot distinguish between 31-bit and 32-bit + pointer types, so we also handle the 32-bit pointer type case here. */ +#define CONST_PTR ADD_SUFFIX (0x800) +#else +/* For x86_64 -m64, the problem reproduces with this 32-bit CONST_PTR, but not + with a 2-power below it. */ +#define CONST_PTR ADD_SUFFIX (0x80000000) +#endif + +int __attribute__((noinline, noclone)) +bar (void) +{ + return 1; +} + +struct S +{ + unsigned long p, q, r; + void *v; +}; + +struct S *s1; +struct S *s2; + +void __attribute__((noinline, noclone)) +fn2 (struct S *x) +{ + s2 = x; +} + +__attribute__((noinline, noclone)) void * +fn1 (struct S *x) +{ + /* Just a statement to make it a non-const function. */ + s1 = x; + + return (void *)0; +} + +int __attribute__((noinline, noclone)) +baz (void) +{ + struct S *x = (struct S *) CONST_PTR; + + x += bar (); + + fn1 (x); + fn2 (x); + + return 0; +} + +int +main (void) +{ + baz (); + + if (s2 != (((struct S *) CONST_PTR) + 1)) + abort (); + + return 0; +} -- 1.9.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH, PR66444] Handle -fipa-ra in reload_combine 2015-06-08 15:37 ` Jakub Jelinek 2015-06-08 17:47 ` Tom de Vries @ 2015-06-08 17:52 ` Tom de Vries 1 sibling, 0 replies; 4+ messages in thread From: Tom de Vries @ 2015-06-08 17:52 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches On 08/06/15 17:31, Jakub Jelinek wrote: > On Mon, Jun 08, 2015 at 02:04:12PM +0200, Tom de Vries wrote: >> >this patch fixes PR66444, a problem with -fipa-ra in reload_combine. >> > >> >The problem is that for the test-case, reload_combine combines these two >> >insns: > Please work out with Vlad whether reload_cse_move2add doesn't need similar > fix (and check other spots too). > Filed PR66463 - review uses of call_used_regs and regs_invalidated_by_call. Thanks, - Tom ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-06-08 17:47 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-08 12:05 [PATCH, PR66444] Handle -fipa-ra in reload_combine Tom de Vries 2015-06-08 15:37 ` Jakub Jelinek 2015-06-08 17:47 ` Tom de Vries 2015-06-08 17:52 ` Tom de Vries
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).