Re: [PATCH 4/5] Don't mark targets of unconditional jumps with side effects as FALLTHRU.

["Marcin Kościelnicki" <koriakin@0x04.net>]

On 22/01/16 08:44, Andreas Krebbel wrote:
> On 01/22/2016 12:10 AM, Jeff Law wrote:
>> On 01/21/2016 03:05 AM, Andreas Krebbel wrote:
>>> On 01/02/2016 08:16 PM, Marcin Kościelnicki wrote:
>>>> When an unconditional jump with side effects targets an immediately
>>>> following label, rtl_tidy_fallthru_edge is called.  Since it has side
>>>> effects, it doesn't remove the jump, but the label is still marked
>>>> as fallthru.  This later causes a verification error.  Do nothing in this
>>>> case instead.
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 	* cfgrtl.c (rtl_tidy_fallthru_edge): Bail for unconditional jumps
>>>> 	with side effects.
>>>
>>> The change looks ok to me (although I'm not able to approve it). Could you please run regressions
>>> tests on x86_64 with that change?
>>>
>>> Perhaps a short comment in the code would be good.
>> I think the patch is technically fine, the question is does it fix a
>> visible bug?  I read the series as new feature enablement so I put this
>> patch into my gcc7 queue.
>
> We need the patch for the S/390 split-stack implementation which we would like to see in GCC 6.  I'm
> aware that this isn't stage 3 material but people seem to have reasons to really want split stack on
> S/390 asap and we would have to backport this feature anyway. Therefore I would prefer to have it in
> the official release already. That's the only common code change we would need for that.
>
> I've started a bootstrap and regression test for the patch also on Power.
>
> Do you see a chance we can get this into GCC 6?
>
> Bye,
>
> -Andreas-
>

I've tested the patch on x86_64, no regressions.

I'm not entirely sure if the patch needs to go in for the current 
version of split-stack support.

This patch fixed a showstopper bug on g5 CPUs when the patch still 
supported them.  I haven't seen this bug with the z900 sequences (which 
are now the only ones left), but since we're still using unconditional 
jumps with side effects, I left it in just to be safe.  The testsuite 
passes on s390x -fsplit-stack both with the patch and without it.

So, I don't know.  It seems to work now, probably because no 
optimization pass has a reason to touch that jump, but it may start to 
fail if someone adds a new optimization that tries to be smart with our 
prologue.