Re: Handle GIMPLE NOPs in is_maybe_undefined (PR, tree-optimization/79529).

[Aldy Hernandez <aldyh at redhat dot com>]

On 02/16/2017 03:46 AM, Martin Liška wrote:
> On 02/15/2017 05:06 PM, Aldy Hernandez wrote:
> > On 02/15/2017 09:49 AM, Martin Liška wrote:
> > > Hi.
> > >
> > > As mentioned in the PR, gimple nops are wrongly handled in is_maybe_undefined function.
> > > Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
> >
> > >       gimple *def = SSA_NAME_DEF_STMT (t);
> > >
> > > +      if (!def || gimple_nop_p (def))
> > > +    return true;
> > > +
> >
> >
> > Out of curiosity, what is T?
>
> It's a SSA name which belongs to a result_decl:
>
> (gdb) p debug_tree(t)
>  <ssa_name 0x7fffdaf3b1f8
>     type <reference_type 0x7ffff122c9d8
>         type <record_type 0x7ffff0664888 UnicodeString addressable needs-constructing BLK
>             size <integer_cst 0x7ffff6892030 constant 512>
>             unit size <integer_cst 0x7ffff6931c00 constant 64>
>             align 64 symtab 0 alias set -1 canonical type 0x7ffff66b0e70 fields <field_decl 0x7ffff0669390 D.165781> context <namespace_decl 0x7ffff66954c0 icu_58>
>             pointer_to_this <pointer_type 0x7ffff06791f8> reference_to_this <reference_type 0x7fffea593e70> chain <type_decl 0x7ffff0669428 UnicodeString>>
>         public unsigned DI
>         size <integer_cst 0x7ffff6876be8 constant 64>
>         unit size <integer_cst 0x7ffff6876c00 constant 8>
>         align 64 symtab 0 alias set -1 structural equality>
>     var <result_decl 0x7fffdaf1f528 resultPattern>
>     def_stmt GIMPLE_NOP
>     version 1
>     ptr-info 0x7fffcc191000>
>
> > Because we early bail out if ssa_undefined_value_p() right before calling SSA_NAME_DEF_STMT, and ssa_undefined_value_p() will already bail if gimple_nop_p.
> >
> > So T must be one of:
> >
> >
> >   /* Parameters get their initial value from the function entry.  */
> >   else if (TREE_CODE (var) == PARM_DECL)
> >     return false;
> >   /* When returning by reference the return address is actually a hidden
> >      parameter.  */
> >   else if (TREE_CODE (var) == RESULT_DECL && DECL_BY_REFERENCE (var))
> >     return false;
>
> This return statement is taken.
>
> >   /* Hard register variables get their initial value from the ether.  */
> >   else if (VAR_P (var) && DECL_HARD_REGISTER (var))
> >     return false;
> >
> > which I wonder if we should special case in is_maybe_undefined.
>
> Do we need a special case in the function?

It's up to Richi et al, but IMO we should probably treat this as we do 
PARM_DECL's, since according to the comment in ssa_undefined_value_p, 
returning by reference the return address is actually a hidden 
parameter.  And as such, we should 'continue' not return in 
is_maybe_undefined.  This way, we can process the rest of the items in 
the worklist.

We already handle:

      /* A PARM_DECL will not have an SSA_NAME_DEF_STMT.  Parameters
	 get their initial value from function entry.  */
      if (SSA_NAME_VAR (t) && TREE_CODE (SSA_NAME_VAR (t)) == PARM_DECL)
	continue;

I say we should also handle the rest of the things we handle in 
ssa_undefined_value_p:

  /* Parameters get their initial value from the function entry.  */
  else if (TREE_CODE (var) == PARM_DECL)
    return false;
  /* When returning by reference the return address is actually a hidden
     parameter.  */
  else if (TREE_CODE (var) == RESULT_DECL && DECL_BY_REFERENCE (var))
    return false;
  /* Hard register variables get their initial value from the ether.  */
  else if (VAR_P (var) && DECL_HARD_REGISTER (var))
    return false;

...all doing a "continue", as opposed to a return.

And if we're going to duplicate all that code, might as well abstract it 
out to an inline function.

Also, we should probably still gracefully handle an empty 
SSA_NAME_DEF_STMT as you do in your patch, but with a continue as well.

Richi, do you agree?

Aldy