[RFA] Fix tree-optimization/59919

As noted in the PR, we have a call to a non-returning function which 
occurs within a function that calls setjmp.

The non-returning call ends its containing basic block and there are no 
normal outgoing edges from that block.  Because the containing function 
calls setjmp, there is an abnormal edge from the block containing the 
non-returning call.

ie:

;;   basic block 2, loop depth 0, count 0, freq 10000, maybe hot
;;    prev block 0, next block 3, flags: (NEW, REACHABLE)
;;    pred:       ENTRY [100.0%]  (FALLTHRU,EXECUTABLE)
   bar (p_3(D));
;;    succ:       3 [100.0%]  (ABNORMAL,EXECUTABLE)

;;   basic block 3, loop depth 0, count 0, freq 10000, maybe hot
;;   Invalid sum of incoming frequencies 15000, should be 10000
;;    prev block 2, next block 4, flags: (NEW, REACHABLE, IRREDUCIBLE_LOOP)
;;    pred:       2 [100.0%]  (ABNORMAL,EXECUTABLE)
;;                3 [50.0%]  (ABNORMAL,DFS_BACK,IRREDUCIBLE_LOOP,EXECUTABLE)
   setjmp (_4(D));
   _6 = &p_3(D)->i;
   foo (_6);
;;    succ:       3 [50.0%]  (ABNORMAL,DFS_BACK,IRREDUCIBLE_LOOP,EXECUTABLE)
;;                4 [50.0%]  (FALLTHRU,EXECUTABLE)

;;   basic block 4, loop depth 0, count 0, freq 5000, maybe hot
;;    prev block 3, next block 1, flags: (NEW, REACHABLE)
;;    pred:       3 [50.0%]  (FALLTHRU,EXECUTABLE)
   return;

We want to insert an ASSERT_EXPR for the call to bar to assert that p_3 
is non-null (because of the non-null attribute)

The code to insert ASSERT_EXPRs doesn't know where to insert the 
ASSERT_EXPR and aborts.

This patch simply avoids registering ASSERT_EXPR insertions that arise 
as a result of non-returning calls.

Bootstrapped and regression tested on x86_64-unknown-linux-gnu.  OK for 
the trunk?

Jeff

On Thu, Jan 23, 2014 at 02:22:35PM -0700, Jeff Law wrote:
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -5891,8 +5891,13 @@ find_assert_locations_1 (basic_block bb, sbitmap live)
>  		    }
>  		}
>  
> -	      register_new_assert_for (op, op, comp_code, value, bb, NULL, si);
> -	      need_assert = true;
> +	      /* Do not register any assertions for a non-returning call.  */
> +	      if (!is_gimple_call (stmt) || !gimple_call_noreturn_p (stmt))
> +		{
> +		  register_new_assert_for (op, op, comp_code,
> +					   value, bb, NULL, si);
> +		  need_assert = true;
> +		}
>  	    }
>  	}

I'd say this belongs into infer_value_range instead.
It has:
  /* If STMT is the last statement of a basic block with no
     successors, there is no point inferring anything about any of its
     operands.  We would not be able to find a proper insertion point
     for the assertion, anyway.  */
  if (stmt_ends_bb_p (stmt) && EDGE_COUNT (gimple_bb (stmt)->succs) == 0)
    return false;
so I'd say it should instead do:
  if (stmt_ends_bb_p (stmt))
    {
      edge_iterator ei;
      edge e;

      FOR_EACH_EDGE (e, ei, gimple_bb (stmt)->succs)
	if (!(e->flags & EDGE_ABNORMAL))
	  break;
      if (e == NULL)
	return false;
    }
Because, there are e.g. two register_new_assert_for calls with
non-NULL bb and NULL e, so you'd need to handle this for both, etc.

	Jakub

On 01/24/14 03:52, Jakub Jelinek wrote:
> On Thu, Jan 23, 2014 at 02:22:35PM -0700, Jeff Law wrote:
>> --- a/gcc/tree-vrp.c
>> +++ b/gcc/tree-vrp.c
>> @@ -5891,8 +5891,13 @@ find_assert_locations_1 (basic_block bb, sbitmap live)
>>   		    }
>>   		}
>>
>> -	      register_new_assert_for (op, op, comp_code, value, bb, NULL, si);
>> -	      need_assert = true;
>> +	      /* Do not register any assertions for a non-returning call.  */
>> +	      if (!is_gimple_call (stmt) || !gimple_call_noreturn_p (stmt))
>> +		{
>> +		  register_new_assert_for (op, op, comp_code,
>> +					   value, bb, NULL, si);
>> +		  need_assert = true;
>> +		}
>>   	    }
>>   	}
>
> I'd say this belongs into infer_value_range instead.
Seems reasonable.  I'll spin that.

The only downside is we lose the ability to backward propagate through a 
typecast which feeds an argument in such a call.   But that's probably 
not a big deal.

Jeff

On Fri, Jan 24, 2014 at 09:35:10AM -0700, Jeff Law wrote:
> On 01/24/14 03:52, Jakub Jelinek wrote:
> >On Thu, Jan 23, 2014 at 02:22:35PM -0700, Jeff Law wrote:
> >>--- a/gcc/tree-vrp.c
> >>+++ b/gcc/tree-vrp.c
> >>@@ -5891,8 +5891,13 @@ find_assert_locations_1 (basic_block bb, sbitmap live)
> >>  		    }
> >>  		}
> >>
> >>-	      register_new_assert_for (op, op, comp_code, value, bb, NULL, si);
> >>-	      need_assert = true;
> >>+	      /* Do not register any assertions for a non-returning call.  */
> >>+	      if (!is_gimple_call (stmt) || !gimple_call_noreturn_p (stmt))
> >>+		{
> >>+		  register_new_assert_for (op, op, comp_code,
> >>+					   value, bb, NULL, si);
> >>+		  need_assert = true;
> >>+		}
> >>  	    }
> >>  	}
> >
> >I'd say this belongs into infer_value_range instead.
> Seems reasonable.  I'll spin that.
> 
> The only downside is we lose the ability to backward propagate
> through a typecast which feeds an argument in such a call.   But
> that's probably not a big deal.

I couldn't actually reproduce any backwards propagation,
since now all pointer casts are useless, so the code to walk the pointer
casts is likely just dead.

	Jakub

On 01/24/14 09:41, Jakub Jelinek wrote:
>>
>> The only downside is we lose the ability to backward propagate
>> through a typecast which feeds an argument in such a call.   But
>> that's probably not a big deal.
>
> I couldn't actually reproduce any backwards propagation,
> since now all pointer casts are useless, so the code to walk the pointer
> casts is likely just dead.
I ran across some not terribly long ago and added similar code to DOM. 
You just need to try harder :-)


jeff