Re: RFA: Fix PR rtl-optimization/60651
On 04/02/14 11:40, Joern Rennecke wrote: On 2 April 2014 17:34, Joern Rennecke joern.renne...@embecosm.com wrote: Hmm, the sanity check in new_seginfo caused a boostrap failure building libjava on x86. There was a block with CODE_LABEL as basic block head, otherwise empty. I've added the testcase - and a bit more detail on this issue - in the PR. I've attached an updated patch, which skips past the CODE_LABEL. And this one bootstraps on i686-pc-linuc-gnu. In optimize_mode_switching, you've got archive in a comment where it should be achieve Fix that and it's good to go. jeff
Re: RFA: Fix PR rtl-optimization/60651
I've added the testcase - and a bit more detail on this issue - in the PR. I've attached an updated patch, which skips past the CODE_LABEL. And this one bootstraps on i686-pc-linuc-gnu. OK for stage 1 modulo the typo (an instead of en) in a comment. -- Eric Botcazou
Re: RFA: Fix PR rtl-optimization/60651
On 28 March 2014 10:20, Eric Botcazou ebotca...@adacore.com wrote:
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.
Actually, the issue also applies to abnormal edges where lcm did leave a set -
but these are rare, and my last patch should handle these properly in any event,
by no longer using the NOTE_INSN_BASIC_BLOCK itself unless the block is
empty.
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
entities.
Actually, all the patch provides is a partial order, just as I stated.
Providing the strict order you describe would require adding another
loop nesting to the entity/basic block/seginfo loop, and it wouldn't
really be useful for targets.
To order by entity first, then by priority, could be useful for some targets,
so that they can express a dependency chain of mode switching events
to be computed in a single lcm pass without inflating the mode count
(which determines how often we have to invoke the lcm machinery).
However, that would require having separate buckets for each entity for
each insert_insn_on_edge point.
For epiphany, EPIPHANY_MSW_ENTITY_FPU_OMNIBUS (for -O0) and
EPIPHANY_MSW_ENTITY_ROUND_KNOWN (used when optimizing)
depend on EPIPHANY_MSW_ENTITY_AND, EPIPHANY_MSW_ENTITY_OR and
EPIPHANY_MSW_ENTITY_CONFIG.
The latter three only have two modes, an the former two use the
enum attr_fp_mode values, the first of which is FP_MODE_ROUND_UNKNOWN.
That value does not actually appear as a needed mode for these entities, hence
the partial order is sufficient.
EPIPHANY_MSW_ENTITY_FPU_OMNIBUS also depends on EPIPHANY_MSW_ENTITY_OR.
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.
Done.
It's not clear to me whether this is a regression or not, so you'll also need
to run it by the RMs.
I don't think it's a regression.
2014-04-02 Joern Rennecke joern.renne...@embecosm.com
gcc:
PR rtl-optimization/60651
* mode-switching.c (optimize_mode_switching): Make sure to emit
sets of a lower numbered entity before sets of a higher numbered
entity to a mode of the same or lower priority.
(new_seginfo): Document and enforce requirement that
NOTE_INSN_BASIC_BLOCK only appears for empty blocks.
* doc/tm.texi.in: Document ordering constraint for emitted mode sets.
* doc/tm.texi: Regenerate.
gcc/testsuite:
PR rtl-optimization/60651
* gcc.target/epiphany/mode-switch.c: New test.
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index f7024a7..b8ca17e 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -9778,6 +9778,8 @@ for @var{entity}. For any fixed @var{entity},
@code{mode_priority_to_mode}
Generate one or more insns to set @var{entity} to @var{mode}.
@var{hard_reg_live} is the set of hard registers live at the point where
the insn(s) are to be inserted.
+Sets of a lower numbered entity will be emitted before sets of a higher
+numbered entity to a mode of the same or lower priority.
@end defmac
@node Target Attributes
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 6dcbde4..d793d26 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -7447,6 +7447,8 @@ for @var{entity}. For any fixed @var{entity},
@code{mode_priority_to_mode}
Generate one or more insns to set @var{entity} to @var{mode}.
@var{hard_reg_live} is the set of hard registers live at
Re: RFA: Fix PR rtl-optimization/60651
Hmm, the sanity check in new_seginfo caused a boostrap failure building libjava on x86. There was a block with CODE_LABEL as basic block head, otherwise empty.
Re: RFA: Fix PR rtl-optimization/60651
On 2 April 2014 17:34, Joern Rennecke joern.renne...@embecosm.com wrote:
Hmm, the sanity check in new_seginfo caused a boostrap failure
building libjava on x86.
There was a block with CODE_LABEL as basic block head, otherwise empty.
I've added the testcase - and a bit more detail on this issue - in the PR.
I've attached an updated patch, which skips past the CODE_LABEL.
And this one bootstraps on i686-pc-linuc-gnu.
2014-04-02 Joern Rennecke joern.renne...@embecosm.com
gcc:
PR rtl-optimization/60651
* mode-switching.c (optimize_mode_switching): Make sure to emit
sets of a lower numbered entity before sets of a higher numbered
entity to a mode of the same or lower priority.
When creating a seginfo for a basic block that starts with a code
label, move the insertion point past the code label.
(new_seginfo): Document and enforce requirement that
NOTE_INSN_BASIC_BLOCK only appears for empty blocks.
* doc/tm.texi.in: Document ordering constraint for emitted mode sets.
* doc/tm.texi: Regenerate.
gcc/testsuite:
PR rtl-optimization/60651
* gcc.target/epiphany/mode-switch.c: New test.
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index f7024a7..b8ca17e 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -9778,6 +9778,8 @@ for @var{entity}. For any fixed @var{entity},
@code{mode_priority_to_mode}
Generate one or more insns to set @var{entity} to @var{mode}.
@var{hard_reg_live} is the set of hard registers live at the point where
the insn(s) are to be inserted.
+Sets of a lower numbered entity will be emitted before sets of a higher
+numbered entity to a mode of the same or lower priority.
@end defmac
@node Target Attributes
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 6dcbde4..d793d26 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -7447,6 +7447,8 @@ for @var{entity}. For any fixed @var{entity},
@code{mode_priority_to_mode}
Generate one or more insns to set @var{entity} to @var{mode}.
@var{hard_reg_live} is the set of hard registers live at the point where
the insn(s) are to be inserted.
+Sets of a lower numbered entity will be emitted before sets of a higher
+numbered entity to a mode of the same or lower priority.
@end defmac
@node Target Attributes
diff --git a/gcc/mode-switching.c b/gcc/mode-switching.c
index 88543b2..088156c 100644
--- a/gcc/mode-switching.c
+++ b/gcc/mode-switching.c
@@ -96,12 +96,18 @@ static void make_preds_opaque (basic_block, int);
/* This function will allocate a new BBINFO structure, initialized
- with the MODE, INSN, and basic block BB parameters. */
+ with the MODE, INSN, and basic block BB parameters.
+ INSN may not be a NOTE_INSN_BASIC_BLOCK, unless it is en empty
+ basic block; that allows us later to insert instructions in a FIFO-like
+ manner. */
static struct seginfo *
new_seginfo (int mode, rtx insn, int bb, HARD_REG_SET regs_live)
{
struct seginfo *ptr;
+
+ gcc_assert (!NOTE_INSN_BASIC_BLOCK_P (insn)
+ || insn == BB_END (NOTE_BASIC_BLOCK (insn)));
ptr = XNEW (struct seginfo);
ptr-mode = mode;
ptr-insn_ptr = insn;
@@ -534,7 +540,13 @@ optimize_mode_switching (void)
break;
if (e)
{
- ptr = new_seginfo (no_mode, BB_HEAD (bb), bb-index, live_now);
+ rtx ins_pos = BB_HEAD (bb);
+ if (LABEL_P (ins_pos))
+ ins_pos = NEXT_INSN (ins_pos);
+ gcc_assert (NOTE_INSN_BASIC_BLOCK_P (ins_pos));
+ if (ins_pos != BB_END (bb))
+ ins_pos = NEXT_INSN (ins_pos);
+ ptr = new_seginfo (no_mode, ins_pos, bb-index, live_now);
add_seginfo (info + bb-index, ptr);
bitmap_clear_bit (transp[bb-index], j);
}
@@ -733,7 +745,15 @@ optimize_mode_switching (void)
{
emitted = true;
if (NOTE_INSN_BASIC_BLOCK_P (ptr-insn_ptr))
- emit_insn_after (mode_set, ptr-insn_ptr);
+ /* We need to emit the insns in a FIFO-like manner,
+ i.e. the first to be emitted at our insertion
+ point ends up first in the instruction steam.
+ Because we made sure that NOTE_INSN_BASIC_BLOCK is
+ only used for initially empty basic blocks, we
+ can archive this by appending at the end of
+ the block. */
+ emit_insn_after
+ (mode_set, BB_END (NOTE_BASIC_BLOCK (ptr-insn_ptr)));
else
emit_insn_before (mode_set, ptr-insn_ptr);
}
--- /dev/null 2014-03-19 18:18:19.244212660 +
+++ b/gcc/testsuite/gcc.target/epiphany/mode-switch.c 2014-03-25
13:31:41.186140611 +
@@ -0,0 +1,12 @@
Re: RFA: Fix PR rtl-optimization/60651
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 entities. 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 ebotca...@adacore.com * mode-switching.c: Make small adjustments to the top comment. -- Eric BotcazouIndex: 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.
Re: RFA: Fix PR rtl-optimization/60651
As described in the PR, this patch fixes a wrong-code bug by making the order of emitted mode switching instructions more consistet predictable. I don't understand this change (but I'm not a specialist of mode switching): currently the mode setting sequence is always emitted before the insns that need it but, with the change, if an insn right after a NOTE_BASIC_BLOCK note needs it, if will be emitted either before it (if insn_ptr is the insn) or after it (if insn_ptr is the NOTE_BASIC_BLOCK note). -- Eric Botcazou
Re: RFA: Fix PR rtl-optimization/60651
On 26 March 2014 08:15, Eric Botcazou ebotca...@adacore.com wrote: As described in the PR, this patch fixes a wrong-code bug by making the order of emitted mode switching instructions more consistet predictable. I don't understand this change (but I'm not a specialist of mode switching): currently the mode setting sequence is always emitted before the insns that need it but, with the change, if an insn right after a NOTE_BASIC_BLOCK note needs it, if will be emitted either before it (if insn_ptr is the insn) or after it (if insn_ptr is the NOTE_BASIC_BLOCK note). When the seginfo is for an initially empty block, appending the mode switching instruction at the end is fine. Now that I'm trying to prove that this is always the case when insn_ptr is set to a a NOTE_INSN_BASIC_BLOCK, I find that is not actually true. insn_ptr gets set in new_seginfo, and there are three calls to that function. The second call is for instructions that themselves need a particular mode, so these are not basic block heads. The third call is for and BB_END, and this is a NOTE_INSN_BASIC_BLOCK exactly iff the block is empty. 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. 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. bootstrapped on i686-pc-linux-gnu, regtest in progress. tmp Description: Binary data
Re: RFA: Fix PR rtl-optimization/60651
On 26 March 2014 12:35, Joern Rennecke joern.renne...@embecosm.com wrote: bootstrapped on i686-pc-linux-gnu, regtest in progress. Passed now.
