Re: [PATCH v2] combine: perform jump threading at the end

[Ilya Leoshkevich <iii at linux dot ibm dot com>]

> Am 06.09.2018 um 20:11 schrieb Jeff Law <law@redhat.com>:
> 
> On 09/05/2018 06:11 AM, Richard Biener wrote:
>> On Wed, Sep 5, 2018 at 2:01 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>>> 
>>> gcc/ChangeLog:
>>> 
>>> 2018-09-05  Ilya Leoshkevich  <iii@linux.ibm.com>
>>> 
>>>        PR target/80080
>>>        * combine.c (rest_of_handle_combine): Perform jump threading.
>>> 
>>> gcc/testsuite/ChangeLog:
>>> 
>>> 2018-09-05  Ilya Leoshkevich  <iii@linux.ibm.com>
>>> 
>>>        PR target/80080
>>>        * gcc.target/s390/pr80080-4.c: New test.
>>> ---
>>> gcc/combine.c                             | 10 ++++++++--
>>> gcc/testsuite/gcc.target/s390/pr80080-4.c | 16 ++++++++++++++++
>>> 2 files changed, 24 insertions(+), 2 deletions(-)
>>> create mode 100644 gcc/testsuite/gcc.target/s390/pr80080-4.c
>>> 
>>> diff --git a/gcc/combine.c b/gcc/combine.c
>>> index a2649b6d5a1..818b4c5b77d 100644
>>> --- a/gcc/combine.c
>>> +++ b/gcc/combine.c
>>> @@ -14960,10 +14960,16 @@ rest_of_handle_combine (void)
>>>        free_dominance_info (CDI_DOMINATORS);
>>>       timevar_push (TV_JUMP);
>>>       rebuild_jump_labels (get_insns ());
>>> -      cleanup_cfg (0);
>>> -      timevar_pop (TV_JUMP);
>>>     }
>>> 
>>> +  /* Combining insns can change basic blocks in a way that they end up
>>> +     containing a single jump_insn. This creates an opportunity to improve code
>>> +     with jump threading.  */
>>> +  cleanup_cfg (CLEANUP_THREADING);
>>> +
>>> +  if (rebuild_jump_labels_after_combine)
>>> +    timevar_pop (TV_JUMP);
>> 
>> cleanup_cfg pushes its own timevar so it doesn't make sense to try covering it
>> with TV_JUMP.  And rebuild_jump_labels immediately pushes TV_REBUILD_JUMP.
>> 
>> So I suggest to remove the timevar_push/pop of TV_JUMP here.
>> 
>> No comment in general about the change, maybe we can detect transforms that
>> make jump-threading viable and conditionalize that properly?  Note the only
>> setter of CLEANUP_THREADING guards it with flag_thread_jumps so maybe better
>> do it above as well (avoids cost at -O0 for example).
> The sad thing is I thought we'd killed the RTL jump threading code eons ago.
Do you mean RTL jump threading is deprecated and/or we better rely on
something else to achieve the same results?
> 
> THe RTL jump threading code tries to prove that the target block has no
> side effects and that we can statically determine the true/false
> condition for the conditional branch at the end of the block.
> 
> This is (of course) much easier to do when the target block has no insns
> other than the conditional branch.  So perhaps only do this when the
> target block has just the conditional?
Sounds reasonable, I implemented this in the new patch.  The only thing
is that combine leaves (note NOTE_INSN_DELETED) around, so I needed to
also account for those.  I used side_effects_p for that.
> 
> Hard to know if that'd work here since RTL wasn't posted.
I will post the RTL with the updated patch shortly.
> 
> Jeff