Re: [PATCH] Fix issues with flag_strict_aliasing changing during compilation (PR c/37106)

["Richard Guenther" <richard dot guenther at gmail dot com>]

On Mon, Nov 3, 2008 at 4:33 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> Clearly a lot of code isn't prepared for flag_strict_aliasing to change
> during compilation.
> new_alias_set () returns 0 if currently !flag_strict_aliasing, but in places
> where we call new_alias_set () just once and remember it in some global
> variable, we might use non-zero in !flag_strict_aliasing mode or vice versa,
> depending on which routine it has been called for first.
> mems_in_disjoint_alias_sets_p attempts to verify that in
> -fno-strict-aliasing mode MEMs have alias set 0, but if flag_strict_aliasing has
> ever been set during the compilation of current CU, this might not be true
> (for ix86_GOT_alias_set this can be fixed in that routine, as RTLs with
> such alias sets shouldn't be shared in between routines, but there are
> cases (e.g. MEMs using const_alias_set) which are shared between different
> routines.
> Still I think the mems_in_disjoint_alias_sets_p checking is useful, but
> can be done only if flag_strict_aliasing has never been set in any routine.
> Thus I've added flag_strict_aliasing_ever_set variable and use it in the
> assert.
> While that change alone fixes the failure, I think the other changes
> are still desirable, they affect generated code quality.  Say if
> the first routine being compiled in the CU uses optimize(1) and the rest
> uses -O2, then ix86_GOT_alias_set () will return always 0, even in the
> flag_strict_aliasing routines.
>
> Ok for trunk if bootstrap/regtest passes?

Hmm.  I do not like this too much.  For 4.4 I'd like us simply avoid
changing flag_strict_aliasing.  For 4.5 we should move flag_strict_aliasing
to struct function instead.

Can you see what it takes to avoid modifying flag_strict_aliasing?

Thanks,
Richard.

> 2008-11-03  Jakub Jelinek  <jakub@redhat.com>
>
>        PR c/37106
>        * config/rs6000/rs6000.c (get_TOC_alias_set): Return 0 immediately for
>        !flag_strict_aliasing.
>        * config/i386/i386.c (ix86_GOT_alias_set): Likewise.
>        * config/alpha/alpha.c (override_options): Don't change
>        alpha_sr_alias_set if it has been already set.
>        * config/sparc/sparc.c (sparc_override_options): Similarly for
>        sparc_sr_alias_set and struct_value_alias_set.
>        * alias.h (flag_strict_aliasing_ever_set): New extern decl.
>        * alias.c (flag_strict_aliasing_ever_set): New variable.
>        (mems_in_disjoint_alias_sets_p): Use flag_strict_aliasing_ever_set
>        instead of flag_strict_aliasing in the assert.
>        (get_varargs_alias_set, get_frame_alias_set): Return 0 immediately for
>        !flag_strict_aliasing.
>        * opts.c (decode_options): Or flag_strict_aliasing into
>        flag_strict_aliasing_ever_set.
>        * builtins.c (expand_builtin_setjmp_setup, expand_builtin_longjmp): Use
>        0 instead of setjmp_alias_set for !flag_strict_aliasing.
>        * c-pch.c (c_common_read_pch): Or previous
>        flag_strict_aliasing_ever_set into flag_strict_aliasing_ever_set.
>
> --- gcc/config/rs6000/rs6000.c.jj       2008-11-03 11:15:08.000000000 +0100
> +++ gcc/config/rs6000/rs6000.c  2008-11-03 15:58:25.000000000 +0100
> @@ -15418,6 +15418,8 @@ static GTY(()) alias_set_type set = -1;
>  alias_set_type
>  get_TOC_alias_set (void)
>  {
> +  if (!flag_strict_aliasing)
> +    return 0;
>   if (set == -1)
>     set = new_alias_set ();
>   return set;
> --- gcc/config/i386/i386.c.jj   2008-10-29 18:49:06.000000000 +0100
> +++ gcc/config/i386/i386.c      2008-11-03 15:08:19.000000000 +0100
> @@ -9085,6 +9085,8 @@ static alias_set_type
>  ix86_GOT_alias_set (void)
>  {
>   static alias_set_type set = -1;
> +  if (!flag_strict_aliasing)
> +    return 0;
>   if (set == -1)
>     set = new_alias_set ();
>   return set;
> --- gcc/config/alpha/alpha.c.jj 2008-09-30 16:57:01.000000000 +0200
> +++ gcc/config/alpha/alpha.c    2008-11-03 16:00:36.000000000 +0100
> @@ -503,7 +503,8 @@ override_options (void)
>     align_functions = 16;
>
>   /* Acquire a unique set number for our register saves and restores.  */
> -  alpha_sr_alias_set = new_alias_set ();
> +  if (alpha_sr_alias_set == 0)
> +    alpha_sr_alias_set = new_alias_set ();
>
>   /* Register variables and functions with the garbage collector.  */
>
> --- gcc/config/sparc/sparc.c.jj 2008-09-30 16:57:02.000000000 +0200
> +++ gcc/config/sparc/sparc.c    2008-11-03 16:01:38.000000000 +0100
> @@ -815,8 +815,11 @@ sparc_override_options (void)
>   sparc_init_modes ();
>
>   /* Acquire unique alias sets for our private stuff.  */
> -  sparc_sr_alias_set = new_alias_set ();
> -  struct_value_alias_set = new_alias_set ();
> +  if (sparc_sr_alias_set == 0)
> +    {
> +      sparc_sr_alias_set = new_alias_set ();
> +      struct_value_alias_set = new_alias_set ();
> +    }
>
>   /* Set up function hooks.  */
>   init_machine_status = sparc_init_machine_status;
> --- gcc/alias.h.jj      2008-09-30 16:57:11.000000000 +0200
> +++ gcc/alias.h 2008-11-03 16:03:39.000000000 +0100
> @@ -30,6 +30,10 @@ along with GCC; see the file COPYING3.
>    set yet).  */
>  typedef int alias_set_type;
>
> +/* Set if flag_strict_aliasing has been set for at least one function
> +   in the CU.  */
> +extern GTY(()) int flag_strict_aliasing_ever_set;
> +
>  extern alias_set_type new_alias_set (void);
>  extern alias_set_type get_alias_set (tree);
>  extern alias_set_type get_varargs_alias_set (void);
> --- gcc/c-pch.c.jj      2008-09-30 16:57:11.000000000 +0200
> +++ gcc/c-pch.c 2008-11-03 16:06:09.000000000 +0100
> @@ -368,6 +368,7 @@ c_common_read_pch (cpp_reader *pfile, co
>   struct save_macro_data *smd;
>   expanded_location saved_loc;
>   bool saved_trace_includes;
> +  int saved_fsa_ever_set;
>
>   f = fdopen (fd, "rb");
>   if (f == NULL)
> @@ -414,6 +415,7 @@ c_common_read_pch (cpp_reader *pfile, co
>   /* Save the location and then restore it after reading the PCH.  */
>   saved_loc = expand_location (line_table->highest_line);
>   saved_trace_includes = line_table->trace_includes;
> +  saved_fsa_ever_set = flag_strict_aliasing_ever_set;
>
>   cpp_prepare_state (pfile, &smd);
>
> @@ -430,6 +432,7 @@ c_common_read_pch (cpp_reader *pfile, co
>   line_table->trace_includes = saved_trace_includes;
>   cpp_set_line_map (pfile, line_table);
>   linemap_add (line_table, LC_RENAME, 0, saved_loc.file, saved_loc.line);
> +  flag_strict_aliasing_ever_set |= saved_fsa_ever_set;
>
>   /* Give the front end a chance to take action after a PCH file has
>      been loaded.  */
> --- gcc/opts.c.jj       2008-10-14 14:00:02.000000000 +0200
> +++ gcc/opts.c  2008-11-03 15:56:09.000000000 +0100
> @@ -1116,7 +1116,7 @@ decode_options (unsigned int argc, const
>       if (!PARAM_SET_P (PARAM_STACK_FRAME_GROWTH))
>         PARAM_VALUE (PARAM_STACK_FRAME_GROWTH) = 40;
>     }
> -
> +  flag_strict_aliasing_ever_set |= flag_strict_aliasing;
>  }
>
>  #define LEFT_COLUMN    27
> --- gcc/alias.c.jj      2008-10-14 13:58:50.000000000 +0200
> +++ gcc/alias.c 2008-11-03 15:46:34.000000000 +0100
> @@ -228,6 +228,10 @@ static GTY((length("reg_known_value_size
>  /* Indicates number of valid entries in reg_known_value.  */
>  static GTY(()) unsigned int reg_known_value_size;
>
> +/* Set if flag_strict_aliasing has been set for at least one function
> +   in the CU.  */
> +int flag_strict_aliasing_ever_set;
> +
>  /* Vector recording for each reg_known_value whether it is due to a
>    REG_EQUIV note.  Future passes (viz., reload) may replace the
>    pseudo with the equivalent expression and so we account for the
> @@ -274,7 +278,7 @@ mems_in_disjoint_alias_sets_p (const_rtx
>    gen_rtx_MEM, and the MEM_ALIAS_SET is not cleared.  If we begin to
>    use alias sets to indicate that spilled registers cannot alias each
>    other, we might need to remove this check.  */
> -  gcc_assert (flag_strict_aliasing
> +  gcc_assert (flag_strict_aliasing_ever_set
>              || (!MEM_ALIAS_SET (mem1) && !MEM_ALIAS_SET (mem2)));
>
>   return ! alias_sets_conflict_p (MEM_ALIAS_SET (mem1), MEM_ALIAS_SET (mem2));
> @@ -808,6 +812,8 @@ get_varargs_alias_set (void)
>      area.  So don't use it anywhere.  */
>   return 0;
>  #else
> +  if (!flag_strict_aliasing)
> +    return 0;
>   if (varargs_set == -1)
>     varargs_set = new_alias_set ();
>
> @@ -823,6 +829,8 @@ static GTY(()) alias_set_type frame_set
>  alias_set_type
>  get_frame_alias_set (void)
>  {
> +  if (!flag_strict_aliasing)
> +    return 0;
>   if (frame_set == -1)
>     frame_set = new_alias_set ();
>
> --- gcc/builtins.c.jj   2008-11-03 11:15:08.000000000 +0100
> +++ gcc/builtins.c      2008-11-03 15:39:55.000000000 +0100
> @@ -681,9 +681,16 @@ expand_builtin_setjmp_setup (rtx buf_add
>   enum machine_mode sa_mode = STACK_SAVEAREA_MODE (SAVE_NONLOCAL);
>   rtx stack_save;
>   rtx mem;
> +  alias_set_type alias;
>
> -  if (setjmp_alias_set == -1)
> -    setjmp_alias_set = new_alias_set ();
> +  if (!flag_strict_aliasing)
> +    alias = 0;
> +  else
> +    {
> +      if (setjmp_alias_set == -1)
> +       setjmp_alias_set = new_alias_set ();
> +      alias = setjmp_alias_set;
> +    }
>
>   buf_addr = convert_memory_address (Pmode, buf_addr);
>
> @@ -694,11 +701,11 @@ expand_builtin_setjmp_setup (rtx buf_add
>      is machine-dependent.  */
>
>   mem = gen_rtx_MEM (Pmode, buf_addr);
> -  set_mem_alias_set (mem, setjmp_alias_set);
> +  set_mem_alias_set (mem, alias);
>   emit_move_insn (mem, targetm.builtin_setjmp_frame_value ());
>
>   mem = gen_rtx_MEM (Pmode, plus_constant (buf_addr, GET_MODE_SIZE (Pmode))),
> -  set_mem_alias_set (mem, setjmp_alias_set);
> +  set_mem_alias_set (mem, alias);
>
>   emit_move_insn (validize_mem (mem),
>                  force_reg (Pmode, gen_rtx_LABEL_REF (Pmode, receiver_label)));
> @@ -706,7 +713,7 @@ expand_builtin_setjmp_setup (rtx buf_add
>   stack_save = gen_rtx_MEM (sa_mode,
>                            plus_constant (buf_addr,
>                                           2 * GET_MODE_SIZE (Pmode)));
> -  set_mem_alias_set (stack_save, setjmp_alias_set);
> +  set_mem_alias_set (stack_save, alias);
>   emit_stack_save (SAVE_NONLOCAL, &stack_save, NULL_RTX);
>
>   /* If there is further processing to do, do it.  */
> @@ -800,14 +807,21 @@ expand_builtin_longjmp (rtx buf_addr, rt
>  {
>   rtx fp, lab, stack, insn, last;
>   enum machine_mode sa_mode = STACK_SAVEAREA_MODE (SAVE_NONLOCAL);
> +  alias_set_type alias;
>
>   /* DRAP is needed for stack realign if longjmp is expanded to current
>      function  */
>   if (SUPPORTS_STACK_ALIGNMENT)
>     crtl->need_drap = true;
>
> -  if (setjmp_alias_set == -1)
> -    setjmp_alias_set = new_alias_set ();
> +  if (!flag_strict_aliasing)
> +    alias = 0;
> +  else
> +    {
> +      if (setjmp_alias_set == -1)
> +       setjmp_alias_set = new_alias_set ();
> +      alias = setjmp_alias_set;
> +    }
>
>   buf_addr = convert_memory_address (Pmode, buf_addr);
>
> @@ -833,9 +847,9 @@ expand_builtin_longjmp (rtx buf_addr, rt
>
>       stack = gen_rtx_MEM (sa_mode, plus_constant (buf_addr,
>                                                   2 * GET_MODE_SIZE (Pmode)));
> -      set_mem_alias_set (fp, setjmp_alias_set);
> -      set_mem_alias_set (lab, setjmp_alias_set);
> -      set_mem_alias_set (stack, setjmp_alias_set);
> +      set_mem_alias_set (fp, alias);
> +      set_mem_alias_set (lab, alias);
> +      set_mem_alias_set (stack, alias);
>
>       /* Pick up FP, label, and SP from the block and jump.  This code is
>         from expand_goto in stmt.c; see there for detailed comments.  */
>
>        Jakub
>