Re: PRE and uninitialized variables

[Richard Biener <richard dot guenther at gmail dot com>]

On Sun, May 17, 2015 at 9:11 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Hello,
>
> first, this patch is not ready (it doesn't even bootstrap), I am posting it
> for comments. The idea is, when we do value numbering, while looking for the
> current value of a local variable, we may hit a clobber or reach the
> beginning of the function. In both cases, it seems to me that we could use a
> default definition as the value of the variable. This later allows the
> uninit pass to warn about the use of an uninitialized or clobbered variable
> (some passes can also optimize the code better). The part about clobbers
> passed regtesting, but the part about reaching the entry of the function
> fails bootstrap with:
>
> Existing SSA name for symbol marked for renaming: __d_14(D)
> internal compiler error: SSA corruption
>
> when execute_update_addresses_taken is called after SRA. In the case I
> looked at, the new code was called on the lhs of an assignment (to check if
> both sides had the same value number).

Yeah - the tail-merging code should now be stripped off using the VN as
now PRE fully propagates all values.

 Creating new ssa_names in the middle
> of sccvn is probably a bad idea, although it doesn't help if I create them
> beforehand, maybe creating the default definition and leaving it unused
> means that no one feels responsible for cleaning it up? (TODO_update_ssa
> doesn't help)
>
> I am quite restrictive in the conditions for the code to apply:
> is_gimple_reg_type, useless_type_conversion_p (uh? I forgot that one in the
> "reached entry" case...), I am not sure what I can do if the local variable
> is an aggregate and we are reading one field, maybe create an artificial
> variable just for the purpose of using its default definition?
>
> Mostly, I would like to know if the approach makes sense. Is storing the
> default definition in SSA_VAL ok, or is there some way to use VN_TOP to mark
> undefinedness? Any pointer would be welcome...

You should use VN_TOP during VN.  Otherwise you'll miss optimization
opportunities.  Now the question is then what to use at eliminate() time.
To get useful warnings you'd need to distinguish which VN_TOP you
get?

Note that funneling in VN_TOP might be tricky ;)

Richard.

>         * tree-ssa-sccvn.c (vn_reference_lookup_2): Handle function entry.
>         (vn_reference_lookup_3): Handle clobbers.
>         (init_scc_vn): Default definitions are their own definition.
>
> PS: the testsuite for libgomp is looong, it almost doubles the time for the
> whole testsuite to complete on gcc112. It would be great if someone could
> split it into a few pieces that run in parallel.
>
> --
> Marc Glisse
> Index: gcc/testsuite/gcc.dg/uninit-clob.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/uninit-clob.c  (revision 0)
> +++ gcc/testsuite/gcc.dg/uninit-clob.c  (working copy)
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -Wuninitialized" } */
> +
> +int *p;
> +
> +int f(){
> +  {
> +    int q = 42;
> +    p = &q;
> +  }
> +  return *p; /* { dg-warning "uninitialized" "warning" } */
> +}
> +
> Index: gcc/testsuite/gcc.dg/uninit-sccvn.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/uninit-sccvn.c (revision 0)
> +++ gcc/testsuite/gcc.dg/uninit-sccvn.c (working copy)
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -Wuninitialized" } */
> +
> +int g, h;
> +void *p;
> +int f(int x){
> +  int a;
> +  g = 42;
> +  h = a;
> +  p = &a;
> +  return h; /* { dg-warning "uninitialized" "warning" } */
> +}
> Index: gcc/tree-ssa-sccvn.c
> ===================================================================
> --- gcc/tree-ssa-sccvn.c        (revision 223269)
> +++ gcc/tree-ssa-sccvn.c        (working copy)
> @@ -1553,26 +1553,33 @@ vn_reference_lookup_1 (vn_reference_t vr
>         *vnresult = (vn_reference_t)*slot;
>        return ((vn_reference_t)*slot)->result;
>      }
>
>    return NULL_TREE;
>  }
>
>  static tree *last_vuse_ptr;
>  static vn_lookup_kind vn_walk_kind;
>  static vn_lookup_kind default_vn_walk_kind;
> +static vn_reference_t
> +vn_reference_lookup_or_insert_for_pieces (tree vuse,
> +                                         alias_set_type set,
> +                                         tree type,
> +                                         vec<vn_reference_op_s,
> +                                               va_heap> operands,
> +                                         tree value);
>
>  /* Callback for walk_non_aliased_vuses.  Adjusts the vn_reference_t VR_
>     with the current VUSE and performs the expression lookup.  */
>
>  static void *
> -vn_reference_lookup_2 (ao_ref *op ATTRIBUTE_UNUSED, tree vuse,
> +vn_reference_lookup_2 (ao_ref *ref, tree vuse,
>                        unsigned int cnt, void *vr_)
>  {
>    vn_reference_t vr = (vn_reference_t)vr_;
>    vn_reference_s **slot;
>    hashval_t hash;
>
>    /* This bounds the stmt walks we perform on reference lookups
>       to O(1) instead of O(N) where N is the number of dominating
>       stores.  */
>    if (cnt > (unsigned) PARAM_VALUE
> (PARAM_SCCVN_MAX_ALIAS_QUERIES_PER_ACCESS))
> @@ -1587,20 +1594,39 @@ vn_reference_lookup_2 (ao_ref *op ATTRIB
>    vr->vuse = vuse_ssa_val (vuse);
>    if (vr->vuse)
>      vr->hashcode = vr->hashcode + SSA_NAME_VERSION (vr->vuse);
>
>    hash = vr->hashcode;
>    slot = current_info->references->find_slot_with_hash (vr, hash,
> NO_INSERT);
>    if (!slot && current_info == optimistic_info)
>      slot = valid_info->references->find_slot_with_hash (vr, hash,
> NO_INSERT);
>    if (slot)
>      return *slot;
> +  if (gimple_nop_p (SSA_NAME_DEF_STMT (vuse)))
> +    {
> +      tree base = ao_ref_base (ref);
> +      if (TREE_CODE (base) == VAR_DECL
> +         && !is_global_var (base)
> +         && is_gimple_reg_type (vr->type))
> +       {
> +         tree val = ssa_default_def (cfun, base);
> +         if (!val)
> +           {
> +             val = get_or_create_ssa_default_def (cfun, base);
> +             VN_INFO_GET (val)->valnum = val;
> +             VN_INFO (val)->expr = NULL_TREE;
> +             VN_INFO (val)->value_id = 0;
> +           }
> +         return vn_reference_lookup_or_insert_for_pieces
> +                  (vuse, vr->set, vr->type, vr->operands, val);
> +       }
> +    }
>
>    return NULL;
>  }
>
>  /* Lookup an existing or insert a new vn_reference entry into the
>     value table for the VUSE, SET, TYPE, OPERANDS reference which
>     has the value VALUE which is either a constant or an SSA name.  */
>
>  static vn_reference_t
>  vn_reference_lookup_or_insert_for_pieces (tree vuse,
> @@ -1712,21 +1738,40 @@ vn_reference_lookup_3 (ao_ref *ref, tree
>    offset = ref->offset;
>    maxsize = ref->max_size;
>
>    /* If we cannot constrain the size of the reference we cannot
>       test if anything kills it.  */
>    if (maxsize == -1)
>      return (void *)-1;
>
>    /* We can't deduce anything useful from clobbers.  */
>    if (gimple_clobber_p (def_stmt))
> -    return (void *)-1;
> +    {
> +      if (TREE_CODE (base) == VAR_DECL
> +         && !is_global_var (base)
> +         && operand_equal_p (base, gimple_assign_lhs (def_stmt), 0)
> +         && is_gimple_reg_type (vr->type)
> +         && useless_type_conversion_p (vr->type, TREE_TYPE (base)))
> +       {
> +         tree val = ssa_default_def (cfun, base);
> +         if (!val)
> +           {
> +             val = get_or_create_ssa_default_def (cfun, base);
> +             VN_INFO_GET (val)->valnum = val;
> +             VN_INFO (val)->expr = NULL_TREE;
> +             VN_INFO (val)->value_id = 0;
> +           }
> +         return vn_reference_lookup_or_insert_for_pieces
> +                  (vuse, vr->set, vr->type, vr->operands, val);
> +       }
> +      return (void *)-1;
> +    }
>
>    /* def_stmt may-defs *ref.  See if we can derive a value for *ref
>       from that definition.
>       1) Memset.  */
>    if (is_gimple_reg_type (vr->type)
>        && gimple_call_builtin_p (def_stmt, BUILT_IN_MEMSET)
>        && integer_zerop (gimple_call_arg (def_stmt, 1))
>        && tree_fits_uhwi_p (gimple_call_arg (def_stmt, 2))
>        && TREE_CODE (gimple_call_arg (def_stmt, 0)) == ADDR_EXPR)
>      {
> @@ -4190,21 +4235,22 @@ init_scc_vn (void)
>
>    VN_TOP = create_tmp_var_raw (void_type_node, "vn_top");
>
>    /* Create the VN_INFO structures, and initialize value numbers to
>       TOP.  */
>    for (i = 0; i < num_ssa_names; i++)
>      {
>        tree name = ssa_name (i);
>        if (name)
>         {
> -         VN_INFO_GET (name)->valnum = VN_TOP;
> +         VN_INFO_GET (name)->valnum = SSA_NAME_IS_DEFAULT_DEF (name) ? name
> +                                                                     :
> VN_TOP;
>           VN_INFO (name)->expr = NULL_TREE;
>           VN_INFO (name)->value_id = 0;
>         }
>      }
>
>    renumber_gimple_stmt_uids ();
>
>    /* Create the valid and optimistic value numbering tables.  */
>    valid_info = XCNEW (struct vn_tables_s);
>    allocate_vn_table (valid_info);
>