This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] for PR 27735

[Testing the boundaries of my recent "RTL optimizers" maintainership :-)]

Hi Zdenek,

> This would be fairly easy to fix by handling this special case;
> nevertheless, the problem with the code for updating the
> EDGE_IRREDUCIBLE_LOOP flag is that it is fairly complicated, and at the
> same time, it handles a quite rare case, so it does not really get a decent
> testing coverage.  I thus consider it safer to propose the following patch
> that makes us simply recompute the flag from scratch whenever we detect
> that EDGE_IRREDUCIBLE_LOOP information might get invalidated.

You're the specialist, but that sounds like a sensible approach.

> 	* cfgloop.h (fix_loop_placement): Declaration removed.
> 	* loop-unswitch.c (unswitch_loop): Do not call fix_loop_placement.

Does this change belong here?

> 	* cfgloopmanip.c (fix_loop_placements, fix_bb_placements, unloop):
> 	Keep track of whether an irreducible region was affected.

It seems that unloop was already using the recompute-if-changed approach for 
the EDGE_IRREDUCIBLE_LOOP flag.  But AFAICS you're changing the trigger from
exit edges

-   edges = get_loop_exit_edges (loop, &num_edges);
!   /* If the loop was inside an irreducible region, we would have to somehow
!      update the irreducible marks inside its body.  While it is certainly
!      possible to do, it is a bit complicated and this situation should be
!      very rare, so we just remark all loops in this case.  */
!   for (i = 0; i < num_edges; i++)
!     if (edges[i]->flags & EDGE_IRREDUCIBLE_LOOP)
!       break;
!   if (i != num_edges)
!     mark_irreducible_loops (loops);

to the preheader edge

!   if (loop_preheader_edge (loop)->flags & EDGE_IRREDUCIBLE_LOOP)
!     *update_irred = true;

While this seems correct given the properties of the preheader edge, I think 
the change should be mentioned in the ChangeLog.

I also think that UPDATE_IRRED is not a very good name for the new "out" 
parameter of fix_loop_placements, fix_bb_placements and unloop.  I'd rather 
use something with CHANGED in it, like for try_remove_empty_loop.  That the 3 
functions get a new parameter must also be mentioned in the ChangeLog and 
remove_path should have "Adjust calls to above functions" or something like 

A last minor nit:

!    If this may cause the information about irreducible regions become
!    invalid, UPDATE_IRRED is set to true.  */

"to become".

> 	* gcc.dg/pr27735.c: New test.

I'm from the old school so I think dg testcases should have a descriptive name 
(for example loop-unswitch-1.c), explicitly mention the PR and have a header 
acknowledging other contributions:

/* PR rtl-optimization/27735 */
/* Reported by dcb <> */
/* Testcase by Andrew Pinski <> */

So, putting aside the ChangeLog and naming nits, the patch looks OK to me 
modulo the fix_loop_placement related changes.

Thanks for tackling this.

Eric Botcazou

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]