RE: PR target/52555: attribute optimize is overriding command line options
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
sell...@imgtec.com
Re: PR target/52555: attribute optimize is overriding command line options
Jakub Jelinek 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 >> > >> >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
Re: PR target/52555: attribute optimize is overriding command line options
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 > > > > 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
Re: PR target/52555: attribute optimize is overriding command line options
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 > > 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 sell...@imgtec.com
Re: PR target/52555: attribute optimize is overriding command line options
On Thu, Feb 21, 2013 at 11:02:56PM +, 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
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.
--- gcc/target-globals.c.jj 2013-02-19 07:40:03.0 +0100
+++ gcc/target-globals.c2013-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;
}
Jakub
Re: PR target/52555: attribute optimize is overriding command line options
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. No, I have not. 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. Feel free to open a PR and CC me on it. Thanks.
RE: PR target/52555: attribute optimize is overriding command line options
On Mon, Feb 18, 2013 at 12:50:59PM -0600, Aldy Hernandez wrote: > OK pending tests? > PR target/52555 > * genopinit.c (raw_optab_handler): Use this_fn_optabs. > (swap_optab_enable): Same. > (init_all_optabs): Use argument instead of global. > * tree.h (struct tree_optimization_option): New field > target_optabs. > * expr.h (init_all_optabs): Add argument to prototype. > (TREE_OPTIMIZATION_OPTABS): New. > (save_optabs_if_changed): Protoize. > * optabs.h: Declare this_fn_optabs. > * optabs.c (save_optabs_if_changed): New. > Declare this_fn_optabs. > (init_optabs): Add argument to init_all_optabs() call. > * function.c (invoke_set_current_function_hook): Handle per > function optabs. > * function.h (struct function): New field optabs. > * config/mips/mips.c (mips_set_mips16_mode): Handle when > optimization_current_node has changed. > * target-globals.h (save_target_globals_default_opts): Protoize. > * target-globals.c (save_target_globals_default_opts): New. > c/family > PR target/52555 > * c-common.c (handle_optimize_attribute): Call > save_optabs_if_changed. Aldy, 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. Steve Ellcey sell...@imgtec.com
Re: PR target/52555: attribute optimize is overriding command line options
On Mon, Feb 18, 2013 at 12:50:59PM -0600, Aldy Hernandez wrote: > OK pending tests? > PR target/52555 > * genopinit.c (raw_optab_handler): Use this_fn_optabs. > (swap_optab_enable): Same. > (init_all_optabs): Use argument instead of global. > * tree.h (struct tree_optimization_option): New field > target_optabs. > * expr.h (init_all_optabs): Add argument to prototype. > (TREE_OPTIMIZATION_OPTABS): New. > (save_optabs_if_changed): Protoize. > * optabs.h: Declare this_fn_optabs. > * optabs.c (save_optabs_if_changed): New. > Declare this_fn_optabs. > (init_optabs): Add argument to init_all_optabs() call. > * function.c (invoke_set_current_function_hook): Handle per > function optabs. > * function.h (struct function): New field optabs. > * config/mips/mips.c (mips_set_mips16_mode): Handle when > optimization_current_node has changed. > * target-globals.h (save_target_globals_default_opts): Protoize. > * target-globals.c (save_target_globals_default_opts): New. > c/family > PR target/52555 > * c-common.c (handle_optimize_attribute): Call > save_optabs_if_changed. Yes, thanks. Jakub
Re: PR target/52555: attribute optimize is overriding command line options
On 02/16/13 05:19, Richard Sandiford wrote:
Looks good to me otherwise, thanks.
Implemented all your suggestions.
Sorry that SWITCHABLE_TARGETS has been so much hassle. TBH, like Jakub
says in the PR, I was hoping things like the optimize attribute could
use the target_globals stuff too. Today we just care about optabs,
but e.g. arm.c has:
if (TARGET_THUMB1 && optimize_size)
{
/* When optimizing for size on Thumb-1, it's better not
to use the HI regs, because of the overhead of
stacking them. */
for (regno = FIRST_HI_REGNUM;
regno <= LAST_HI_REGNUM; ++regno)
fixed_regs[regno] = call_used_regs[regno] = 1;
}
which affects other cached global state like IRA tables.
...
...
So in the end I think we'll end up trying solve the same problem that
the SWITCHABLE_TARGETS stuff was trying to solve, but this time for
__attribute__((optimize)).
Ughhh... perhaps this can be something for 4.9, reimplementing the
optimize attribute with the target_globals infrastructure?
Regstrapping attached patch.
OK pending tests?
PR target/52555
* genopinit.c (raw_optab_handler): Use this_fn_optabs.
(swap_optab_enable): Same.
(init_all_optabs): Use argument instead of global.
* tree.h (struct tree_optimization_option): New field
target_optabs.
* expr.h (init_all_optabs): Add argument to prototype.
(TREE_OPTIMIZATION_OPTABS): New.
(save_optabs_if_changed): Protoize.
* optabs.h: Declare this_fn_optabs.
* optabs.c (save_optabs_if_changed): New.
Declare this_fn_optabs.
(init_optabs): Add argument to init_all_optabs() call.
* function.c (invoke_set_current_function_hook): Handle per
function optabs.
* function.h (struct function): New field optabs.
* config/mips/mips.c (mips_set_mips16_mode): Handle when
optimization_current_node has changed.
* target-globals.h (save_target_globals_default_opts): Protoize.
* target-globals.c (save_target_globals_default_opts): New.
c/family
PR target/52555
* c-common.c (handle_optimize_attribute): Call
save_optabs_if_changed.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 1e6afaa..a1d47a6 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree
args,
DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
= build_optimization_node ();
+ save_optabs_if_changed (*node);
+
/* Restore current options. */
cl_optimization_restore (&global_options, &cur_opts);
}
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index b203cdd..252e828 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -16313,7 +16313,7 @@ mips_set_mips16_mode (int mips16_p)
if (mips16_p)
{
if (!mips16_globals)
- mips16_globals = save_target_globals ();
+ mips16_globals = save_target_globals_default_opts ();
else
restore_target_globals (mips16_globals);
}
diff --git a/gcc/expr.h b/gcc/expr.h
index f5063b4..15fcb47 100644
--- a/gcc/expr.h
+++ b/gcc/expr.h
@@ -718,7 +718,7 @@ extern bool split_comparison (enum rtx_code, enum
machine_mode,
/* Call this once to initialize the contents of the optabs
appropriately for the current target machine. */
extern void init_optabs (void);
-extern void init_all_optabs (void);
+extern void init_all_optabs (struct target_optabs *);
/* Call this to initialize an optab function entry. */
extern rtx init_one_libfunc (const char *);
diff --git a/gcc/function.c b/gcc/function.c
index 4ce2259..1b41cf2 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4400,6 +4400,26 @@ invoke_set_current_function_hook (tree fndecl)
}
targetm.set_current_function (fndecl);
+
+ if (opts == optimization_default_node)
+ this_fn_optabs = this_target_optabs;
+ else
+ {
+ struct function *fn = DECL_STRUCT_FUNCTION (fndecl);
+ if (fn->optabs == NULL)
+ {
+ if (this_target_optabs == &default_target_optabs)
+ fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
+ else
+ {
+ fn->optabs = (unsigned char *)
+ ggc_alloc_atomic (sizeof (struct target_optabs));
+ init_all_optabs ((struct target_optabs *) fn->optabs);
+ }
+ }
+ this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs
+ : this_target_optabs;
+ }
}
}
diff --git a/gcc/function.h b/gcc/function.h
index 89d71e5..53e28b7 100644
--- a/gcc/function.h
+++ b/gcc/function.h
@@ -580,6 +580,9 @@ struct GTY(()) function {
a string describing the reason for failure. */
const char * GTY((skip)) cannot_be_copied_reason;
+ /* Optabs for this function. T
Re: PR target/52555: attribute optimize is overriding command line options
Aldy Hernandez writes:
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> index b203cdd..5e98485 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -16313,7 +16313,26 @@ mips_set_mips16_mode (int mips16_p)
>if (mips16_p)
> {
>if (!mips16_globals)
> - mips16_globals = save_target_globals ();
> + {
> + if (optimization_current_node != optimization_default_node)
> + {
> + tree opts = optimization_current_node;
> + /* Temporarily switch to the default optimization node,
> + so that *this_target_optabs is set to the default,
> + not reflecting whatever the first mips16 function
> + uses for the optimize attribute. */
> + optimization_current_node = optimization_default_node;
> + cl_optimization_restore
> + (&global_options,
> + TREE_OPTIMIZATION (optimization_default_node));
> + mips16_globals = save_target_globals ();
> + optimization_current_node = opts;
> + cl_optimization_restore (&global_options,
> +TREE_OPTIMIZATION (opts));
> + }
> + else
> + mips16_globals = save_target_globals ();
> + }
I think this should go in target-globals.c, maybe as
save_target_globals_default_opts.
> diff --git a/gcc/function.c b/gcc/function.c
> index 4ce2259..c5eea2e 100644
> --- a/gcc/function.c
> +++ b/gcc/function.c
> @@ -4400,6 +4400,31 @@ invoke_set_current_function_hook (tree fndecl)
> }
>
>targetm.set_current_function (fndecl);
> +
> + if (opts == optimization_default_node)
> + this_fn_optabs = this_target_optabs;
> + else
> + {
> + struct function *fn = DECL_STRUCT_FUNCTION (fndecl);
> + if (fn->optabs == NULL)
> + {
> + if (!SWITCHABLE_TARGET)
> + fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
> + else
> + {
> + if (this_target_optabs == &default_target_optabs)
> + fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
> + else
> + {
> + fn->optabs = (unsigned char *)
> + ggc_alloc_atomic (sizeof (struct target_optabs));
> + init_all_optabs ((struct target_optabs *) fn->optabs);
> + }
> + }
Following on from Jakub's:
if (!SWITCHABLE_TARGET
|| this_target_optabs == &default_target_optabs)
fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
I'd prefer just:
if (this_target_optabs == &default_target_optabs)
fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
Looks good to me otherwise, thanks.
Sorry that SWITCHABLE_TARGETS has been so much hassle. TBH, like Jakub
says in the PR, I was hoping things like the optimize attribute could
use the target_globals stuff too. Today we just care about optabs,
but e.g. arm.c has:
if (TARGET_THUMB1 && optimize_size)
{
/* When optimizing for size on Thumb-1, it's better not
to use the HI regs, because of the overhead of
stacking them. */
for (regno = FIRST_HI_REGNUM;
regno <= LAST_HI_REGNUM; ++regno)
fixed_regs[regno] = call_used_regs[regno] = 1;
}
which affects other cached global state like IRA tables.
The rtx_costs also often depend on optimize_size, and are cached in
various places like expmed.c. E.g. for:
int
foo (int x, int y)
{
return x * 10;
}
the -O2 version on MIPS32 is:
sll $2,$4,1
sll $4,$4,3
j $31
addu$2,$2,$4
and the -Os version is:
li $2,10
j $31
mul $2,$4,$2
But even if an optimize attribute is added:
int __attribute__((optimize(2)))
foo (int x, int y)
{
return x * 10;
}
what you get depends on whether -Os or -O2 was passed on the command line.
The attribute doesn't affect things either way. The same thing happens
on x86_64.
So in the end I think we'll end up trying solve the same problem that
the SWITCHABLE_TARGETS stuff was trying to solve, but this time for
__attribute__((optimize)).
Richard
Re: PR target/52555: attribute optimize is overriding command line options
Looks good, just a few nits. But please wait for Richard's feedback on it.
Will do.
+ this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs
+ : this_target_optabs;
I'd prefer : here be below ? on the line above it.
Blame emacs. I use whatever indentation it decides to use.
+ /* ?? An existing optabs indicates multiple ((optimize))
+attributes for the same function. Is this even valid? For
+now, just clobber the existing entry with the new optabs. */
+ if (TREE_OPTIMIZATION_OPTABS (optnode))
+ XDELETE (TREE_OPTIMIZATION_OPTABS (optnode));
The comment is wrong,
void foo (void) __attribute__((optimize (2)));
void foo (void) __attribute__((optimize ("fast-math")));
void foo (void) __attribute__((optimize ("unroll-loops")));
Oh, that's just sick. Fair enough, I removed the comment altogether.
Plus XDELETE on GC allocated memory is wrong. Just remove the comment
and if and XDELETE lines.
And this is just embarrassing...fixed. Thanks.
Retesting the attached patch.
+2013-02-15 Aldy Hernandez
+ Jakub Jelinek
+
+ PR target/52555
+ * genopinit.c (raw_optab_handler): Use this_fn_optabs.
+ (swap_optab_enable): Same.
+ (init_all_optabs): Use argument instead of global.
+ * tree.h (struct tree_optimization_option): New field
+ target_optabs.
+ * expr.h (init_all_optabs): Add argument to prototype.
+ (TREE_OPTIMIZATION_OPTABS): New.
+ (save_optabs_if_changed): Protoize.
+ * optabs.h: Declare this_fn_optabs.
+ * optabs.c (save_optabs_if_changed): New.
+ Declare this_fn_optabs.
+ (init_optabs): Add argument to init_all_optabs() call.
+ * function.c (invoke_set_current_function_hook): Handle per
+ function optabs.
+ * function.h (struct function): New field optabs.
+ * config/mips/mips.c (mips_set_mips16_mode): Handle when
+ optimization_current_node has changed.
c/family
+ PR target/52555
+ * c-common.c (handle_optimize_attribute): Call
+ save_optabs_if_changed.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 1e6afaa..a1d47a6 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree
args,
DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
= build_optimization_node ();
+ save_optabs_if_changed (*node);
+
/* Restore current options. */
cl_optimization_restore (&global_options, &cur_opts);
}
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index b203cdd..5e98485 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -16313,7 +16313,26 @@ mips_set_mips16_mode (int mips16_p)
if (mips16_p)
{
if (!mips16_globals)
- mips16_globals = save_target_globals ();
+ {
+ if (optimization_current_node != optimization_default_node)
+ {
+ tree opts = optimization_current_node;
+ /* Temporarily switch to the default optimization node,
+so that *this_target_optabs is set to the default,
+not reflecting whatever the first mips16 function
+uses for the optimize attribute. */
+ optimization_current_node = optimization_default_node;
+ cl_optimization_restore
+ (&global_options,
+TREE_OPTIMIZATION (optimization_default_node));
+ mips16_globals = save_target_globals ();
+ optimization_current_node = opts;
+ cl_optimization_restore (&global_options,
+ TREE_OPTIMIZATION (opts));
+ }
+ else
+ mips16_globals = save_target_globals ();
+ }
else
restore_target_globals (mips16_globals);
}
diff --git a/gcc/expr.h b/gcc/expr.h
index f5063b4..15fcb47 100644
--- a/gcc/expr.h
+++ b/gcc/expr.h
@@ -718,7 +718,7 @@ extern bool split_comparison (enum rtx_code, enum
machine_mode,
/* Call this once to initialize the contents of the optabs
appropriately for the current target machine. */
extern void init_optabs (void);
-extern void init_all_optabs (void);
+extern void init_all_optabs (struct target_optabs *);
/* Call this to initialize an optab function entry. */
extern rtx init_one_libfunc (const char *);
diff --git a/gcc/function.c b/gcc/function.c
index 4ce2259..c87f62d 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4400,6 +4400,27 @@ invoke_set_current_function_hook (tree fndecl)
}
targetm.set_current_function (fndecl);
+
+ if (opts == optimization_default_node)
+ this_fn_optabs = this_target_optabs;
+ else
+ {
+ struct function *fn = DECL_STRUCT_FUNCTION (fndecl);
+ if (fn->optabs == NULL)
+ {
+ if (!SWITCHABLE_TARGET
+ || this_target_optabs == &default_target_
Re: PR target/52555: attribute optimize is overriding command line options
On Fri, Feb 15, 2013 at 11:23:01AM -0600, Aldy Hernandez wrote:
> +2013-02-15 Aldy Hernandez
> + Jakub Jelinek
> +
> + PR target/52555
> + * genopinit.c (raw_optab_handler): Use this_fn_optabs.
> + (swap_optab_enable): Same.
> + (init_all_optabs): Use argument instead of global.
> + * tree.h (struct tree_optimization_option): New field
> + target_optabs.
> + * expr.h (init_all_optabs): Add argument to prototype.
> + (TREE_OPTIMIZATION_OPTABS): New.
> + (save_optabs_if_changed): Protoize.
> + * optabs.h: Declare this_fn_optabs.
> + * optabs.c (save_optabs_if_changed): New.
> + Declare this_fn_optabs.
> + (init_optabs): Add argument to init_all_optabs() call.
> + * function.c (invoke_set_current_function_hook): Handle per
> + function optabs.
> + * function.h (struct function): New field optabs.
> + * config/mips/mips.c (mips_set_mips16_mode): Handle when
> + optimization_current_node has changed.
> c/family
> + PR target/52555
> + * c-common.c (handle_optimize_attribute): Call
> + save_optabs_if_changed.
Looks good, just a few nits. But please wait for Richard's feedback on it.
> + if (!SWITCHABLE_TARGET)
> + fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
> + else
> + {
> + if (this_target_optabs == &default_target_optabs)
> + fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
Just use
if (!SWITCHABLE_TARGET
|| this_target_optabs == &default_target_optabs)
fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
> + else
> + {
> + fn->optabs = (unsigned char *)
> + ggc_alloc_atomic (sizeof (struct target_optabs));
> + init_all_optabs ((struct target_optabs *) fn->optabs);
> + }
> + }
and reindent.
> + }
> + this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs
> + : this_target_optabs;
I'd prefer : here be below ? on the line above it.
> + /* ?? An existing optabs indicates multiple ((optimize))
> + attributes for the same function. Is this even valid? For
> + now, just clobber the existing entry with the new optabs. */
> + if (TREE_OPTIMIZATION_OPTABS (optnode))
> + XDELETE (TREE_OPTIMIZATION_OPTABS (optnode));
The comment is wrong,
void foo (void) __attribute__((optimize (2)));
void foo (void) __attribute__((optimize ("fast-math")));
void foo (void) __attribute__((optimize ("unroll-loops")));
void
foo (void)
{
}
is just fine, and for foo results in -O2 -ffast-math -funroll-loops
options being in effect.
Plus XDELETE on GC allocated memory is wrong. Just remove the comment
and if and XDELETE lines.
Jakub
Re: PR target/52555: attribute optimize is overriding command line options
Actually, thinking more about SWITCHABLE_TARGETS, I don't think this works
at all on those targets. this_target_optab is some random optab from
whatever has been left there by previous function, it can be either the
Eeech... this significantly complicates things.
After some further interaction off-list... attached is a patch using
cfun for SWITCHABLE_TARGETs, caching the default optimization node, and
using GC for the pointers.
Jakub, I got rid of the suggested ifdef's because I'd prefer the
compiler to compile/stress the non-common SWITCHABLE_TARGET case. All
targets define SWITCHABLE_TARGET to 0/1, so there should never be an
undefined symbol. If you'd prefer the ifdef, I can grudgingly change it
back :).
[Richard, unfortunately there are some mips specific changes.]
How does this look y'all?
+2013-02-15 Aldy Hernandez
+ Jakub Jelinek
+
+ PR target/52555
+ * genopinit.c (raw_optab_handler): Use this_fn_optabs.
+ (swap_optab_enable): Same.
+ (init_all_optabs): Use argument instead of global.
+ * tree.h (struct tree_optimization_option): New field
+ target_optabs.
+ * expr.h (init_all_optabs): Add argument to prototype.
+ (TREE_OPTIMIZATION_OPTABS): New.
+ (save_optabs_if_changed): Protoize.
+ * optabs.h: Declare this_fn_optabs.
+ * optabs.c (save_optabs_if_changed): New.
+ Declare this_fn_optabs.
+ (init_optabs): Add argument to init_all_optabs() call.
+ * function.c (invoke_set_current_function_hook): Handle per
+ function optabs.
+ * function.h (struct function): New field optabs.
+ * config/mips/mips.c (mips_set_mips16_mode): Handle when
+ optimization_current_node has changed.
c/family
+ PR target/52555
+ * c-common.c (handle_optimize_attribute): Call
+ save_optabs_if_changed.
+
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 1e6afaa..a1d47a6 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree
args,
DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
= build_optimization_node ();
+ save_optabs_if_changed (*node);
+
/* Restore current options. */
cl_optimization_restore (&global_options, &cur_opts);
}
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index b203cdd..5e98485 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -16313,7 +16313,26 @@ mips_set_mips16_mode (int mips16_p)
if (mips16_p)
{
if (!mips16_globals)
- mips16_globals = save_target_globals ();
+ {
+ if (optimization_current_node != optimization_default_node)
+ {
+ tree opts = optimization_current_node;
+ /* Temporarily switch to the default optimization node,
+so that *this_target_optabs is set to the default,
+not reflecting whatever the first mips16 function
+uses for the optimize attribute. */
+ optimization_current_node = optimization_default_node;
+ cl_optimization_restore
+ (&global_options,
+TREE_OPTIMIZATION (optimization_default_node));
+ mips16_globals = save_target_globals ();
+ optimization_current_node = opts;
+ cl_optimization_restore (&global_options,
+ TREE_OPTIMIZATION (opts));
+ }
+ else
+ mips16_globals = save_target_globals ();
+ }
else
restore_target_globals (mips16_globals);
}
diff --git a/gcc/expr.h b/gcc/expr.h
index f5063b4..15fcb47 100644
--- a/gcc/expr.h
+++ b/gcc/expr.h
@@ -718,7 +718,7 @@ extern bool split_comparison (enum rtx_code, enum
machine_mode,
/* Call this once to initialize the contents of the optabs
appropriately for the current target machine. */
extern void init_optabs (void);
-extern void init_all_optabs (void);
+extern void init_all_optabs (struct target_optabs *);
/* Call this to initialize an optab function entry. */
extern rtx init_one_libfunc (const char *);
diff --git a/gcc/function.c b/gcc/function.c
index 4ce2259..c5eea2e 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4400,6 +4400,31 @@ invoke_set_current_function_hook (tree fndecl)
}
targetm.set_current_function (fndecl);
+
+ if (opts == optimization_default_node)
+ this_fn_optabs = this_target_optabs;
+ else
+ {
+ struct function *fn = DECL_STRUCT_FUNCTION (fndecl);
+ if (fn->optabs == NULL)
+ {
+ if (!SWITCHABLE_TARGET)
+ fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
+ else
+ {
+ if (this_target_optabs == &default_target_optabs)
+ fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
+ else
+ {
+
Re: PR target/52555: attribute optimize is overriding command line options
On Wed, Feb 13, 2013 at 12:07:52PM -0600, Aldy Hernandez wrote: > >Sorry, just noticed: > > > >>+ /* If the optabs changed, record it in the node. */ > >>+ if (memcmp (tmp_target_optabs, &default_target_optabs, > >>+ sizeof (struct target_optabs))) > > > >This should be this_target_optabs rather than &default_target_optabs. > >Nothing but target code and initialisers should use &default_target_optabs > >directly. > > Fixed. > > > > >I don't think that needs a retest though. It only makes a difference > >on MIPS. > > I verified that the testcase is still fixed by this patch (just in > case). Thanks so much for the review. > > Final patch attached. Actually, thinking more about SWITCHABLE_TARGETS, I don't think this works at all on those targets. this_target_optab is some random optab from whatever has been left there by previous function, it can be either the same, or different target (mips vs. mips16?). So, for SWITCHABLE_TARGETS, I'm afraid you really can't save the optabs in the optimization node (because, there could be different optabs for say all of -Ofast + mips, -Ofast + mips16, -O2 + mips and -O2 + mips16 combinations), but you probably can't save it even from within the target hook, because some non-default optimization node might be in effect. So, for SWITCHABLE_TARGETS, I think you can only save it in cfun->this_fn_optab or similar, and perhaps cache the default for mips16 only when you know the default optimization node is in effect (or force it into effect temporarily). For !SWITCHABLE_TARGETS, you can easily remember it in the optimization nodes and just copy the pointer over to cfun->this_fn_optab. And unfortunately it seems optimize attribute is supported everywhere, just the target attribute is not. But, as soon as it is reachable from cfun->this_fn_optab, we might need to GC allocate it, while optimization nodes are never freed, cfun is freed I think. Jakub
Re: PR target/52555: attribute optimize is overriding command line options
Sorry, just noticed:
+ /* If the optabs changed, record it in the node. */
+ if (memcmp (tmp_target_optabs, &default_target_optabs,
+ sizeof (struct target_optabs)))
This should be this_target_optabs rather than &default_target_optabs.
Nothing but target code and initialisers should use &default_target_optabs
directly.
Fixed.
I don't think that needs a retest though. It only makes a difference
on MIPS.
I verified that the testcase is still fixed by this patch (just in
case). Thanks so much for the review.
Final patch attached.
Jakub, OK?
+ PR target/52555
+ * genopinit.c (main): Use this_fn_optabs in generated
+ init_all_optabs, raw_optab_handler, and swap_optab_enable.
+ * tree.h (struct tree_optimization_option): New field
+ target_optabs.
+ (TREE_OPTIMIZATION_OPTABS): New.
+ (save_optabs_if_changed): Protoize.
+ * optabs.h: Declare this_fn_optabs.
+ * optabs.c (save_optabs_if_changed): New.
+ Declare this_fn_optabs.
+ * function.c (invoke_set_current_function_hook): Set
+ this_fn_optabs if there is one in the optimization node.
c-family/
+ PR target/52555
+ * c-common.c (handle_optimize_attribute): Call
+ save_optabs_if_changed.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 1e6afaa..3711e69 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree
args,
DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
= build_optimization_node ();
+ save_optabs_if_changed (DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node));
+
/* Restore current options. */
cl_optimization_restore (&global_options, &cur_opts);
}
diff --git a/gcc/function.c b/gcc/function.c
index 4ce2259..688242a 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4383,6 +4383,7 @@ static bool in_dummy_function;
static void
invoke_set_current_function_hook (tree fndecl)
{
+ this_fn_optabs = this_target_optabs;
if (!in_dummy_function)
{
tree opts = ((fndecl)
@@ -4397,6 +4398,11 @@ invoke_set_current_function_hook (tree fndecl)
{
optimization_current_node = opts;
cl_optimization_restore (&global_options, TREE_OPTIMIZATION (opts));
+
+ /* Change optabs if needed. */
+ if (TREE_OPTIMIZATION_OPTABS (opts))
+ this_fn_optabs
+ = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts);
}
targetm.set_current_function (fndecl);
diff --git a/gcc/genopinit.c b/gcc/genopinit.c
index 1bb2f77..13ebdc5 100644
--- a/gcc/genopinit.c
+++ b/gcc/genopinit.c
@@ -423,7 +423,7 @@ main (int argc, char **argv)
fprintf (s_file, "};\n\n");
fprintf (s_file, "void\ninit_all_optabs (void)\n{\n");
- fprintf (s_file, " bool *ena = this_target_optabs->pat_enable;\n");
+ fprintf (s_file, " bool *ena = this_fn_optabs->pat_enable;\n");
for (i = 0; patterns.iterate (i, &p); ++i)
fprintf (s_file, " ena[%u] = HAVE_%s;\n", i, p->name);
fprintf (s_file, "}\n\n");
@@ -456,7 +456,7 @@ main (int argc, char **argv)
"raw_optab_handler (unsigned scode)\n"
"{\n"
" int i = lookup_handler (scode);\n"
- " return (i >= 0 && this_target_optabs->pat_enable[i]\n"
+ " return (i >= 0 && this_fn_optabs->pat_enable[i]\n"
" ? pats[i].icode : CODE_FOR_nothing);\n"
"}\n\n");
@@ -468,8 +468,8 @@ main (int argc, char **argv)
" int i = lookup_handler (scode);\n"
" if (i >= 0)\n"
"{\n"
- " bool ret = this_target_optabs->pat_enable[i];\n"
- " this_target_optabs->pat_enable[i] = set;\n"
+ " bool ret = this_fn_optabs->pat_enable[i];\n"
+ " this_fn_optabs->pat_enable[i] = set;\n"
" return ret;\n"
"}\n"
" else\n"
diff --git a/gcc/optabs.c b/gcc/optabs.c
index c1dacf4..00a13da 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. If not see
struct target_optabs default_target_optabs;
struct target_libfuncs default_target_libfuncs;
+struct target_optabs *this_fn_optabs = &default_target_optabs;
#if SWITCHABLE_TARGET
struct target_optabs *this_target_optabs = &default_target_optabs;
struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs;
@@ -6207,6 +6208,40 @@ init_optabs (void)
targetm.init_libfuncs ();
}
+/* Recompute the optabs. If they have changed, save the new set of
+ optabs in the optimization node OPTNODE. */
+
+void
+save_optabs_if_changed (tree optnode)
+{
+ struct target_optabs *save_fn_optabs = this_fn_optabs;
+ struct target_optabs *tmp_target_optabs = XCNEW (struct target_optabs);
+
+ /* Generate a new set of optabs into tmp_target_optabs. */
+ this_fn_optabs = tmp_target_optabs;
+ init_all_optabs ();
+ this_fn_opta
Re: PR target/52555: attribute optimize is overriding command line options
Aldy Hernandez writes: > Richard. > > I made all the changes you suggested. > > I also changed other instances of s/this_target_optabs/this_fn_optabs/ > which I forgot in the previous iteration. And I also changed > save_optabs_if_changed() to use this_fn_optabs, since init_all_optabs() > will generate the optabs into this_fn_optabs. Sorry, just noticed: > + /* If the optabs changed, record it in the node. */ > + if (memcmp (tmp_target_optabs, &default_target_optabs, > + sizeof (struct target_optabs))) This should be this_target_optabs rather than &default_target_optabs. Nothing but target code and initialisers should use &default_target_optabs directly. I don't think that needs a retest though. It only makes a difference on MIPS. Looks good to me otherwise. If there's any fallout on MIPS (hopefully not) I'll fix it up after the commit. Thanks, Richard
Re: PR target/52555: attribute optimize is overriding command line options
Richard.
I made all the changes you suggested.
I also changed other instances of s/this_target_optabs/this_fn_optabs/
which I forgot in the previous iteration. And I also changed
save_optabs_if_changed() to use this_fn_optabs, since init_all_optabs()
will generate the optabs into this_fn_optabs.
Tested on x86-64 Linux.
OK?
+ PR target/52555
+ * genopinit.c (main): Use this_fn_optabs in generated
+ init_all_optabs, raw_optab_handler, and swap_optab_enable.
+ * tree.h (struct tree_optimization_option): New field
+ target_optabs.
+ (TREE_OPTIMIZATION_OPTABS): New.
+ (save_optabs_if_changed): Protoize.
+ * optabs.h: Declare this_fn_optabs.
+ * optabs.c (save_optabs_if_changed): New.
+ Declare this_fn_optabs.
+ * function.c (invoke_set_current_function_hook): Set
+ this_fn_optabs if there is one in the optimization node.
c-family/
+ PR target/52555
+ * c-common.c (handle_optimize_attribute): Call
+ save_optabs_if_changed.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 1e6afaa..3711e69 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree
args,
DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
= build_optimization_node ();
+ save_optabs_if_changed (DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node));
+
/* Restore current options. */
cl_optimization_restore (&global_options, &cur_opts);
}
diff --git a/gcc/function.c b/gcc/function.c
index 4ce2259..688242a 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4383,6 +4383,7 @@ static bool in_dummy_function;
static void
invoke_set_current_function_hook (tree fndecl)
{
+ this_fn_optabs = this_target_optabs;
if (!in_dummy_function)
{
tree opts = ((fndecl)
@@ -4397,6 +4398,11 @@ invoke_set_current_function_hook (tree fndecl)
{
optimization_current_node = opts;
cl_optimization_restore (&global_options, TREE_OPTIMIZATION (opts));
+
+ /* Change optabs if needed. */
+ if (TREE_OPTIMIZATION_OPTABS (opts))
+ this_fn_optabs
+ = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts);
}
targetm.set_current_function (fndecl);
diff --git a/gcc/genopinit.c b/gcc/genopinit.c
index 1bb2f77..13ebdc5 100644
--- a/gcc/genopinit.c
+++ b/gcc/genopinit.c
@@ -423,7 +423,7 @@ main (int argc, char **argv)
fprintf (s_file, "};\n\n");
fprintf (s_file, "void\ninit_all_optabs (void)\n{\n");
- fprintf (s_file, " bool *ena = this_target_optabs->pat_enable;\n");
+ fprintf (s_file, " bool *ena = this_fn_optabs->pat_enable;\n");
for (i = 0; patterns.iterate (i, &p); ++i)
fprintf (s_file, " ena[%u] = HAVE_%s;\n", i, p->name);
fprintf (s_file, "}\n\n");
@@ -456,7 +456,7 @@ main (int argc, char **argv)
"raw_optab_handler (unsigned scode)\n"
"{\n"
" int i = lookup_handler (scode);\n"
- " return (i >= 0 && this_target_optabs->pat_enable[i]\n"
+ " return (i >= 0 && this_fn_optabs->pat_enable[i]\n"
" ? pats[i].icode : CODE_FOR_nothing);\n"
"}\n\n");
@@ -468,8 +468,8 @@ main (int argc, char **argv)
" int i = lookup_handler (scode);\n"
" if (i >= 0)\n"
"{\n"
- " bool ret = this_target_optabs->pat_enable[i];\n"
- " this_target_optabs->pat_enable[i] = set;\n"
+ " bool ret = this_fn_optabs->pat_enable[i];\n"
+ " this_fn_optabs->pat_enable[i] = set;\n"
" return ret;\n"
"}\n"
" else\n"
diff --git a/gcc/optabs.c b/gcc/optabs.c
index c1dacf4..145930f 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. If not see
struct target_optabs default_target_optabs;
struct target_libfuncs default_target_libfuncs;
+struct target_optabs *this_fn_optabs = &default_target_optabs;
#if SWITCHABLE_TARGET
struct target_optabs *this_target_optabs = &default_target_optabs;
struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs;
@@ -6207,6 +6208,40 @@ init_optabs (void)
targetm.init_libfuncs ();
}
+/* Recompute the optabs. If they have changed, save the new set of
+ optabs in the optimization node OPTNODE. */
+
+void
+save_optabs_if_changed (tree optnode)
+{
+ struct target_optabs *save_fn_optabs = this_fn_optabs;
+ struct target_optabs *tmp_target_optabs = XCNEW (struct target_optabs);
+
+ /* Generate a new set of optabs into tmp_target_optabs. */
+ this_fn_optabs = tmp_target_optabs;
+ init_all_optabs ();
+ this_fn_optabs = save_fn_optabs;
+
+ /* If the optabs changed, record it in the node. */
+ if (memcmp (tmp_target_optabs, &default_target_optabs,
+ sizeof (struct target_optabs)))
+{
+ /* ?? An existing entry in TREE_OPTIMIZATION_OPT
Re: PR target/52555: attribute optimize is overriding command line options
Aldy Hernandez writes:
>> Rather than:
>>
>>/* Change optabs if needed. */
>>if (TREE_OPTIMIZATION_OPTABS (opts))
>> this_target_optabs
>>= (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts);
>>else
>> this_target_optabs = &default_target_optabs;
>>
>> I think it'd be better to have:
>>
>>/* Change optabs if needed. */
>>if (TREE_OPTIMIZATION_OPTABS (opts))
>> this_fn_optabs
>>= (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts);
>>else
>> this_fn_optabs = this_target_optabs;
>>
>> with genopinit.c updated to use this_fn_optabs instead of this_target_optabs.
>
> Hmmm, ok.
>
> I also added a default case setting this_fn_optabs =
> &default_target_optabs when the optimizations haven't changed. I can
> remove this if redundant.
No, sounds like a good plan, but I think it should be this_target_optabs
rather than &default_target_optabs. Also:
@@ -76,11 +76,8 @@ struct target_optabs {
};
extern struct target_optabs default_target_optabs;
-#if SWITCHABLE_TARGET
extern struct target_optabs *this_target_optabs;
-#else
-#define this_target_optabs (&default_target_optabs)
-#endif
This shouldn't be needed now, and:
@@ -44,8 +44,9 @@ along with GCC; see the file COPYING3. If not see
struct target_optabs default_target_optabs;
struct target_libfuncs default_target_libfuncs;
-#if SWITCHABLE_TARGET
struct target_optabs *this_target_optabs = &default_target_optabs;
+struct target_optabs *this_fn_optabs = &default_target_optabs;
+#if SWITCHABLE_TARGET
struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs;
#endif
I think this should be:
struct target_optabs default_target_optabs;
struct target_libfuncs default_target_libfuncs;
+struct target_optabs *this_fn_optabs = &default_target_optabs;
#if SWITCHABLE_TARGET
struct target_optabs *this_target_optabs = &default_target_optabs;
struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs;
#endif
Looks good to me otherwise as far as switchable targets go.
Richard
Re: PR target/52555: attribute optimize is overriding command line options
Richard Sandiford writes:
> Aldy Hernandez writes:
>>> Rather than:
>>>
>>> /* Change optabs if needed. */
>>> if (TREE_OPTIMIZATION_OPTABS (opts))
>>> this_target_optabs
>>> = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts);
>>> else
>>> this_target_optabs = &default_target_optabs;
>>>
>>> I think it'd be better to have:
>>>
>>> /* Change optabs if needed. */
>>> if (TREE_OPTIMIZATION_OPTABS (opts))
>>> this_fn_optabs
>>> = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts);
>>> else
>>> this_fn_optabs = this_target_optabs;
>>>
>>> with genopinit.c updated to use this_fn_optabs instead of
>>> this_target_optabs.
>>
>> Hmmm, ok.
>>
>> I also added a default case setting this_fn_optabs =
>> &default_target_optabs when the optimizations haven't changed. I can
>> remove this if redundant.
>
> No, sounds like a good plan, but I think it should be this_target_optabs
> rather than &default_target_optabs.
Gah, just realised after sending that it would be better to have:
static void
invoke_set_current_function_hook (tree fndecl)
{
this_fn_optabs = this_target_optabs;
if (!in_dummy_function)
{
tree opts = ((fndecl)
? DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl)
: optimization_default_node);
if (!opts)
opts = optimization_default_node;
/* Change optimization options if needed. */
if (optimization_current_node != opts)
{
optimization_current_node = opts;
cl_optimization_restore (&global_options, TREE_OPTIMIZATION (opts));
/* Change optabs if needed. */
if (TREE_OPTIMIZATION_OPTABS (opts))
this_fn_optabs
= (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts);
}
targetm.set_current_function (fndecl);
}
}
Richard
Re: PR target/52555: attribute optimize is overriding command line options
Rather than:
/* Change optabs if needed. */
if (TREE_OPTIMIZATION_OPTABS (opts))
this_target_optabs
= (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts);
else
this_target_optabs = &default_target_optabs;
I think it'd be better to have:
/* Change optabs if needed. */
if (TREE_OPTIMIZATION_OPTABS (opts))
this_fn_optabs
= (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts);
else
this_fn_optabs = this_target_optabs;
with genopinit.c updated to use this_fn_optabs instead of this_target_optabs.
Hmmm, ok.
I also added a default case setting this_fn_optabs =
&default_target_optabs when the optimizations haven't changed. I can
remove this if redundant.
Jakub also recommended bailing if TREE_OPTIMIZATION_OPTABS already set,
thus avoiding recomputing init_all_optabs() in save_optabs_if_changed:
+ if (TREE_OPTIMIZATION_OPTABS (optnode))
+return;
Is this still part of the plan?
How is this revision?
commit 48c1f4d243f81ca975b2aae34773b91ef6cbd8ca
Author: Aldy Hernandez
Date: Mon Feb 11 15:51:24 2013 -0600
PR target/52555
* genopinit.c (main): Use this_fn_optabs in generated
init_all_optabs.
* tree.h (struct tree_optimization_option): New field
target_optabs.
(TREE_OPTIMIZATION_OPTABS): New.
(save_optabs_if_changed): Protoize.
* optabs.h: Always declare this_target_optabs.
Declare this_fn_optabs.
* optabs.c (save_optabs_if_changed): New.
Always declare this_target_optabs.
Declare this_fn_optabs.
* function.c (invoke_set_current_function_hook): Set
this_fn_optabs if there is one in the optimization node.
c-family/
* c-common.c (handle_optimize_attribute): Call
save_optabs_if_changed.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 1e6afaa..3711e69 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree
args,
DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
= build_optimization_node ();
+ save_optabs_if_changed (DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node));
+
/* Restore current options. */
cl_optimization_restore (&global_options, &cur_opts);
}
diff --git a/gcc/function.c b/gcc/function.c
index 4ce2259..b177a98 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4397,7 +4397,16 @@ invoke_set_current_function_hook (tree fndecl)
{
optimization_current_node = opts;
cl_optimization_restore (&global_options, TREE_OPTIMIZATION (opts));
+
+ /* Change optabs if needed. */
+ if (TREE_OPTIMIZATION_OPTABS (opts))
+ this_fn_optabs
+ = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts);
+ else
+ this_fn_optabs = this_target_optabs;
}
+ else
+ this_fn_optabs = &default_target_optabs;
targetm.set_current_function (fndecl);
}
diff --git a/gcc/genopinit.c b/gcc/genopinit.c
index 1bb2f77..2a4b3e2 100644
--- a/gcc/genopinit.c
+++ b/gcc/genopinit.c
@@ -423,7 +423,7 @@ main (int argc, char **argv)
fprintf (s_file, "};\n\n");
fprintf (s_file, "void\ninit_all_optabs (void)\n{\n");
- fprintf (s_file, " bool *ena = this_target_optabs->pat_enable;\n");
+ fprintf (s_file, " bool *ena = this_fn_optabs->pat_enable;\n");
for (i = 0; patterns.iterate (i, &p); ++i)
fprintf (s_file, " ena[%u] = HAVE_%s;\n", i, p->name);
fprintf (s_file, "}\n\n");
diff --git a/gcc/optabs.c b/gcc/optabs.c
index c1dacf4..e40bb5f 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -44,8 +44,9 @@ along with GCC; see the file COPYING3. If not see
struct target_optabs default_target_optabs;
struct target_libfuncs default_target_libfuncs;
-#if SWITCHABLE_TARGET
struct target_optabs *this_target_optabs = &default_target_optabs;
+struct target_optabs *this_fn_optabs = &default_target_optabs;
+#if SWITCHABLE_TARGET
struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs;
#endif
@@ -6207,6 +6208,40 @@ init_optabs (void)
targetm.init_libfuncs ();
}
+/* Recompute the optabs. If they have changed, save the new set of
+ optabs in the optimization node OPTNODE. */
+
+void
+save_optabs_if_changed (tree optnode)
+{
+ struct target_optabs *save_target_optabs = this_target_optabs;
+ struct target_optabs *tmp_target_optabs = XCNEW (struct target_optabs);
+
+ /* Generate a new set of optabs into tmp_target_optabs. */
+ this_target_optabs = tmp_target_optabs;
+ init_all_optabs ();
+ this_target_optabs = save_target_optabs;
+
+ /* If the optabs changed, record it in the node. */
+ if (memcmp (tmp_target_optabs, &default_target_optabs,
+ sizeof (struct target_optabs)))
+{
+ /* ?? An existing entry in TREE_OPTIMIZATION_OPTABS indicates
+
Re: PR target/52555: attribute optimize is overriding command line options
Jakub Jelinek writes: > On Mon, Feb 11, 2013 at 06:15:05PM -0600, Aldy Hernandez wrote: >> How does this look? > > Looks good to me. > >> Jakub, what's this you mention in the PR about caching >> __optimize__((3))? You also mention I shouldn't compare against >> this_target_optabs, but default_target_optabs. But what if >> this_target_optabs has changed? (See patch). > > The reason for that is that this_target_optabs could at that point be > simply whatever optabs used the last parsed function. > this_target_optabs changes only either because of optimize attribute > (not sure if MIPS as the only switchable target? supports that), or > because of mips_set_mips16_mode. I think invoke_set_current_function_hook > invokes the target hook after the code you've changed, so I'd say it should > work fine even on MIPS. CCing Richard for that anyway. The target hook won't do anything for consecutive functions that have the same mode though. It expects this_target_optabs (and other this_target_* stuff) to stay the same. Rather than: /* Change optabs if needed. */ if (TREE_OPTIMIZATION_OPTABS (opts)) this_target_optabs = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); else this_target_optabs = &default_target_optabs; I think it'd be better to have: /* Change optabs if needed. */ if (TREE_OPTIMIZATION_OPTABS (opts)) this_fn_optabs = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); else this_fn_optabs = this_target_optabs; with genopinit.c updated to use this_fn_optabs instead of this_target_optabs. Richard
Re: PR target/52555: attribute optimize is overriding command line options
On Tue, Feb 12, 2013 at 09:58:38AM -0600, Aldy Hernandez wrote:
> OK for trunk?
I'd still prefer Richard to chime in, I'm really not familiar enough
with MIPS switchable target stuff.
> +/* Recompute the optabs. If they have changed, save the new set of
> + optabs in the optimization node OPTNODE. */
> +
> +void
> +save_optabs_if_changed (tree optnode)
> +{
> + struct target_optabs *save_target_optabs = this_target_optabs;
> + struct target_optabs *tmp_target_optabs = XCNEW (struct target_optabs);
> +
> + /* Generate a new set of optabs into tmp_target_optabs. */
> + this_target_optabs = tmp_target_optabs;
> + init_all_optabs ();
> + this_target_optabs = save_target_optabs;
> +
> + /* If the optabs changed, record it in the node. */
> + if (memcmp (tmp_target_optabs, &default_target_optabs,
> + sizeof (struct target_optabs)))
> +{
> + /* ?? An existing entry in TREE_OPTIMIZATION_OPTABS indicates
> + multiple ((optimize)) attributes for the same function. Is
> + this even valid? For now, just clobber the existing entry
> + with the new optabs. */
> + if (TREE_OPTIMIZATION_OPTABS (optnode))
> + XDELETE (TREE_OPTIMIZATION_OPTABS (optnode));
I wonder if this necessarily won't mean that if TREE_OPTIMIZATION_OPTABS
is non-NULL, then memcmp (tmp_target_optabs, TREE_OPTIMIZATION_OPTABS
(optnode), sizeof (*tmp_target_optabs)) == 0. Because, optimization nodes
are only shared if they contain the same set of all options.
Multiple optimize attributes seems to be valid, see what
handle_optimize_attribute says on them, but they together either create
a fresh optimization node which certainly won't have
TREE_OPTIMIZATION_OPTABS set, or they return one older, but that should be
already initialized to the same thing.
So perhaps start the function with
if (TREE_OPTIMIZATION_OPTABS (optnode))
return;
? I.e., if you have multiple functions with the same optimization node,
there is no need to do init_all_optabs again and again.
Perhaps we want to have a flag whether TREE_OPTIMIZATION_OPTABS has been
computed already, and don't call save_optabs_if_changed if already set,
or add some magic value for TREE_OPTIMIZATION_OPTABS, where e.g. NULL
would mean not computed yet, the special magic value would mean the default
and other values the special optabs? Perhaps the magic value could be
just &default_target_optabs...
Jakub
Re: PR target/52555: attribute optimize is overriding command line options
Jakub, what's this you mention in the PR about caching
__optimize__((3))? You also mention I shouldn't compare against
this_target_optabs, but default_target_optabs. But what if
this_target_optabs has changed? (See patch).
The reason for that is that this_target_optabs could at that point be
simply whatever optabs used the last parsed function.
this_target_optabs changes only either because of optimize attribute
(not sure if MIPS as the only switchable target? supports that), or
because of mips_set_mips16_mode. I think invoke_set_current_function_hook
invokes the target hook after the code you've changed, so I'd say it should
work fine even on MIPS. CCing Richard for that anyway.
Ok, fixed.
I think you should just use XCNEW and drop the memset.
Perfect. Done.
Shouldn't this (and above) be XDELETE to match the allocation style?
Absolutely. Done.
OK for trunk?
commit ee1f2aebe23fe0d5cecfdfde9822ef681bf6ef0c
Author: Aldy Hernandez
Date: Mon Feb 11 15:51:24 2013 -0600
PR target/52555
* tree.h (struct tree_optimization_option): New field
target_optabs.
(TREE_OPTIMIZATION_OPTABS): New.
(save_optabs_if_changed): Protoize.
* optabs.h: Always declare this_target_optabs.
* optabs.c (save_optabs_if_changed): New.
Always declare this_target_optabs.
* function.c (invoke_set_current_function_hook): Set
this_target_optabs if there is one in the optimization node.
c-family/
* c-common.c (handle_optimize_attribute): Call
save_optabs_if_changed.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 1e6afaa..3711e69 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree
args,
DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
= build_optimization_node ();
+ save_optabs_if_changed (DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node));
+
/* Restore current options. */
cl_optimization_restore (&global_options, &cur_opts);
}
diff --git a/gcc/function.c b/gcc/function.c
index 4ce2259..f37e91f 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4397,6 +4397,13 @@ invoke_set_current_function_hook (tree fndecl)
{
optimization_current_node = opts;
cl_optimization_restore (&global_options, TREE_OPTIMIZATION (opts));
+
+ /* Change optabs if needed. */
+ if (TREE_OPTIMIZATION_OPTABS (opts))
+ this_target_optabs
+ = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts);
+ else
+ this_target_optabs = &default_target_optabs;
}
targetm.set_current_function (fndecl);
diff --git a/gcc/optabs.c b/gcc/optabs.c
index c1dacf4..11153aa 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -44,8 +44,8 @@ along with GCC; see the file COPYING3. If not see
struct target_optabs default_target_optabs;
struct target_libfuncs default_target_libfuncs;
-#if SWITCHABLE_TARGET
struct target_optabs *this_target_optabs = &default_target_optabs;
+#if SWITCHABLE_TARGET
struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs;
#endif
@@ -6207,6 +6207,40 @@ init_optabs (void)
targetm.init_libfuncs ();
}
+/* Recompute the optabs. If they have changed, save the new set of
+ optabs in the optimization node OPTNODE. */
+
+void
+save_optabs_if_changed (tree optnode)
+{
+ struct target_optabs *save_target_optabs = this_target_optabs;
+ struct target_optabs *tmp_target_optabs = XCNEW (struct target_optabs);
+
+ /* Generate a new set of optabs into tmp_target_optabs. */
+ this_target_optabs = tmp_target_optabs;
+ init_all_optabs ();
+ this_target_optabs = save_target_optabs;
+
+ /* If the optabs changed, record it in the node. */
+ if (memcmp (tmp_target_optabs, &default_target_optabs,
+ sizeof (struct target_optabs)))
+{
+ /* ?? An existing entry in TREE_OPTIMIZATION_OPTABS indicates
+multiple ((optimize)) attributes for the same function. Is
+this even valid? For now, just clobber the existing entry
+with the new optabs. */
+ if (TREE_OPTIMIZATION_OPTABS (optnode))
+ XDELETE (TREE_OPTIMIZATION_OPTABS (optnode));
+
+ TREE_OPTIMIZATION_OPTABS (optnode) = tmp_target_optabs;
+}
+ else
+{
+ TREE_OPTIMIZATION_OPTABS (optnode) = NULL;
+ XDELETE (tmp_target_optabs);
+}
+}
+
/* A helper function for init_sync_libfuncs. Using the basename BASE,
install libfuncs into TAB for BASE_N for 1 <= N <= MAX. */
diff --git a/gcc/optabs.h b/gcc/optabs.h
index c08adcf..2e8b6ec 100644
--- a/gcc/optabs.h
+++ b/gcc/optabs.h
@@ -76,11 +76,7 @@ struct target_optabs {
};
extern struct target_optabs default_target_optabs;
-#if SWITCHABLE_TARGET
extern struct target_optabs *this_target_optabs;
-#else
-#define this_target_optabs (&default_target_optabs)
-#endif
/* Define functions given in optabs.c.
Re: PR target/52555: attribute optimize is overriding command line options
On Mon, Feb 11, 2013 at 06:15:05PM -0600, Aldy Hernandez wrote:
> How does this look?
Looks good to me.
> Jakub, what's this you mention in the PR about caching
> __optimize__((3))? You also mention I shouldn't compare against
> this_target_optabs, but default_target_optabs. But what if
> this_target_optabs has changed? (See patch).
The reason for that is that this_target_optabs could at that point be
simply whatever optabs used the last parsed function.
this_target_optabs changes only either because of optimize attribute
(not sure if MIPS as the only switchable target? supports that), or
because of mips_set_mips16_mode. I think invoke_set_current_function_hook
invokes the target hook after the code you've changed, so I'd say it should
work fine even on MIPS. CCing Richard for that anyway.
>
> Tested on x86-64 Linux. It would be nice if someone with access to
> a SWITCHABLE_TARGET platform (mips) could also test this.
A few nits below. I'm not sure about the behavior of multiple optimize
attributes either, let's try it and see how it is handled right now
(ignoring optabs).
> @@ -6184,6 +6184,41 @@ init_optabs (void)
>targetm.init_libfuncs ();
> }
>
> +/* Recompute the optabs. If they have changed, save the new set of
> + optabs in the optimization node OPTNODE. */
> +
> +void
> +save_optabs_if_changed (tree optnode)
> +{
> + struct target_optabs *save_target_optabs = this_target_optabs;
> + struct target_optabs *tmp_target_optabs = XNEW (struct target_optabs);
> +
> + /* Generate a new set of optabs into tmp_target_optabs. */
> + memset (tmp_target_optabs, 0, sizeof (struct target_optabs));
I think you should just use XCNEW and drop the memset.
> + this_target_optabs = tmp_target_optabs;
> + init_all_optabs ();
> + this_target_optabs = save_target_optabs;
> +
> + /* If the optabs changed, record it in the node. */
> + if (memcmp (tmp_target_optabs, this_target_optabs,
> + sizeof (struct target_optabs)))
> +{
> + /* ?? An existing entry in TREE_OPTIMIZATION_OPTABS indicates
> + multiple ((optimize)) attributes for the same function. Is
> + this even valid? For now, just clobber the existing entry
> + with the new optabs. */
> + if (TREE_OPTIMIZATION_OPTABS (optnode))
> + free (TREE_OPTIMIZATION_OPTABS (optnode));
> +
> + TREE_OPTIMIZATION_OPTABS (optnode) = tmp_target_optabs;
> +}
> + else
> +{
> + TREE_OPTIMIZATION_OPTABS (optnode) = NULL;
> + free (tmp_target_optabs);
Shouldn't this (and above) be XDELETE to match the allocation style?
Jakub
