Sink clobbers if EH block contains just clobbers (PR tree-optimization/51117)

Hello,

On Fri, 9 Dec 2011, Jakub Jelinek wrote:

> I had to tweak a little bit the expander conflict checking, because
> if we have a BB with two incoming EH edges and clobber stmts from both
> sunk into its beginning, then it would consider both variables (a and b
> above) to be live at the same time, while there is no insn on which they
> can actually live at the same time, the PHIs don't mention either of them
> (and after all, PHIs aren't memory loads), and after the PHIs we have
> immediately the clobbers.

The idea is sound, the implementation can be tidied with the observation 
that only the first real instruction (instead of the BB start) is the 
point at which all currently live things need to be conflicted, like in 
the patch below (only cfgexpand.c part changed).  I.e. moving the existing 
code from add_scope_clobbers_1 a bit is enough.  I'm putting this through 
regstrapping on x86_64-linux and will commit if that succeeds, given rths 
approval for the other parts.

I wonder how to best test this.


Ciao,
Michael.
	PR tree-optimization/51117
	* tree-eh.c (sink_clobbers): New function.
	(execute_lower_eh_dispatch): Call it for BBs ending with
	internally throwing RESX.
	* cfgexpand.c (add_scope_conflicts_1): Add all conflicts only
	at the first real instruction.

On Mon, Dec 12, 2011 at 03:46:01PM +0100, Michael Matz wrote:
> > I had to tweak a little bit the expander conflict checking, because
> > if we have a BB with two incoming EH edges and clobber stmts from both
> > sunk into its beginning, then it would consider both variables (a and b
> > above) to be live at the same time, while there is no insn on which they
> > can actually live at the same time, the PHIs don't mention either of them
> > (and after all, PHIs aren't memory loads), and after the PHIs we have
> > immediately the clobbers.
> 
> The idea is sound, the implementation can be tidied with the observation 
> that only the first real instruction (instead of the BB start) is the 
> point at which all currently live things need to be conflicted, like in 
> the patch below (only cfgexpand.c part changed).  I.e. moving the existing 
> code from add_scope_clobbers_1 a bit is enough.  I'm putting this through 
> regstrapping on x86_64-linux and will commit if that succeeds, given rths 
> approval for the other parts.

Looks cleaner, yes.
Just I wonder:
1) what if a bb contains no real insns (I know, they should be optimized
   out, but do we assert that etc.?) - then the EXECUTE_IF_SET_IN_BITMAP
   loop just wouldn't be done.  Perhaps that is fine, it would make it
   into the bitmap at the end of the bb and perhaps following bb would
   do this loop.
2) the PHIs are then handled always with visit_op instead of visit_conflict,
   I'd guess the needed add_stack_var_conflict calls would then happen
   in that EXECUTE_IF_SET_IN_BITMAP loop, right?

> I wonder how to best test this.

One kind of testing was watching the size of .gcc_except_table going down
with each patch (vanilla -> optimize_cloobers -> optimize_clobbers thinko
fix -> sink_clobbers).

The following test unfortunately succeeds already with current trunk
(i.e. tests already the optimize_clobbers optimization), and with
throw; instead of return 1; it contains __cxa_rethrow call even
with the sink optimization and the difference is mainly that
.gcc_except_table is smaller and __cxa_rethrow block is reachable just from
one spot (recorded in .gcc_except_table), while without sink_clobbers
there is also a jump to it from another spot.  For the
s/return 1/throw/ testcase we could perhaps
scan-tree-dump-not the optimized dump for __builtin_eh_copy_values.

// { dg-do compile }
// { dg-options "-O2 -fexceptions" }

struct A { char buf[64]; };
void bar (A *);

int
foo ()
{
  A c;
  bar (&c);
  try
  {
    {
      A a;
      bar (&a);
      if (a.buf[13])
	throw 1;
      else if (a.buf[52])
	throw 3;
    }
    {
      A b;
      bar (&b);
      if (b.buf[13])
	throw 2;
    }
  }
  catch ( ...)
  {
    return 1;
  }
  return 0;
}

// { dg-final { scan-assembler-not "__cxa_rethrow" } }


	Jakub

Hi,

On Mon, 12 Dec 2011, Jakub Jelinek wrote:

> Looks cleaner, yes.
> Just I wonder:
> 1) what if a bb contains no real insns (I know, they should be optimized
>    out, but do we assert that etc.?) - then the EXECUTE_IF_SET_IN_BITMAP
>    loop just wouldn't be done.  Perhaps that is fine, it would make it
>    into the bitmap at the end of the bb and perhaps following bb would
>    do this loop.

Not only perhaps.  That is exactly what will happen.  If some of the 
successor BBs then has real instructions _that_ one will cause creation of 
all the necessary conflicts.

> 2) the PHIs are then handled always with visit_op instead of visit_conflict,
>    I'd guess the needed add_stack_var_conflict calls would then happen
>    in that EXECUTE_IF_SET_IN_BITMAP loop, right?

Correct.  The PHIs don't need to create the conflicts, any new mention of 
a DECL name will be noted as active, and then creates a conflict at the 
next real instruction (if not cancelled by a clobber before).

> > I wonder how to best test this.
> 
> One kind of testing was watching the size of .gcc_except_table going down
> with each patch (vanilla -> optimize_cloobers -> optimize_clobbers thinko
> fix -> sink_clobbers).

My idea was to somehow check the EH tree for some dump (.ehcleanup2 
perhaps) for being in the expected form, or that the correct number of 
removals happen.  And of course that the sharing still happens, but that's 
even worse to test in the .expand dump :-/


Ciao,
Michael.

On Mon, Dec 12, 2011 at 05:29:09PM +0100, Michael Matz wrote:
> On Mon, 12 Dec 2011, Jakub Jelinek wrote:
> > Just I wonder:
> > 1) what if a bb contains no real insns (I know, they should be optimized
> >    out, but do we assert that etc.?) - then the EXECUTE_IF_SET_IN_BITMAP
> >    loop just wouldn't be done.  Perhaps that is fine, it would make it
> >    into the bitmap at the end of the bb and perhaps following bb would
> >    do this loop.
> 
> Not only perhaps.  That is exactly what will happen.  If some of the 
> successor BBs then has real instructions _that_ one will cause creation of 
> all the necessary conflicts.

Ok.

> > 2) the PHIs are then handled always with visit_op instead of visit_conflict,
> >    I'd guess the needed add_stack_var_conflict calls would then happen
> >    in that EXECUTE_IF_SET_IN_BITMAP loop, right?
> 
> Correct.  The PHIs don't need to create the conflicts, any new mention of 
> a DECL name will be noted as active, and then creates a conflict at the 
> next real instruction (if not cancelled by a clobber before).

Ok.  So, I'm happy with your changes and rth already acked the tree-eh.c
side, so can we just get an ack on these cfgexpand.c changes?  Thanks.

The testcases can perhaps follow when they are ready (and I plan to submit
at least the one checking for no __cxa_rethrow in assembly).

	Jakub

Hi,

On Mon, 12 Dec 2011, Jakub Jelinek wrote:

> Ok.  So, I'm happy with your changes and rth already acked the tree-eh.c 
> side, so can we just get an ack on these cfgexpand.c changes?  Thanks.

Hmpf, I would have simply committed without a re-approval, but if you 
think it's necessary I'll wait.

FYI, I've actually regstrapped with the patch starting iterating from i+1 
in the nested EXECUTE_IF_SET_IN_BITMAP to ignore the diagonal.


Ciao,
Michael.

On 12/12/2011 09:03 AM, Michael Matz wrote:
> Hmpf, I would have simply committed without a re-approval, but if you 
> think it's necessary I'll wait.

The revised patch is ok too.


r~