[PATCH] Fix PR42617, alias-export and restrict pointers
Richard Guenther
rguenther@suse.de
Tue Apr 6 14:59:00 GMT 2010
On Tue, 6 Apr 2010, Richard Guenther wrote:
> On Tue, 6 Apr 2010, Richard Guenther wrote:
>
> >
> > I am currently re-testing the following three patches against trunk
> > and will commit them to fix alias-export with regarding to
> > restrict qualified pointers (well, generally for pointer accesses).
>
> And I now run into a latent problem with the first patch. IVOPTs
> expands memory accesses to RTL to determine computation cost.
> This ends up creating MEM_ATTRs with MEM_EXPRs that contain SSA
> names. Those are _permanently_ stored in the mem_attrs_htab
> (well, unless collected), including the references to eventually
> released SSA names. On MEM_EXPR lookup we call operand_equal_p
> which eventually looks at the type of its operands which, for
> released SSA names, is NULL_TREE. Oops.
>
> Now that IVOPTs expands things is ugly, and it technically doesn't need
> MEM_EXPRs at all. Thus we either can avoid generating them in the
> first place (though I see no easy way to do so apart from adding a
> global flag), or at least avoid keeping them in the hashtable.
>
> This is what the following implements - we switch the mem_attrs
> hashtable to a temporary one during IVOPTs (carefully throwing
> away old entries once we release SSA names, which we do once
> for each IV optimized loop).
>
> I'm not sure this completely avoids the issues (as the hashtable is
> still global, so we might get references to released SSA names
> across functions), as RTL alias export will cause SSA names to
> leak into the hashtable as well (but they are not released until
> the function is completely processed).
>
> The IVOPTs problem is latent on all branches, the RTL export
> issue only on 4.5 and onwards.
>
> Any better ideas? Why do we share MEM_ATTRs across functions
> in the first place?
Alternative patch, harden operand_equal_p and tree_nop_conversion
against released SSA names. Should completely fix the issue.
Richard.
Index: gcc/tree.c
===================================================================
*** gcc/tree.c (revision 157991)
--- gcc/tree.c (working copy)
*************** tree_nop_conversion (const_tree exp)
*** 10645,10650 ****
--- 10645,10653 ----
outer_type = TREE_TYPE (exp);
inner_type = TREE_TYPE (TREE_OPERAND (exp, 0));
+ if (!inner_type)
+ return false;
+
/* Use precision rather then machine mode when we can, which gives
the correct answer even for submode (bit-field) types. */
if ((INTEGRAL_TYPE_P (outer_type)
Index: gcc/fold-const.c
===================================================================
*** gcc/fold-const.c (revision 157991)
--- gcc/fold-const.c (working copy)
*************** operand_equal_p (const_tree arg0, const_
*** 3170,3175 ****
--- 3170,3180 ----
|| TREE_TYPE (arg1) == error_mark_node)
return 0;
+ /* Similar, if either does not have a type (like a released SSA name),
+ they aren't equal. */
+ if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1))
+ return 0;
+
/* Check equality of integer constants before bailing out due to
precision differences. */
if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST)
>
> Thanks,
> Richard.
>
> 2010-04-06 Richard Guenther <rguenther@suse.de>
>
> PR middle-end/42617
> * emit-rtl.c (set_mem_attributes_minus_bitpos): Do not
> discard plain indirect references.
>
> (mem_attrs_htab_saved): New global variable.
> (switch_mem_attr_htab): New function.
> * rtl.h (switch_mem_attr_htab): Declare.
> * tree-ssa-loop-ivopts.c (tree_ssa_iv_optimize): Switch
> to alternate MEM_ATTR hashtable.
>
> Index: gcc/emit-rtl.c
> ===================================================================
> *** gcc/emit-rtl.c.orig 2010-04-06 16:03:54.000000000 +0200
> --- gcc/emit-rtl.c 2010-04-06 16:26:00.000000000 +0200
> *************** static GTY ((if_marked ("ggc_marked_p"),
> *** 160,165 ****
> --- 160,169 ----
> static GTY ((if_marked ("ggc_marked_p"), param_is (struct reg_attrs)))
> htab_t reg_attrs_htab;
>
> + /* A hash table storing memory attribute structures. */
> + static GTY ((if_marked ("ggc_marked_p"), param_is (struct mem_attrs)))
> + htab_t mem_attrs_htab_saved;
> +
> /* A hash table storing all CONST_DOUBLEs. */
> static GTY ((if_marked ("ggc_marked_p"), param_is (struct rtx_def)))
> htab_t const_double_htab;
> *************** set_mem_attributes_minus_bitpos (rtx ref
> *** 1750,1755 ****
> --- 1754,1760 ----
> /* ??? Any reason the field size would be different than
> the size we got from the type? */
> }
> +
> else if (flag_argument_noalias > 1
> && (INDIRECT_REF_P (t2))
> && TREE_CODE (TREE_OPERAND (t2, 0)) == PARM_DECL)
> *************** set_mem_attributes_minus_bitpos (rtx ref
> *** 1757,1762 ****
> --- 1762,1776 ----
> expr = t2;
> offset = NULL;
> }
> +
> + /* If this is an indirect reference, record it. */
> + else if (TREE_CODE (t) == INDIRECT_REF
> + || TREE_CODE (t) == MISALIGNED_INDIRECT_REF)
> + {
> + expr = t;
> + offset = const0_rtx;
> + apply_bitpos = bitpos;
> + }
> }
>
> /* If this is a Fortran indirect argument reference, record the
> *************** set_mem_attributes_minus_bitpos (rtx ref
> *** 1769,1774 ****
> --- 1783,1797 ----
> offset = NULL;
> }
>
> + /* If this is an indirect reference, record it. */
> + else if (TREE_CODE (t) == INDIRECT_REF
> + || TREE_CODE (t) == MISALIGNED_INDIRECT_REF)
> + {
> + expr = t;
> + offset = const0_rtx;
> + apply_bitpos = bitpos;
> + }
> +
> if (!align_computed && !INDIRECT_REF_P (t))
> {
> unsigned int obj_align
> *************** init_emit_once (void)
> *** 5965,5970 ****
> --- 5988,6013 ----
> if (STORE_FLAG_VALUE == 1)
> const_tiny_rtx[1][(int) BImode] = const1_rtx;
> }
> +
> + /* Switches the MEM_ATTR hashtable to a temporary one or frees it
> + and restores the previously saved one. */
> +
> + void
> + switch_mem_attr_htab (void)
> + {
> + if (mem_attrs_htab_saved)
> + {
> + htab_delete (mem_attrs_htab);
> + mem_attrs_htab = mem_attrs_htab_saved;
> + mem_attrs_htab_saved = NULL;
> + }
> + else
> + {
> + mem_attrs_htab_saved = mem_attrs_htab;
> + mem_attrs_htab = htab_create_ggc (37, mem_attrs_htab_hash,
> + mem_attrs_htab_eq, NULL);
> + }
> + }
>
> /* Produce exact duplicate of insn INSN after AFTER.
> Care updating of libcall regions if present. */
> Index: gcc/rtl.h
> ===================================================================
> *** gcc/rtl.h.orig 2010-03-16 13:30:28.000000000 +0100
> --- gcc/rtl.h 2010-04-06 16:37:07.000000000 +0200
> *************** extern void force_next_line_note (void);
> *** 2225,2230 ****
> --- 2225,2231 ----
> extern void init_emit (void);
> extern void init_emit_regs (void);
> extern void init_emit_once (void);
> + extern void switch_mem_attr_htab (void);
> extern void push_topmost_sequence (void);
> extern void pop_topmost_sequence (void);
> extern void set_new_first_and_last_insn (rtx, rtx);
> Index: gcc/tree-ssa-loop-ivopts.c
> ===================================================================
> *** gcc/tree-ssa-loop-ivopts.c.orig 2010-04-01 18:16:50.000000000 +0200
> --- gcc/tree-ssa-loop-ivopts.c 2010-04-06 16:44:22.000000000 +0200
> *************** tree_ssa_iv_optimize (void)
> *** 5864,5870 ****
> --- 5864,5874 ----
> if (dump_file && (dump_flags & TDF_DETAILS))
> flow_loop_dump (loop, dump_file, NULL, 1);
>
> + /* Avoid permanently entering MEM_ATTRs we create into the
> + global hashtable. They might refer to released SSA names. */
> + switch_mem_attr_htab ();
> tree_ssa_iv_optimize_loop (&data, loop);
> + switch_mem_attr_htab ();
> }
>
> tree_ssa_iv_optimize_finalize (&data);
>
>
> > 2010-04-06 Richard Guenther <rguenther@suse.de>
> >
> > PR middle-end/42617
> > * emit-rtl.c (set_mem_attributes_minus_bitpos): Do not
> > discard plain indirect references.
> >
> > Index: gcc/emit-rtl.c
> > ===================================================================
> > *** gcc/emit-rtl.c.orig 2010-01-22 14:13:11.000000000 +0100
> > --- gcc/emit-rtl.c 2010-01-22 14:29:31.000000000 +0100
> > *************** set_mem_attributes_minus_bitpos (rtx ref
> > *** 1750,1755 ****
> > --- 1750,1756 ----
> > /* ??? Any reason the field size would be different than
> > the size we got from the type? */
> > }
> > +
> > else if (flag_argument_noalias > 1
> > && (INDIRECT_REF_P (t2))
> > && TREE_CODE (TREE_OPERAND (t2, 0)) == PARM_DECL)
> > *************** set_mem_attributes_minus_bitpos (rtx ref
> > *** 1757,1762 ****
> > --- 1758,1772 ----
> > expr = t2;
> > offset = NULL;
> > }
> > +
> > + /* If this is an indirect reference, record it. */
> > + else if (TREE_CODE (t) == INDIRECT_REF
> > + || TREE_CODE (t) == MISALIGNED_INDIRECT_REF)
> > + {
> > + expr = t;
> > + offset = const0_rtx;
> > + apply_bitpos = bitpos;
> > + }
> > }
> >
> > /* If this is a Fortran indirect argument reference, record the
> > *************** set_mem_attributes_minus_bitpos (rtx ref
> > *** 1769,1774 ****
> > --- 1779,1793 ----
> > offset = NULL;
> > }
> >
> > + /* If this is an indirect reference, record it. */
> > + else if (TREE_CODE (t) == INDIRECT_REF
> > + || TREE_CODE (t) == MISALIGNED_INDIRECT_REF)
> > + {
> > + expr = t;
> > + offset = const0_rtx;
> > + apply_bitpos = bitpos;
> > + }
> > +
> > if (!align_computed && !INDIRECT_REF_P (t))
> > {
> > unsigned int obj_align
> >
>
--
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex
More information about the Gcc-patches
mailing list