Re: [PATCH] Don't clobber dominator info in the combiner (PR target/77526)

[Jakub Jelinek]

On Fri, Sep 16, 2016 at 08:47:28AM +0200, Richard Biener wrote:
> On Fri, 16 Sep 2016, Jakub Jelinek wrote:
> 
> > Hi!
> > 
> > As mentioned in the PR, combiner sometimes calls
> > purge_all_dead_edges or purge_dead_edges that can invalidate the dominator
> > info if it is computed.  Other passes like CSE in that case free the
> > dominance info, this patch does the same in the combiner.
> > 
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> Ok.  (I wonder if/why cleanup_cfg (0) doesn't do this then?)

cleanup_cfg only frees it in some cases if it does some kind of changes:
  /* ???  We probably do this way too often.  */
  if (current_loops
  && (changed
  || (mode & CLEANUP_CFG_CHANGED)))
{
...
  calculate_dominance_info (CDI_DOMINATORS);
But here the dominator info gets out of sync earlier, in particular on this
testcase in the
  new_direct_jump_p |= purge_all_dead_edges ();
call.

Perhaps ideally we should teach all these functions to update the dominator
info, but I'm afraid it is a lot of work.

Jakub

Re: [PATCH] Don't clobber dominator info in the combiner (PR target/77526)

[Richard Biener]

On Fri, 16 Sep 2016, Jakub Jelinek wrote:

> Hi!
> 
> As mentioned in the PR, combiner sometimes calls
> purge_all_dead_edges or purge_dead_edges that can invalidate the dominator
> info if it is computed.  Other passes like CSE in that case free the
> dominance info, this patch does the same in the combiner.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.  (I wonder if/why cleanup_cfg (0) doesn't do this then?)

Thanks,
Richard.

> 2016-09-16  Jakub Jelinek  
> 
>   PR target/77526
>   * combine.c (rest_of_handle_combine): If any edges have been purged,
>   free dominators if available.
> 
>   * gcc.target/i386/pr77526.c: New test.
> 
> --- gcc/combine.c.jj  2016-08-12 17:33:46.0 +0200
> +++ gcc/combine.c 2016-09-15 16:12:26.154982064 +0200
> @@ -14393,6 +14393,8 @@ rest_of_handle_combine (void)
>   instructions.  */
>if (rebuild_jump_labels_after_combine)
>  {
> +  if (dom_info_available_p (CDI_DOMINATORS))
> + free_dominance_info (CDI_DOMINATORS);
>timevar_push (TV_JUMP);
>rebuild_jump_labels (get_insns ());
>cleanup_cfg (0);
> --- gcc/testsuite/gcc.target/i386/pr77526.c.jj2016-09-15 
> 16:13:37.149105476 +0200
> +++ gcc/testsuite/gcc.target/i386/pr77526.c   2016-09-15 16:13:13.0 
> +0200
> @@ -0,0 +1,13 @@
> +/* PR target/77526 */
> +/* { dg-do compile { target int128 } } */
> +/* { dg-options "-Os -fno-forward-propagate -fno-gcse 
> -fno-rerun-cse-after-loop -mstringop-strategy=byte_loop -Wno-psabi" } */
> +
> +typedef char U __attribute__((vector_size(64)));
> +typedef __int128 V __attribute__((vector_size(64)));
> +
> +V
> +foo (int a, int b, __int128 c, U u)
> +{
> +  u = (u >> (u & 7)) | (u << -(u & 7));
> +  return a + b + c + (V)u;
> +}
> 
>   Jakub
> 
> 

-- 
Richard Biener 
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 
21284 (AG Nuernberg)

Re: [PATCH] Don't clobber dominator info in the combiner (PR target/77526)

[Jakub Jelinek]

On Thu, Sep 15, 2016 at 06:20:11PM -0500, Segher Boessenkool wrote:
> On Fri, Sep 16, 2016 at 12:50:44AM +0200, Jakub Jelinek wrote:
> > As mentioned in the PR, combiner sometimes calls
> > purge_all_dead_edges or purge_dead_edges that can invalidate the dominator
> > info if it is computed.  Other passes like CSE in that case free the
> > dominance info, this patch does the same in the combiner.
> > 
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> Okay, but does it need the same for the post-dominators?  Oh, those
> never exist longer than a single pass?

Yeah, indeed, passes that compute post-dominators are required to free them
as well, and combine doesn't.

Jakub

Re: [PATCH] Don't clobber dominator info in the combiner (PR target/77526)

[Segher Boessenkool]

Hi Jakub,

On Fri, Sep 16, 2016 at 12:50:44AM +0200, Jakub Jelinek wrote:
> As mentioned in the PR, combiner sometimes calls
> purge_all_dead_edges or purge_dead_edges that can invalidate the dominator
> info if it is computed.  Other passes like CSE in that case free the
> dominance info, this patch does the same in the combiner.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Okay, but does it need the same for the post-dominators?  Oh, those
never exist longer than a single pass?

Thanks,


Segher