This is the mail archive of the
mailing list for the GCC project.
Re: [patch] for PR 27735
- From: Eric Botcazou <ebotcazou at libertysurf dot fr>
- To: Zdenek Dvorak <rakdver at atrey dot karlin dot mff dot cuni dot cz>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Thu, 10 Aug 2006 12:12:58 +0200
- Subject: Re: [patch] for PR 27735
- References: <20060524095159.GA14182@atrey.karlin.mff.cuni.cz>
[Testing the boundaries of my recent "RTL optimizers" maintainership :-)]
> 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
- 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)
! 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. */
> * 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 <email@example.com> */
/* Testcase by Andrew Pinski <firstname.lastname@example.org> */
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.