On Thu, Feb 16, 2017 at 11:45 AM, Aldy Hernandez <al...@redhat.com> wrote:
> 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?
Yes, we should handle all of the "hidden initialized" cases at
/* 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;
maybe add a predicate for those, like
ssa_defined_default_def_p ()
right next to ssa_undefined_value_p and use it from there as well.
Richard.
>
> Aldy
