Don't create superfluous parm in expand_omp_taskreg

[ was: Re: [committed, gomp4] Fix release_dangling_ssa_names ]

On 05/08/15 13:13, Richard Biener wrote:
> On Wed, 5 Aug 2015, Tom de Vries wrote:
>
>> On 05/08/15 11:30, Richard Biener wrote:
>>> On Wed, 5 Aug 2015, Tom de Vries wrote:
>>>
>>>> On 05/08/15 09:29, Richard Biener wrote:
>>>>>> This patch fixes that by making sure we reset the def stmt to NULL.
>>>>>> This
>>>>>> means
>>>>>>> we can simplify release_dangling_ssa_names to just test for NULL def
>>>>>> stmts.
>>>>> Not sure if I understand the problem correctly but why are you not
>>>>> simply
>>>>> releasing the SSA name when you remove its definition?
>>>>
>>>> In move_sese_region_to_fn we move a region of blocks from one function to
>>>> another, bit by bit.
>>>>
>>>> When we encounter an ssa_name as def or use in the region, we:
>>>> - generate a new ssa_name,
>>>> - set the def stmt of the old name as def stmt of the new name, and
>>>> - add a mapping from the old to the new name.
>>>> The next time we encounter the same ssa_name in another statement, we find
>>>> it
>>>> in the map.
>>>>
>>>> If we release the old ssa name, we effectively create statements with
>>>> operands
>>>> in the free-list. The first point where that cause breakage, is in
>>>> walk_gimple_op, which expects the TREE_TYPE of the lhs of an assign to be
>>>> defined, which is not the case if it's in the free-list:
>>>> ...
>>>> case GIMPLE_ASSIGN:
>>>>     /* Walk the RHS operands.  If the LHS is of a non-renamable type or
>>>>        is a register variable, we may use a COMPONENT_REF on the RHS.*/
>>>>     if (wi)
>>>>       {
>>>>         tree lhs = gimple_assign_lhs (stmt);
>>>>         wi->val_only
>>>>           = (is_gimple_reg_type (TREE_TYPE (lhs)) && !is_gimple_reg (lhs))
>>>>              || gimple_assign_rhs_class (stmt) != GIMPLE_SINGLE_RHS;
>>>>       }
>>>> ...
>>>
>>> Hmm, ok, probably because the stmt moving doesn't happen in DOM
>>> order (move defs before uses).  But
>>>
>>
>> There seems to be similar code for the rhs, so I don't think changing the
>> order would fix anything.
>>
>>> +
>>> +      if (!SSA_NAME_IS_DEFAULT_DEF (name))
>>> +       /* The statement has been moved to the child function.  It no
>>> longer
>>> +          defines name in the original function.  Mark the def stmt NULL,
>>> and
>>> +          let release_dangling_ssa_names deal with it.  */
>>> +       SSA_NAME_DEF_STMT (name) = NULL;
>>>
>>> applies also to uses - I don't see why it couldn't happen that you
>>> move a use but not its def (the def would be a parameter to the
>>> split-out function).  You'd wreck the IL of the source function this way.
>>>
>>
>> If you first move a use, you create a mapping. When you encounter the def, you
>> use the mapping. Indeed, if the def is a default def, we don't encounter the
>> def. Which is why we create a nop as defining def for those cases. The default
>> def in the source function still has a defining nop, and has no uses anymore.
>> I don't understand what is broken here.
>
> If you never encounter the DEF then it's broken.  Say, if for
>
> foo(int a)
> {
>    int b = a;
>    if (b)
>      {
>        < code using b >
>      }
> }
>
> you move < code using b > to a function.  Then the def is still in
> foo but you create a mapping for its use(s).  Clearly the outlining
> process in this case has to pass b as parameter to the outlined
> function, something that may not happen currently.
>

Ah, I see. Indeed, this is a situation that is assumed not to happen.

> It would probably be cleaner to separate the def and use remapping
> to separate functions and record on whether we saw a def or not.
>

Right, or some other means to detect this situation, say when copying 
the def stmt in replace_ssa_name, check whether it's part of the sese 
region.

>>> I think that the whole dance of actually moving things instead of
>>> just copying it isn't worth the extra maintainance (well, if we already
>>> have a machinery duplicating a SESE region to another function - I
>>> suppose gimple_duplicate_sese_region could be trivially changed to
>>> support that).
>>>
>>
>> I'll mention that as todo. For now, I think the fastest way to get a working
>> version is to fix move_sese_region_to_fn.
>
> Sure.
>
>>> Trunk doesn't have release_dangling_ssa_names it seems
>>
>> Yep, I only ran into this trouble for the kernels region handling. But I don't
>> exclude the possibility it could happen for trunk as well.
>>
>>> but I think
>>> it belongs to move_sese_region_to_fn and not to omp-low.c
>>
>> Makes sense indeed.
>>
>>> and it
>>> could also just walk the d->vars_map replace_ssa_name fills to
>>> iterate over the removal candidates
>>
>> Agreed, I suppose in general that's a win over iterating over all the ssa
>> names.
>>
>>> (and if the situation of
>>> moving uses but not defs cannot happen you don't need any
>>> SSA_NAME_DEF_STMT dance either).
>>
>> I'd prefer to keep the SSA_NAME_DEF_STMT () = NULL bit. It makes sure a stmt
>> is the defining stmt of only one ssa-name at all times.
>>
>> I'll prepare a patch for trunk then.
>

This patch fixes two problems with expand_omp_taskreg:
- it makes sure we don't generate a dummy default def in the original
   function (which we cannot get rid of afterwards, given that it's a
   default def).
- it releases ssa-names in the original function that have defining
   statements that have been moved to the split-off function.

Bootstrapped and reg-tested on x86_64.

OK for trunk?

Thanks,
- Tom

Don't create superfluous parm in expand_omp_taskreg

Commit Message

Comments

Patch