RFA: Fix PR rtl-optimization/60651

Message ID 1483548.tKn4spAyxv@polaris
State New
Headers show

Commit Message

Eric Botcazou March 28, 2014, 10:20 a.m.
> However, the first call is for blocks with incoming abnormal edges.
> If these are empty, the change as I wrote it yesterday is fine, but not
> when they are non-empty; in that case, we should indeed insert before the
> first instruction in that block.

OK, so the issue is specific to empty basic blocks and boils down to inserting 
instructions in a FIFO manner into them.

> This can be archived by finding an insert-before position using NEXT_INSN
> on the basic block head; this amounts to the very same insertion place
> as inserting after the basic block head.  Also, we will continue to set no
> location, and use the same bb, because both add_insn_before and
> add_insn_after (in contradiction to its block comment) will infer the basic
> block from the insn given (in the case for add_insn_before, I assume
> that the basic block doesn't start with a BARRIER - that would be invalid -
> and that the insn it starts with has a valid BLOCK_FOR_INSN setting the
> same way the basic block head has.

This looks reasonable, but I think that we need more commentary because it's 
not straightforward to understand, so I would:

  1. explicitly state that we enforce an order on the entities in addition to 
the order on priority, both in the code (for example create a 4th paragraph in 
the comment at the top of the file, before "More details ...") and in the doc 
as you already did, but "ordering" the two orders for the sake of clarity: 
first the order on priority then, for the same priority, the order to the 

  2. add a line in the head comment of new_seginfo saying that INSN may not be 
a NOTE_BASIC_BLOCK, unless BB is empty.

  3. add a comment above the trick in optimize_mode_switching saying that it 
is both required to implement the FIFO insertion and valid because we know 
that the basic block was initially empty.

It's not clear to me whether this is a regression or not, so you'll also need 
to run it by the RMs.  In the meantime I have installed the attached patchlet.

2014-03-28  Eric Botcazou  <ebotcazou@adacore.com>

	* mode-switching.c: Make small adjustments to the top comment.


Index: mode-switching.c
--- mode-switching.c	(revision 208879)
+++ mode-switching.c	(working copy)
@@ -45,20 +45,20 @@  along with GCC; see the file COPYING3.
    and finding all the insns which require a specific mode.  Each insn gets
    a unique struct seginfo element.  These structures are inserted into a list
    for each basic block.  For each entity, there is an array of bb_info over
-   the flow graph basic blocks (local var 'bb_info'), and contains a list
+   the flow graph basic blocks (local var 'bb_info'), which contains a list
    of all insns within that basic block, in the order they are encountered.
    For each entity, any basic block WITHOUT any insns requiring a specific
-   mode are given a single entry, without a mode.  (Each basic block
-   in the flow graph must have at least one entry in the segment table.)
+   mode are given a single entry without a mode (each basic block in the
+   flow graph must have at least one entry in the segment table).
    The LCM algorithm is then run over the flow graph to determine where to
-   place the sets to the highest-priority value in respect of first the first
+   place the sets to the highest-priority mode with respect to the first
    insn in any one block.  Any adjustments required to the transparency
    vectors are made, then the next iteration starts for the next-lower
    priority mode, till for each entity all modes are exhausted.
-   More details are located in the code for optimize_mode_switching().  */
+   More details can be found in the code of optimize_mode_switching.  */
 /* This structure contains the information for each insn which requires
    either single or double mode to be set.