This is the mail archive of the gcc-patches@gcc.gnu.org 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] Fix PR34263: Cleaning up latch blocks


> Following the RFC message
> (http://gcc.gnu.org/ml/gcc/2007-12/msg00015.html), this patch tries
> to clean up latch blocks in the out-of-ssa phase for the purpose of
> restoring single-basic-block loops. This can be helpful not only in the
> SMS perspective. Given the fact we are in stage 3 I can alternatively
> address this inside the SMS (and not in out-of-ssa).

> The patch was successfully tested a week ago (when I posted the RFC
> message) on ppc64 and I am re-checking it now to verify it is still OK.

> OK for mainline once testing completes?

So If I get this right, This patch addresses extra blocks that are created in circumstances like:

BB6:
a_2 = PHI <a_3(4), a_4(6)>
<...>
a_4 = expr
if (x) then BB6 else BB7
BB7:

which we currently translate to something like:

a_2 = a_3
BB6:
<...>
a_4 = expr
if (x) then BB14 else BB7
BB14:
a_2 = a_4
goto BB6:
BB7:

and with this change, we would then generate:
a_2 = a_3
BB6:
<...>
a_4 = expr
a_9 = a_2
a_2 = a_4
if (x) then BB6 else BB7
BB7:
a_2 = a_9

Effectively stashing the value of a_2 to load back in the case where we exit. Is this right? (except it is actually working on the coalesced root variables, not the actual SSA_NAMES)

Seems to me there is an older PR that involves a similar situation too , but I dont recall it.

The patch looks fine, except I would ask that all this logic be lifted out to a function which returns TRUE if it is a single block loop and performs the work. So the code looks something like:

if (single_edge)
{
/* Add stmts to the edge unless processed specially as a single-block loop latch edge. */
if (!process_single_block_loop_latch (single_edge)) bsi_commit_one_edge_insert (single_edge, NULL);
}


And a few extra comments in the body would help to read it too, just simple things like:


/* Find the successor edge which is not the latch edge. */
+ FOR_EACH_EDGE (e, ei, b_loop->succs)
+ if (e->dest != b_loop)
+ break;
+
+ b_exit = e->dest;
+
/* Check that the exit block has only the loop as a predecessor, and that there
are no pending stmts on that edge as well. */ + if (EDGE_COUNT (b_exit->preds) != 1 || PENDING_STMT (e))
+ do_it = false;
+
/* Find the predecessor edge which is used for loop entry. */ + FOR_EACH_EDGE (e, ei, b_loop->preds)
+ if (e->src != b_loop)
+ break;
+



It just makes it a little easier to read.


Should there be some kind of limit to the number of copies being inserted? Ie, if there are 10 copies being inserted on the edge, I would think 10 extra copies in the loop would tend to start adding up and offset whatever gain you might get.

As long as there isn't disagreement on whether the extra copies in and out of the loop are a problem, the patch istself is OK.

Andrew


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