Re: [PATCH] Fix PR80776

[Jeff Law <law@redhat.com>]

On 11/28/2017 02:14 AM, Richard Biener wrote:
> On Mon, 27 Nov 2017, Jeff Law wrote:
> 
>> On 11/27/2017 06:39 AM, Richard Biener wrote:
>>>
>>> The following avoids -Wformat-overflow false positives by teaching
>>> EVRP the trick about __builtin_unreachable () "other" edges and
>>> attaching range info to SSA names.  EVRP does a better job in keeping
>>> ranges for every SSA name from conditional info (VRP "optimizes" its
>>> costly ASSERT_EXPR insertion process).
>>>
>>> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
>>>
>>> This will also fix the testcase from PR83072 but it doesn't really
>>> fix all cases I want to fix with a fix for it.  OTOH it might be
>>> this is enough for stage3.
>>>
>>> Richard.
>>>
>>> 2017-11-27  Richard Biener  <rguenther@suse.de>
>>>
>>> 	PR tree-optimization/80776
>>> 	* gimple-ssa-evrp-analyze.h (evrp_range_analyzer::set_ssa_range_info):
>>> 	Declare.
>>> 	* gimple-ssa-evrp-analyze.c (evrp_range_analyzer::set_ssa_range_info):
>>> 	New function.
>>> 	(evrp_range_analyzer::record_ranges_from_incoming_edges):
>>> 	If the incoming edge is an effective fallthru because the other
>>> 	edge only reaches a __builtin_unreachable () then record ranges
>>> 	derived from the controlling condition in SSA info.
>>> 	(evrp_range_analyzer::record_ranges_from_phis): Use set_ssa_range_info.
>>> 	(evrp_range_analyzer::record_ranges_from_stmt): Likewise.
>>>
>>> 	* gcc.dg/pr80776-1.c: New testcase.
>>> 	* gcc.dg/pr80776-2.c: Likewise.
>> So the thing to make sure of here is that the range information we
>> reflect back into the SSA_NAME actually applies everywhere the SSA_NAME
>> can appear.  ie, it's globally valid.
>>
>> This means we can't reflect anything we derive from conditionals or
>> things like a *p making the range non-null back to the SSA_NAME.
>>
>> I'd be concerned about the change to record_ranges_from_incoming_edge.
> 
> It's basically a copy of what VRP does when removing range assertions.
> I've added the correctness check I missed and also the trick with
> setting nonzero bits.
> 
> This causes us to no longer handle the gcc.dg/pr80776-1.c case:
> 
>   <bb 2> :
>   i_4 = somerandom ();
>   if (i_4 < 0)
>     goto <bb 3>; [INV]
>   else
>     goto <bb 4>; [INV]
> 
>   <bb 3> :
>   __builtin_unreachable ();
> 
>   <bb 4> :
>   i.0_1 = (unsigned int) i_4;
>   if (i.0_1 > 999999)
>     goto <bb 5>; [INV]
>   else
>     goto <bb 6>; [INV]
> 
>   <bb 5> :
>   __builtin_unreachable ();
> 
>   <bb 6> :
>   _7 = __builtin___sprintf_chk (&number, 1, 7, "%d", i_4);
> 
> when trying to update the SSA range info for i_4 from the
> if (i.0_1 > 999999) we see the use in the dominating condition
> and thus conclude we cannot update the SSA range info like we want.
So the unreachables add a twist.  If we assume the unreachables are in
fact unreachable, then ISTM we can use the conditionals to derive
refined ranges on the other edge.  The implications of the unreachable
didn't really sink in until just now.

> 
> ifcombine also doesn't merge the tests because I think it gets
> confused by the __builtin_unreachable ().  ifcombine also runs after
> VRP1 which gets rid of the __builtin_unreachable ()s.
Yea.  As you know I was just poking at a case yesterday where ifcombine
worked sub-optimally and its placement was inconvenient...  But given
the particular case I was looking at it made more sense to simplify the
IL prior to ifcombine.

> 
> I think for GCC 9 we may want to experiment with moving ifcombine
> before VRP1 and handling if-chains with __builtin_unreachable ()s.
Worth investigation.

Jeff