Fix PR52448 (cselim with calls)

Hi,

marking of non-trapping accesses currently has an issue when there's a 
call between the dominated and the dominating access (that makes the 
former non-trapping), namely when that call frees the accessed memory (or 
makes it unavailable via other means).

Instead of a full-blown algorithm iterating until a fixed point to exactly 
determine the killed set of accesses I simply clear the whole hash table 
of seen accesses whenever I see a call, which requires being conservative 
on backedges (or generally when there are unvisited predecessors, but the 
current domwalker ensures that domchilds are visited in topological 
order).  Quick clearing is done by using a phase counter.

This still retains the important transformation in hmmer, but fixes the 
bug.  Regstrapped on x86_64-linux, okay for trunk?


Ciao,
Michael.

On Wed, Feb 06, 2013 at 02:45:10PM +0100, Michael Matz wrote:
> @@ -1329,20 +1337,53 @@ add_or_mark_expr (basic_block bb, tree e
>      }
>  }
>  
> +/* Return true when CALL is a call stmt that definitely doesn't
> +   free any memory or makes it unavailable otherwise.  */
> +static bool
> +nonfreeing_call_p (gimple call)
> +{
> +  tree fndecl = gimple_call_fndecl (call);
> +
> +  if (!fndecl || DECL_BUILT_IN_CLASS (fndecl) != BUILT_IN_NORMAL)
> +    return false;
> +
> +  switch (DECL_FUNCTION_CODE (fndecl))
> +    {
> +      case BUILT_IN_FREE:
> +      case BUILT_IN_STACK_RESTORE:
> +	return false;
> +      default:
> +	return true;
> +    }
> +}

This doesn't look conservative enough.

First of all, I'd use gimple_call_builtin_p (call, BUILT_IN_NORMAL)
test.  And, the function should certainly conservatively return false
for all builtins that aren't (gimple_call_flags (call) & ECF_LEAF) != 0,
otherwise they might call hooks in the current CU and similar and free in
those hooks (likewise setjmp or similar, but again, that is not ECF_LEAF).

	Jakub

Hi,

On Wed, 6 Feb 2013, Jakub Jelinek wrote:

> First of all, I'd use gimple_call_builtin_p (call, BUILT_IN_NORMAL) 
> test.  And, the function should certainly conservatively return false 
> for all builtins that aren't (gimple_call_flags (call) & ECF_LEAF) != 0, 
> otherwise they might call hooks in the current CU and similar and free 
> in those hooks (likewise setjmp or similar, but again, that is not 
> ECF_LEAF).

Sure.  Consider the patch changed to introduce this function:

static bool
nonfreeing_call_p (gimple call)
{
  if (gimple_call_builtin_p (call, BUILT_IN_NORMAL)
      && gimple_call_flags (call) & ECF_LEAF)
    return true;

  return false;
}

Tested on the testcases and hmmer, okay after regstrapping?


Ciao,
Michael.

On Wed, Feb 6, 2013 at 3:20 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Wed, 6 Feb 2013, Jakub Jelinek wrote:
>
>> First of all, I'd use gimple_call_builtin_p (call, BUILT_IN_NORMAL)
>> test.  And, the function should certainly conservatively return false
>> for all builtins that aren't (gimple_call_flags (call) & ECF_LEAF) != 0,
>> otherwise they might call hooks in the current CU and similar and free
>> in those hooks (likewise setjmp or similar, but again, that is not
>> ECF_LEAF).
>
> Sure.  Consider the patch changed to introduce this function:
>
> static bool
> nonfreeing_call_p (gimple call)
> {
>   if (gimple_call_builtin_p (call, BUILT_IN_NORMAL)
>       && gimple_call_flags (call) & ECF_LEAF)
>     return true;
>
>   return false;
> }
>
> Tested on the testcases and hmmer, okay after regstrapping?

realloc can also effectively free memory.  But the above no longer
considers free () a freeing call as well?!  builtins.def also suggests
built_in_tm_free is to be handled as well.

That said, please enumerate freeing builtins even if they are
not ECF_LEAF at the moment - they might become such I suppose
(realloc is LEAF, free is not) - curiously there is no __builtin_in_tm_realloc.

Richard.

>
> Ciao,
> Michael.

Hi,

On Wed, 6 Feb 2013, Richard Biener wrote:

> realloc can also effectively free memory.  But the above no longer
> considers free () a freeing call as well?!

free is not ECF_LEAF, so is handled correctly, like tm_free.  I didn't 
consider realloc, it's indeed not handled.

> That said, please enumerate freeing builtins even if they are not 
> ECF_LEAF at the moment - they might become such I suppose (realloc is 
> LEAF, free is not) - curiously there is no __builtin_in_tm_realloc.

The function now reads:

--------------
static bool
nonfreeing_call_p (gimple call)
{
  if (gimple_call_builtin_p (call, BUILT_IN_NORMAL)
      && gimple_call_flags (call) & ECF_LEAF)
    switch (DECL_FUNCTION_CODE (gimple_call_fndecl (call)))
      {
        /* Just in case these become ECF_LEAF in the future.  */
        case BUILT_IN_FREE:
        case BUILT_IN_TM_FREE:
        case BUILT_IN_REALLOC:
        case BUILT_IN_STACK_RESTORE:
          return false;
        default:
          return true;
      }

  return false;
}
--------------

Testcase and hmmer work, okay after regstrapping?


Ciao,
Michael.

On Wed, Feb 6, 2013 at 4:07 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Wed, 6 Feb 2013, Richard Biener wrote:
>
>> realloc can also effectively free memory.  But the above no longer
>> considers free () a freeing call as well?!
>
> free is not ECF_LEAF, so is handled correctly, like tm_free.  I didn't
> consider realloc, it's indeed not handled.
>
>> That said, please enumerate freeing builtins even if they are not
>> ECF_LEAF at the moment - they might become such I suppose (realloc is
>> LEAF, free is not) - curiously there is no __builtin_in_tm_realloc.
>
> The function now reads:
>
> --------------
> static bool
> nonfreeing_call_p (gimple call)
> {
>   if (gimple_call_builtin_p (call, BUILT_IN_NORMAL)
>       && gimple_call_flags (call) & ECF_LEAF)
>     switch (DECL_FUNCTION_CODE (gimple_call_fndecl (call)))
>       {
>         /* Just in case these become ECF_LEAF in the future.  */
>         case BUILT_IN_FREE:
>         case BUILT_IN_TM_FREE:
>         case BUILT_IN_REALLOC:
>         case BUILT_IN_STACK_RESTORE:
>           return false;
>         default:
>           return true;
>       }
>
>   return false;
> }
> --------------
>
> Testcase and hmmer work, okay after regstrapping?

Ok.

Thanks,
Richard.

>
> Ciao,
> Michael.