Re: [RFA][PATCH][PR middle-end/61118] Improve tree CFG accuracy for setjmp/longjmp

[Jeff Law <law at redhat dot com>]

On 02/28/2018 03:48 AM, Richard Biener wrote:
> On Wed, Feb 28, 2018 at 11:43 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Feb 28, 2018 at 1:16 AM, Jeff Law <law@redhat.com> wrote:
>>> Richi, you worked on 57147 which touches on the issues here.  Your
>>> thoughts would be greatly appreciated.
>>>
>>>
>>> So 61118 is one of several bugs related to the clobbered-by-longjmp warning.
>>>
>>> In 61118 is we are unable to coalesce all the objects in the key
>>> partitions.  To remove the relevant PHIs we have to create two
>>> assignments to the key pseudos.
>>>
>>> Pseudos with more than one assignment are subject to the
>>> clobbered-by-longjmp analysis:
>>>
>>>  * True if register REGNO was alive at a place where `setjmp' was
>>>    called and was set more than once or is an argument.  Such regs may
>>>    be clobbered by `longjmp'.  */
>>>
>>> static bool
>>> regno_clobbered_at_setjmp (bitmap setjmp_crosses, int regno)
>>> {
>>>   /* There appear to be cases where some local vars never reach the
>>>      backend but have bogus regnos.  */
>>>   if (regno >= max_reg_num ())
>>>     return false;
>>>
>>>   return ((REG_N_SETS (regno) > 1
>>>            || REGNO_REG_SET_P (df_get_live_out (ENTRY_BLOCK_PTR_FOR_FN
>>> (cfun)),
>>>                                regno))
>>>           && REGNO_REG_SET_P (setjmp_crosses, regno));
>>> }
>>>
>>>
>>> The fact that no path sets the pseudo more than once is not considered.
>>> If there is more than one static set of the pseudo, then it is
>>> considered for possible warning.
>>>
>>> --
>>>
>>>
>>> I looked at the propagations which led to the inability to coalesce.
>>> They all seemed valid to me.  We have always allowed copy propagation to
>>> replace one pseudo with another as long as neither has
>>> SSA_NAME_USED_IN_ABNORMAL_PHI set.
>>>
>>> We have a PHI like
>>>
>>> x1(ab) = (x0, x3 (ab))
>>>
>>> x0 is not marked as abnormal because the edge isn't abnormal and thus we
>>> can propagate into the x0 argument of the PHI.  This is consistent with
>>> behavior since, well, forever.   We propagate a value for x0 resulting
>>> in something like
>>>
>>> x1(b) = (y0, x3 (ab))
>>>
>>>
>>> Where y0 is still live across the PHI.  Thus the partition for x1/x3,
>>> etc conflicts with the partition for y0 and they can not be coalesced.
>>> This leads to the multiple assignments to the pseudo for the x1/x3
>>> partition.  I briefly looked marking all the PHI arguments as abnormal
>>> when the destination is abnormal, but it just doesn't seem right.
>>>
>>> Anyway, I'd already been looking at 21161 and was aware that the CFG's
>>> we're building in presence of setjmp/longjmp were slightly inaccurate.
>>>
>>> In particular, a longjmp returns to the point immediately after the
>>> setjmp, not to the setjmp itself.  But our CFG building has the edge
>>> from the abnormal dispatcher going to the block containing the setjmp call.
>>
>> Yeah...  for SJLJ EH we get this right via __builtin_setjmp_receiver.
>>
>>> This creates unnecessary irreducible loops.  It turns out that if we fix
>>> the tree CFG, then lifetimes become more accurate (and more
>>> constrained).  The more constrained, more accurate lifetime information
>>> is enough to allow things to coalesce the way we want and everything for
>>> 61118 just works.
>>
>> Sounds good.
> 
> Oh - and to mention it, we have one long-standing issue after me trying
> to fix things here which is that RTL doesn't have all those abnormal edges
> for setjmp and friends because we throw away abnormal edges during
> RTL expansion and expect to recompute them...
> 
> IIRC there's a bugreport about this to track this issue and the fix is57147
> to stop removing abnormal edges and instread transition them to RTL:
> 
>           /* At the moment not all abnormal edges match the RTL
>              representation.  It is safe to remove them here as
>              find_many_sub_basic_blocks will rediscover them.
>              In the future we should get this fixed properly.  */
>           if ((e->flags & EDGE_ABNORMAL)
>               && !(e->flags & EDGE_SIBCALL))
>             remove_edge (e);
> 
> not sure if we can use an edge flag to mark those we want to preserve.
> But I don't understand the comment very well - why would any abnormal
> edges not "match" the RTL representation?
Yea, I was a bit surprised when I found out the lack of coordination
between gimple and RTL on this stuff.  I got as far as realizing that
the gimple edges related to setjmp/longjmp were dropped on the floor
when looking at 21161.  Fixing the CFG in gimple made absolutely no
difference and surprised the hell out of me.

Luckily I had the code stashed and could resurrect it when I realized
61118 was a problem with life analysis/coalescing caused by the slight
incorrectness in the gimple CFG.

For 21161 we end up needing to scan the RTL after the call to find the
condjump.  Then we figure out where the condjump goes.  That allows us
to look at the registers live on the longjmp path as opposed to what's
live at the setjmp call.  That's about 95% complete -- I fat fingered
and lost the sparc/s390 specific bits, so I need to recreate those.
Ultimately, I can fix 21161 on all targets but mips (which have an
amazingly annoying unspec between the setjmp call and the conditional
branch on its result).



WRT 57147, the test that I'm removing was something you derived from the
original plus inspection of tree-cfg.c.  The test for the original issue
is pr57147-1.c and I had already verified that we continue to do the
right thing for it -- I probably should have noted that in the patch
submission.

jeff