Patchwork PR target/52555: attribute optimize is overriding command line options

login
register
mail settings
Submitter Jakub Jelinek
Date Feb. 22, 2013, 10:03 a.m.
Message ID <20130222100311.GJ1215@tucnak.zalov.cz>
Download mbox | patch
Permalink /patch/222497/
State New
Headers show

Comments

Jakub Jelinek - Feb. 22, 2013, 10:03 a.m.
On Thu, Feb 21, 2013 at 11:02:56PM +0000, Steve Ellcey wrote:
> Have you gotten any reports of problems with this patch?  It seems to be sending cc1 into an infinite
> loop during the GCC testsuite for me.  I am testing the mips-mti-linux-gnu target and tests like
> gcc.target/mips/call-saved-1.c are causing cc1 to suck up all my memory and swap space before the
> test times out.
> 
> I will keep digging and see if I can figure out what is going on but I wanted to see if anyone else has
> reported this problem.

I think this should fix this (but totally untested except for
call-saved-1.c, and it doesn't make any sense to test on non-mips).

The problem I believe is that Aldy has changed init_optabs and insn-opinit.c
to use this_fn_optabs instead of this_target_optabs, but it is only set in
invoke_set_current_function_hook.  During save_target_globals we want to
init this_target_optabs, so we need to temporarily switch this_fn_optabs to
make that happen.

2013-02-22  Jakub Jelinek  <jakub@redhat.com>

	PR target/52555
	* target-globals.c (save_target_globals): For init_reg_sets and
	target_reinit remporarily set this_fn_optabs to this_target_optabs.


	Jakub
Steve Ellcey - Feb. 22, 2013, 5:31 p.m.
On Fri, 2013-02-22 at 11:03 +0100, Jakub Jelinek wrote:

> The problem I believe is that Aldy has changed init_optabs and insn-opinit.c
> to use this_fn_optabs instead of this_target_optabs, but it is only set in
> invoke_set_current_function_hook.  During save_target_globals we want to
> init this_target_optabs, so we need to temporarily switch this_fn_optabs to
> make that happen.
> 
> 2013-02-22  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/52555
> 	* target-globals.c (save_target_globals): For init_reg_sets and
> 	target_reinit remporarily set this_fn_optabs to this_target_optabs.
> 

Jakub,

I built with this patch and ran the GCC testsuite (using the target
mips-mti-linux-gnu), it fixed the problem and caused no regressions for
me.

Steve Ellcey
sellcey@imgtec.com
Jakub Jelinek - Feb. 22, 2013, 6:17 p.m.
On Fri, Feb 22, 2013 at 09:31:54AM -0800, Steve Ellcey wrote:
> On Fri, 2013-02-22 at 11:03 +0100, Jakub Jelinek wrote:
> 
> > The problem I believe is that Aldy has changed init_optabs and insn-opinit.c
> > to use this_fn_optabs instead of this_target_optabs, but it is only set in
> > invoke_set_current_function_hook.  During save_target_globals we want to
> > init this_target_optabs, so we need to temporarily switch this_fn_optabs to
> > make that happen.
> > 
> > 2013-02-22  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR target/52555
> > 	* target-globals.c (save_target_globals): For init_reg_sets and
> > 	target_reinit remporarily set this_fn_optabs to this_target_optabs.
> > 
> 
> I built with this patch and ran the GCC testsuite (using the target
> mips-mti-linux-gnu), it fixed the problem and caused no regressions for
> me.

Richard, is that ok for trunk then?

	Jakub
Richard Sandiford - Feb. 22, 2013, 7:49 p.m.
Jakub Jelinek <jakub@redhat.com> writes:
> On Fri, Feb 22, 2013 at 09:31:54AM -0800, Steve Ellcey wrote:
>> On Fri, 2013-02-22 at 11:03 +0100, Jakub Jelinek wrote:
>> 
>> > The problem I believe is that Aldy has changed init_optabs and insn-opinit.c
>> > to use this_fn_optabs instead of this_target_optabs, but it is only set in
>> > invoke_set_current_function_hook.  During save_target_globals we want to
>> > init this_target_optabs, so we need to temporarily switch this_fn_optabs to
>> > make that happen.
>> > 
>> > 2013-02-22  Jakub Jelinek  <jakub@redhat.com>
>> > 
>> > 	PR target/52555
>> > 	* target-globals.c (save_target_globals): For init_reg_sets and
>> > 	target_reinit remporarily set this_fn_optabs to this_target_optabs.
>> > 
>> 
>> I built with this patch and ran the GCC testsuite (using the target
>> mips-mti-linux-gnu), it fixed the problem and caused no regressions for
>> me.
>
> Richard, is that ok for trunk then?

Looks good to me.  Thanks for fixing it.

Richard
Steve Ellcey - March 1, 2013, 11:36 p.m.
Jakub and Aldy,

It looks like I am having another problem with this patch.  Jakubs earlier patch fixed things
for me when building my mips-mti-elf target but I just started building glibc in mips16 mode
with the latest compiler and I am running into this assert:

mktime.c:147:1: internal compiler error: in save_optabs_if_changed, at optabs.c:6221
 {
 ^
0x8198e5 save_optabs_if_changed(tree_node*)
	/local/home/sellcey/gcc/linux_mips16/src/gcc/gcc/optabs.c:6221
0x4b090b decl_attributes(tree_node**, tree_node*, int)
	/local/home/sellcey/gcc/linux_mips16/src/gcc/gcc/attribs.c:545
0x4cf728 start_function(c_declspecs*, c_declarator*, tree_node*)
	/local/home/sellcey/gcc/linux_mips16/src/gcc/gcc/c/c-decl.c:7727
0x557114 c_common_parse_file()
	/local/home/sellcey/gcc/linux_mips16/src/gcc/gcc/c-family/c-opts.c:1046

I looked at this_target_optabs and it appears to be a valid pointer but it is not pointing at
default_target_optabs addr and so I get the assert.  I am still trying to dig out more 
information and create a good test case.

Steve Ellcey
sellcey@imgtec.com

Patch

--- gcc/target-globals.c.jj	2013-02-19 07:40:03.000000000 +0100
+++ gcc/target-globals.c	2013-02-22 10:55:36.725435859 +0100
@@ -67,6 +67,7 @@  struct target_globals *
 save_target_globals (void)
 {
   struct target_globals *g;
+  struct target_optabs *saved_this_fn_optabs = this_fn_optabs;
 
   g = ggc_alloc_target_globals ();
   g->flag_state = XCNEW (struct target_flag_state);
@@ -86,8 +87,10 @@  save_target_globals (void)
   g->bb_reorder = XCNEW (struct target_bb_reorder);
   g->lower_subreg = XCNEW (struct target_lower_subreg);
   restore_target_globals (g);
+  this_fn_optabs = this_target_optabs;
   init_reg_sets ();
   target_reinit ();
+  this_fn_optabs = saved_this_fn_optabs;
   return g;
 }