RE: [PATCH] Fix PR58115

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

Hi,

On Tue, 7 Jan 2014 15:10:20, Richard Biener wrote:
>
> On Tue, Jan 7, 2014 at 1:12 PM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>>>> How about this patch for the big comment?
>>>>
>>>
>>> The comment should say that target_set_current_function()
>>> cannot call target_reinit() because:
>>>
>>> target_reinit()=>lang_dependent_init_target()
>>> =>init_optabs()=>init_all_optabs(this_fn_optabs);
>>>
>>> uses this_fn_optabs which is undefined here.
>>>
>>> However many targets (nios2, rx, i386, rs6000) do exactly that.
>>>
>>> Is there currently any target, that sets this_target_optab in the
>>> target_set_current_function?
>>
>> MIPS :-) (via save_target_globals_default_opts=>save_target_globals)
>>
>> I think other targets need to do the same thing in order for tests
>> like that extended intrinsics_4.c to work. How does this patch look?
>> Tested on x86_64-linux-gnu.
>>
>> I didn't remove save_target_globals_default_opts because there the
>> temporary optimization_current_node also protects a call to init_reg_sets.
>
> Well, if it works the patch is ok. You're way more familiar with the
> details of this machinery.
>
> Richard.
>

I found another test case that still fails with today's trunk:

#include <immintrin.h>

__m256 a[10], b[10], c[10];

void __attribute__((target ("sse2"), optimize (3)))
foo (void)
{
}

void __attribute__((target ("avx"), optimize (3)))
bar (void)
{
  a[0] = _mm256_and_ps (b[0], c[0]);
}

compile with i686-pc-linux-gnu-gcc -O2 -msse2 -mno-avx -S  

The attached patch seems to fix this test case for
targets that do not have SWITCHABLE_TARGET.

What do you think about it?

I think Jakub's patch will fix this case, but I did not try.
However even if the i368 is now clean, there are
still many targets that use target_reinit() in
target_set_current_function.


Bernd.

>> Thanks,
>> Richard
>>
>>
>> gcc/
>> PR target/58115
>> * target-globals.c (save_target_globals): Remove this_fn_optab
>> handling.
>> * toplev.c: Include optabs.h.
>> (target_reinit): Temporarily restore the global options if another
>> set of options are in force.
>>
>> gcc/testsuite/
>> * gcc.target/i386/intrinsics_4.c (bar): New function.
>>
>> Index: gcc/target-globals.c
>> ===================================================================
>> --- gcc/target-globals.c 2014-01-02 22:16:03.042278971 +0000
>> +++ gcc/target-globals.c 2014-01-07 12:08:33.569900970 +0000
>> @@ -68,7 +68,6 @@ 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);
>> @@ -88,10 +87,8 @@ 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;
>> }
>>
>> Index: gcc/toplev.c
>> ===================================================================
>> --- gcc/toplev.c 2014-01-07 08:11:43.888058805 +0000
>> +++ gcc/toplev.c 2014-01-07 12:10:19.448096479 +0000
>> @@ -78,6 +78,7 @@ Software Foundation; either version 3, o
>> #include "diagnostic-color.h"
>> #include "context.h"
>> #include "pass_manager.h"
>> +#include "optabs.h"
>>
>> #if defined(DBX_DEBUGGING_INFO) || defined(XCOFF_DEBUGGING_INFO)
>> #include "dbxout.h"
>> @@ -1752,6 +1753,23 @@ target_reinit (void)
>> {
>> struct rtl_data saved_x_rtl;
>> rtx *saved_regno_reg_rtx;
>> + tree saved_optimization_current_node;
>> + struct target_optabs *saved_this_fn_optabs;
>> +
>> + /* Temporarily switch to the default optimization node, so that
>> + *this_target_optabs is set to the default, not reflecting
>> + whatever a previous function used for the optimize
>> + attribute. */
>> + saved_optimization_current_node = optimization_current_node;
>> + saved_this_fn_optabs = this_fn_optabs;
>> + if (saved_optimization_current_node != optimization_default_node)
>> + {
>> + optimization_current_node = optimization_default_node;
>> + cl_optimization_restore
>> + (&global_options,
>> + TREE_OPTIMIZATION (optimization_default_node));
>> + }
>> + this_fn_optabs = this_target_optabs;
>>
>> /* Save *crtl and regno_reg_rtx around the reinitialization
>> to allow target_reinit being called even after prepare_function_start. */
>> @@ -1769,7 +1787,16 @@ target_reinit (void)
>> /* Reinitialize lang-dependent parts. */
>> lang_dependent_init_target ();
>>
>> - /* And restore it at the end, as free_after_compilation from
>> + /* Restore the original optimization node. */
>> + if (saved_optimization_current_node != optimization_default_node)
>> + {
>> + optimization_current_node = saved_optimization_current_node;
>> + cl_optimization_restore (&global_options,
>> + TREE_OPTIMIZATION (optimization_current_node));
>> + }
>> + this_fn_optabs = saved_this_fn_optabs;
>> +
>> + /* Restore regno_reg_rtx at the end, as free_after_compilation from
>> expand_dummy_function_end clears it. */
>> if (saved_regno_reg_rtx)
>> {
>> Index: gcc/testsuite/gcc.target/i386/intrinsics_4.c
>> ===================================================================
>> --- gcc/testsuite/gcc.target/i386/intrinsics_4.c 2013-06-27 22:30:03.877275875 +0100
>> +++ gcc/testsuite/gcc.target/i386/intrinsics_4.c 2014-01-07 08:15:51.509627927 +0000
>> @@ -12,3 +12,10 @@ foo (void)
>> {
>> a[0] = _mm256_and_ps (b[0], c[0]);
>> }
>> +
>> +/* Try again with a combination of target and optimization attributes. */
>> +void __attribute__((target ("avx"), optimize(3)))
>> +bar (void)
>> +{
>> + a[0] = _mm256_and_ps (b[0], c[0]);
>> +}

Attachment: patch-optabs.diff
Description: Binary data