| Message ID | 5808DB6A.6070105@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
On 10/20/2016 08:57 AM, Kyrill Tkachov wrote: > Hi all, > > In this PR we've got code like this: > register struct test2_s *g __asm__("x28"); > > void > do_something () > { > test_fptr (); > struct test2_s *p1 = 0; > *p1 = *g; > } > > And we get an ICE in gcc/ree.c:655 in get_sub_rtx. > The problem is when we try to process the defining insn of register x28 > from the insn: > (set (reg/f:DI 1 x1 [orig:74 g.1_2 ] [74]) > (zero_extend:DI (reg/v:SI 28 x28 [ g ]))) > > The dataflow reports the insn setting x28 to be: > (call_insn 8 7 10 2 (parallel [ > (call (mem:DI (reg/f:DI 0 x0 [orig:77 test_fptr ] [77]) [0 > *test_fptr.0_1 S8 A8]) > (const_int 0 [0])) > (use (const_int 0 [0])) > (clobber (reg:DI 30 x30)) > ]) "ree.c":14 41 {*call_reg} > (expr_list:REG_CALL_DECL (nil) > (nil)) > (expr_list (clobber (reg:DI 17 x17)) > (expr_list (clobber (reg:DI 16 x16)) > (nil)))) > > which, as you can see, doesn't actually set x28. > AFAICS the reason dataflow reports call_insn 8 as defining x28 is > because x28 is a global register variable > and that means that every function call is considered to define it. > But the ree pass can't really use any of that. It only wants some SET > RTL patterns to play with. > One solution is to bail out at the ICE location, in get_sub_rtx, when no > SETs are found inside the parallel. > But I think we shouldn't be getting this far along anyway. > So this patch prevents the call_insn from being recognised as a defining > insn for x28 for the purposes of ree. > It makes sure that if the register we're extending is a global register > that all the insns in the def chain > actually set it directly as determined by set_of from rtlanal.c. > This should hopefully still allow ree to optimise extensions of global > registers as long as they are combined > with legitimate defining insns. > > Bootstrapped and tested on aarch64, arm, x86_64. > > Ok for trunk? > > Thanks, > Kyrill > > 2016-10-20 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR rtl-optimization/78038 > * ree.c (get_defs): Return NULL if a defining insn for REG cannot > be deduced to set REG through the RTL structure. > (make_defs_and_copies_lists): Return false on a failing get_defs call. > > 2016-10-20 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR rtl-optimization/78038 > * gcc.target/aarch64/pr78038.c: New test. OK. Jeff
On 20/10/16 19:52, Jeff Law wrote: > On 10/20/2016 08:57 AM, Kyrill Tkachov wrote: >> Hi all, >> >> In this PR we've got code like this: >> register struct test2_s *g __asm__("x28"); >> >> void >> do_something () >> { >> test_fptr (); >> struct test2_s *p1 = 0; >> *p1 = *g; >> } >> >> And we get an ICE in gcc/ree.c:655 in get_sub_rtx. >> The problem is when we try to process the defining insn of register x28 >> from the insn: >> (set (reg/f:DI 1 x1 [orig:74 g.1_2 ] [74]) >> (zero_extend:DI (reg/v:SI 28 x28 [ g ]))) >> >> The dataflow reports the insn setting x28 to be: >> (call_insn 8 7 10 2 (parallel [ >> (call (mem:DI (reg/f:DI 0 x0 [orig:77 test_fptr ] [77]) [0 >> *test_fptr.0_1 S8 A8]) >> (const_int 0 [0])) >> (use (const_int 0 [0])) >> (clobber (reg:DI 30 x30)) >> ]) "ree.c":14 41 {*call_reg} >> (expr_list:REG_CALL_DECL (nil) >> (nil)) >> (expr_list (clobber (reg:DI 17 x17)) >> (expr_list (clobber (reg:DI 16 x16)) >> (nil)))) >> >> which, as you can see, doesn't actually set x28. >> AFAICS the reason dataflow reports call_insn 8 as defining x28 is >> because x28 is a global register variable >> and that means that every function call is considered to define it. >> But the ree pass can't really use any of that. It only wants some SET >> RTL patterns to play with. >> One solution is to bail out at the ICE location, in get_sub_rtx, when no >> SETs are found inside the parallel. >> But I think we shouldn't be getting this far along anyway. >> So this patch prevents the call_insn from being recognised as a defining >> insn for x28 for the purposes of ree. >> It makes sure that if the register we're extending is a global register >> that all the insns in the def chain >> actually set it directly as determined by set_of from rtlanal.c. >> This should hopefully still allow ree to optimise extensions of global >> registers as long as they are combined >> with legitimate defining insns. >> >> Bootstrapped and tested on aarch64, arm, x86_64. >> >> Ok for trunk? >> >> Thanks, >> Kyrill >> >> 2016-10-20 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR rtl-optimization/78038 >> * ree.c (get_defs): Return NULL if a defining insn for REG cannot >> be deduced to set REG through the RTL structure. >> (make_defs_and_copies_lists): Return false on a failing get_defs call. >> >> 2016-10-20 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR rtl-optimization/78038 >> * gcc.target/aarch64/pr78038.c: New test. > OK. Thanks. As this patch has been in trunk for more than a week with no issues I'd like to apply it to the release branches. I've so far bootstrapped and tested it on the GCC 6 branch on aarch64-none-linux-gnu and I'll be applying it there. Kyrill > > Jeff >
diff --git a/gcc/ree.c b/gcc/ree.c index 4ab2ad088c363caaaae8abaad375ec5a78bf13ea..374988e792e27fc342518e8e98af618bc595dbce 100644 --- a/gcc/ree.c +++ b/gcc/ree.c @@ -482,6 +482,14 @@ get_defs (rtx_insn *insn, rtx reg, vec<rtx_insn *> *dest) return NULL; if (DF_REF_INSN_INFO (ref_link->ref) == NULL) return NULL; + /* As global regs are assumed to be defined at each function call + dataflow can report a call_insn as being a definition of REG. + But we can't do anything with that in this pass so proceed only + if the instruction really sets REG in a way that can be deduced + from the RTL structure. */ + if (global_regs[REGNO (reg)] + && !set_of (reg, DF_REF_INSN (ref_link->ref))) + return NULL; } if (dest) @@ -580,7 +588,7 @@ make_defs_and_copies_lists (rtx_insn *extend_insn, const_rtx set_pat, /* Initialize the work list. */ if (!get_defs (extend_insn, src_reg, &state->work_list)) - gcc_unreachable (); + return false; is_insn_visited = XCNEWVEC (bool, max_insn_uid); diff --git a/gcc/testsuite/gcc.target/aarch64/pr78038.c b/gcc/testsuite/gcc.target/aarch64/pr78038.c new file mode 100644 index 0000000000000000000000000000000000000000..76d97d3b0ad44cd75afa1e1e45434413421c5afa --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/pr78038.c @@ -0,0 +1,28 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +/* PR rtl-optimization/78038. + Make sure ree can gracefully handle extensions of the global + variable register after a call. */ + +typedef void (*test_fptr_t) (void); +void +test_f (void) +{ +} +test_fptr_t test_fptr = test_f; + +struct test2_s +{ + int f; +}; + +register struct test2_s *g __asm__("x28"); + +void +do_something () +{ + test_fptr (); + struct test2_s *p1 = 0; + *p1 = *g; +}
Hi all, In this PR we've got code like this: register struct test2_s *g __asm__("x28"); void do_something () { test_fptr (); struct test2_s *p1 = 0; *p1 = *g; } And we get an ICE in gcc/ree.c:655 in get_sub_rtx. The problem is when we try to process the defining insn of register x28 from the insn: (set (reg/f:DI 1 x1 [orig:74 g.1_2 ] [74]) (zero_extend:DI (reg/v:SI 28 x28 [ g ]))) The dataflow reports the insn setting x28 to be: (call_insn 8 7 10 2 (parallel [ (call (mem:DI (reg/f:DI 0 x0 [orig:77 test_fptr ] [77]) [0 *test_fptr.0_1 S8 A8]) (const_int 0 [0])) (use (const_int 0 [0])) (clobber (reg:DI 30 x30)) ]) "ree.c":14 41 {*call_reg} (expr_list:REG_CALL_DECL (nil) (nil)) (expr_list (clobber (reg:DI 17 x17)) (expr_list (clobber (reg:DI 16 x16)) (nil)))) which, as you can see, doesn't actually set x28. AFAICS the reason dataflow reports call_insn 8 as defining x28 is because x28 is a global register variable and that means that every function call is considered to define it. But the ree pass can't really use any of that. It only wants some SET RTL patterns to play with. One solution is to bail out at the ICE location, in get_sub_rtx, when no SETs are found inside the parallel. But I think we shouldn't be getting this far along anyway. So this patch prevents the call_insn from being recognised as a defining insn for x28 for the purposes of ree. It makes sure that if the register we're extending is a global register that all the insns in the def chain actually set it directly as determined by set_of from rtlanal.c. This should hopefully still allow ree to optimise extensions of global registers as long as they are combined with legitimate defining insns. Bootstrapped and tested on aarch64, arm, x86_64. Ok for trunk? Thanks, Kyrill 2016-10-20 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR rtl-optimization/78038 * ree.c (get_defs): Return NULL if a defining insn for REG cannot be deduced to set REG through the RTL structure. (make_defs_and_copies_lists): Return false on a failing get_defs call. 2016-10-20 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR rtl-optimization/78038 * gcc.target/aarch64/pr78038.c: New test.