Re: [PATCH] Remove dead labels to increase superblock scope
On 09/12/11 14:59, Eric Botcazou wrote:
>> 2011-12-07 Tom de Vries
>>
>> * cfgcleanup.c (try_optimize_cfg): Replace call to delete_isns_chain by
>
> delete_insn_chain
>
>> call to delete_insn. Remove code to reorder BASIC_BLOCK note and
>> DELETED_LABEL note, and move it to ...
>> * cfg_rtl.c (delete_insn): Change return type to void.
>
> The file is cfgrtl.c without underscore. When you have a "to..." somewhere,
> you need to have a "...here" somewhere else:
>
> * cfgrtl.c (delete_insn): ...here. Change return type to void.
>
> The hunk for delete_insn contains a line with trailing TABs and spaces.
>
>> (delete_insn_and_edges): Likewise.
>> (delete_insn_chain): Handle new return type of delete_insn. Delete
>> chain backwards rather than forwards.
>> * rtl.h (delete_insn, delete_insn_and_edges): Change return type to
>> void.
>> * cfglayout.c (fixup_reorder_chain): Delete unused label.
>>
>> * gcc.dg/superblock.c: New test.
>
> OK for stage1 when it re-opens (modulo the above nits). Nice cleanup!
>
nits fixed and committed.
Thanks,
- Tom
2012-04-14 Tom de Vries
* cfgcleanup.c (try_optimize_cfg): Replace call to delete_insn_chain by
call to delete_insn. Remove code to reorder BASIC_BLOCK note and
DELETED_LABEL note, and move it to ...
* cfgrtl.c (delete_insn): ... here. Change return type to void.
(delete_insn_and_edges): Likewise.
(delete_insn_chain): Handle new return type of delete_insn. Delete
chain backwards rather than forwards.
* rtl.h (delete_insn, delete_insn_and_edges): Change return type to
void.
* cfglayout.c (fixup_reorder_chain): Delete unused label.
* gcc.dg/superblock.c: New test.
Index: gcc/cfgcleanup.c
===
--- gcc/cfgcleanup.c (revision 181652)
+++ gcc/cfgcleanup.c (working copy)
@@ -2632,20 +2632,7 @@ try_optimize_cfg (int mode)
|| ! label_is_jump_target_p (BB_HEAD (b),
BB_END (single_pred (b)
{
- rtx label = BB_HEAD (b);
-
- delete_insn_chain (label, label, false);
- /* If the case label is undeletable, move it after the
- BASIC_BLOCK note. */
- if (NOTE_KIND (BB_HEAD (b)) == NOTE_INSN_DELETED_LABEL)
- {
- rtx bb_note = NEXT_INSN (BB_HEAD (b));
-
- reorder_insns_nobb (label, label, bb_note);
- BB_HEAD (b) = bb_note;
- if (BB_END (b) == bb_note)
- BB_END (b) = label;
- }
+ delete_insn (BB_HEAD (b));
if (dump_file)
fprintf (dump_file, "Deleted label in block %i.\n",
b->index);
Index: gcc/cfglayout.c
===
--- gcc/cfglayout.c (revision 181652)
+++ gcc/cfglayout.c (working copy)
@@ -857,6 +857,9 @@ fixup_reorder_chain (void)
(e_taken->src, e_taken->dest));
e_taken->flags |= EDGE_FALLTHRU;
update_br_prob_note (bb);
+ if (LABEL_NUSES (ret_label) == 0
+ && single_pred_p (e_taken->dest))
+ delete_insn (ret_label);
continue;
}
}
Index: gcc/rtl.h
===
--- gcc/rtl.h (revision 181652)
+++ gcc/rtl.h (working copy)
@@ -2460,12 +2460,12 @@ extern void add_insn_before (rtx, rtx, s
extern void add_insn_after (rtx, rtx, struct basic_block_def *);
extern void remove_insn (rtx);
extern rtx emit (rtx);
-extern rtx delete_insn (rtx);
+extern void delete_insn (rtx);
extern rtx entry_of_function (void);
extern void emit_insn_at_entry (rtx);
extern void delete_insn_chain (rtx, rtx, bool);
extern rtx unlink_insn_chain (rtx, rtx);
-extern rtx delete_insn_and_edges (rtx);
+extern void delete_insn_and_edges (rtx);
extern rtx gen_lowpart_SUBREG (enum machine_mode, rtx);
extern rtx gen_const_mem (enum machine_mode, rtx);
extern rtx gen_frame_mem (enum machine_mode, rtx);
Index: gcc/cfgrtl.c
===
--- gcc/cfgrtl.c (revision 181652)
+++ gcc/cfgrtl.c (working copy)
@@ -111,12 +111,11 @@ can_delete_label_p (const_rtx label)
&& !in_expr_list_p (forced_labels, label));
}
-/* Delete INSN by patching it out. Return the next insn. */
+/* Delete INSN by patching it out. */
-rtx
+void
delete_insn (rtx insn)
{
- rtx next = NEXT_INSN (insn);
rtx note;
bool really_delete = true;
@@ -128,11 +127,22 @@ delete_insn (rtx insn)
if (! can_delete_label_p (insn))
{
const char *name = LABEL_NAME (insn);
+ basic_block bb = BLOCK_FOR_INSN (insn);
+ rtx bb_note = NEXT_INSN (insn);
really_delete = false;
PUT_CODE (insn, NOTE);
NOTE_KIND (insn) = NOTE_INSN_DELETED_LABEL;
NOTE_DELETED_LABEL_NAME (insn) = name;
+
+ if (bb_note != NULL_RTX && NOTE_INSN_BASIC_BLOCK_P (bb_note)
+ && BLOCK_FOR_INSN (bb_note) == bb)
+ {
+ reorder_insns_nobb (insn, insn, bb_note);
+ BB_HEAD (bb) = bb_n
Re: [PATCH] Remove dead labels to increase superblock scope
> 2011-12-07 Tom de Vries > > * cfgcleanup.c (try_optimize_cfg): Replace call to delete_isns_chain by delete_insn_chain > call to delete_insn. Remove code to reorder BASIC_BLOCK note and > DELETED_LABEL note, and move it to ... > * cfg_rtl.c (delete_insn): Change return type to void. The file is cfgrtl.c without underscore. When you have a "to..." somewhere, you need to have a "...here" somewhere else: * cfgrtl.c (delete_insn): ...here. Change return type to void. The hunk for delete_insn contains a line with trailing TABs and spaces. > (delete_insn_and_edges): Likewise. > (delete_insn_chain): Handle new return type of delete_insn. Delete > chain backwards rather than forwards. > * rtl.h (delete_insn, delete_insn_and_edges): Change return type to > void. > * cfglayout.c (fixup_reorder_chain): Delete unused label. > > * gcc.dg/superblock.c: New test. OK for stage1 when it re-opens (modulo the above nits). Nice cleanup! -- Eric Botcazou
Re: [PATCH] Remove dead labels to increase superblock scope
On 06/12/11 14:25, Michael Matz wrote:
> Hi,
>
> On Tue, 6 Dec 2011, Tom de Vries wrote:
>
>> what should be the 'next' returned by delete_insn?
>
> There are exactly two calls of delete_insn that take the return value.
> One (delete_insn_and_edges) just uses it to return it itself (and there
> are no calls to delete_insn_and_edges that use the returned value), the
> other (delete_insn_chain) wants to have the original next insn (before
> reordering), even if that means that it can see the insn twice (once as
> label, once as note, the latter would be skipped).
>
> So, return the original next one. Even better would be to somehow clean
> up the single last use of the return value in delete_insn_chain, and make
> delete_insn return nothing.
>
I did that now. The only problem I ran into was an assert in remove_insn:
...
if (BB_HEAD (bb) == insn)
{
/* Never ever delete the basic block note without deleting whole
basic block. */
gcc_assert (!NOTE_P (insn));
BB_HEAD (bb) = next;
}
...
The problematic case was a removing a basic_block using delete_insn_chain:
...
(code_label 33 26 34 4 1 "" [0 uses])
(note 34 33 50 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
(note 50 34 51 4 NOTE_INSN_EPILOGUE_BEG)
...
Normally, first the code_label was replaced by a note, then the BASIC_BLOCK note
was removed. The assert would not trigger while removing this note because the
note was not BB_HEAD.
With the fixup, when removing the BASIC_BLOCK note, the note would be at the
head and the assert would trigger:
...
(note 34 33 50 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
(note 33 26 34 4 1 "" NOTE_INSN_DELETED_LABEL)
(note 50 34 51 4 NOTE_INSN_EPILOGUE_BEG)
...
I solved this by removing backwards rather than forwards in delete_insn_chain.
Bootstrapped and reg-tested on x86_64.
OK for next stage1?
Thanks,
- Tom
2011-12-07 Tom de Vries
* cfgcleanup.c (try_optimize_cfg): Replace call to delete_isns_chain by
call to delete_insn. Remove code to reorder BASIC_BLOCK note and
DELETED_LABEL note, and move it to ...
* cfg_rtl.c (delete_insn): Change return type to void.
(delete_insn_and_edges): Likewise.
(delete_insn_chain): Handle new return type of delete_insn. Delete
chain backwards rather than forwards.
* rtl.h (delete_insn, delete_insn_and_edges): Change return type to
void.
* cfglayout.c (fixup_reorder_chain): Delete unused label.
* gcc.dg/superblock.c: New test.
Index: gcc/cfgcleanup.c
===
--- gcc/cfgcleanup.c (revision 181652)
+++ gcc/cfgcleanup.c (working copy)
@@ -2632,20 +2632,7 @@ try_optimize_cfg (int mode)
|| ! label_is_jump_target_p (BB_HEAD (b),
BB_END (single_pred (b)
{
- rtx label = BB_HEAD (b);
-
- delete_insn_chain (label, label, false);
- /* If the case label is undeletable, move it after the
- BASIC_BLOCK note. */
- if (NOTE_KIND (BB_HEAD (b)) == NOTE_INSN_DELETED_LABEL)
- {
- rtx bb_note = NEXT_INSN (BB_HEAD (b));
-
- reorder_insns_nobb (label, label, bb_note);
- BB_HEAD (b) = bb_note;
- if (BB_END (b) == bb_note)
- BB_END (b) = label;
- }
+ delete_insn (BB_HEAD (b));
if (dump_file)
fprintf (dump_file, "Deleted label in block %i.\n",
b->index);
Index: gcc/cfglayout.c
===
--- gcc/cfglayout.c (revision 181652)
+++ gcc/cfglayout.c (working copy)
@@ -857,6 +857,9 @@ fixup_reorder_chain (void)
(e_taken->src, e_taken->dest));
e_taken->flags |= EDGE_FALLTHRU;
update_br_prob_note (bb);
+ if (LABEL_NUSES (ret_label) == 0
+ && single_pred_p (e_taken->dest))
+ delete_insn (ret_label);
continue;
}
}
Index: gcc/rtl.h
===
--- gcc/rtl.h (revision 181652)
+++ gcc/rtl.h (working copy)
@@ -2460,12 +2460,12 @@ extern void add_insn_before (rtx, rtx, s
extern void add_insn_after (rtx, rtx, struct basic_block_def *);
extern void remove_insn (rtx);
extern rtx emit (rtx);
-extern rtx delete_insn (rtx);
+extern void delete_insn (rtx);
extern rtx entry_of_function (void);
extern void emit_insn_at_entry (rtx);
extern void delete_insn_chain (rtx, rtx, bool);
extern rtx unlink_insn_chain (rtx, rtx);
-extern rtx delete_insn_and_edges (rtx);
+extern void delete_insn_and_edges (rtx);
extern rtx gen_lowpart_SUBREG (enum machine_mode, rtx);
extern rtx gen_const_mem (enum machine_mode, rtx);
extern rtx gen_frame_mem (enum machine_mode, rtx);
Index: gcc/cfgrtl.c
===
--- gcc/cfgrtl.c (revision 181652)
+++ gcc/cfgrtl.c (working copy)
@@ -111,12 +111,11 @@ can_delete_label_p (const_rtx label)
&& !in_expr_list_p (forced_labels, label));
}
-/* Delete INSN by p
Re: [PATCH] Remove dead labels to increase superblock scope
Hi, On Tue, 6 Dec 2011, Tom de Vries wrote: > what should be the 'next' returned by delete_insn? There are exactly two calls of delete_insn that take the return value. One (delete_insn_and_edges) just uses it to return it itself (and there are no calls to delete_insn_and_edges that use the returned value), the other (delete_insn_chain) wants to have the original next insn (before reordering), even if that means that it can see the insn twice (once as label, once as note, the latter would be skipped). So, return the original next one. Even better would be to somehow clean up the single last use of the return value in delete_insn_chain, and make delete_insn return nothing. Ciao, Michael.
Re: [PATCH] Remove dead labels to increase superblock scope
On 05/12/11 13:50, Michael Matz wrote: > Hi, > > On Sun, 4 Dec 2011, Eric Botcazou wrote: > >>> I'm just a bit worried about the name "delete_label". "delete_insn >>> (label)" should always do the right thing for a pure deletion; the >>> point of the new routine is that it also moves instructions. >> >> It only fixes things up though, so that the RTL stream is valid again. >> Hence the question: why not retrofit it into delete_insn directly? > > Was my first reaction too. What good is delete_insn(label) if it leaves > the stream in a state to be fixed up immediately. > delete_insn returns the next insn. If I have: (code_label 33 26 34 4 1 "" [0 uses]) (note 34 33 50 4 [bb 4] NOTE_INSN_BASIC_BLOCK) (note 50 34 51 4 NOTE_INSN_EPILOGUE_BEG) and delete_insn turns the label into a note and changes order into: (note 34 33 50 4 [bb 4] NOTE_INSN_BASIC_BLOCK) (note 33 26 34 4 1 "" NOTE_INSN_DELETED_LABEL) (note 50 34 51 4 NOTE_INSN_EPILOGUE_BEG) what should be the 'next' returned by delete_insn? Thanks, - Tom > > Ciao, > Michael.
Re: [PATCH] Remove dead labels to increase superblock scope
Hi, On Sun, 4 Dec 2011, Eric Botcazou wrote: > > I'm just a bit worried about the name "delete_label". "delete_insn > > (label)" should always do the right thing for a pure deletion; the > > point of the new routine is that it also moves instructions. > > It only fixes things up though, so that the RTL stream is valid again. > Hence the question: why not retrofit it into delete_insn directly? Was my first reaction too. What good is delete_insn(label) if it leaves the stream in a state to be fixed up immediately. Ciao, Michael.
Re: [PATCH] Remove dead labels to increase superblock scope
> Looks good codewise. Seconded, modulo the file: the function should be in cfgrtl.c instead. > I'm just a bit worried about the name "delete_label". > "delete_insn (label)" should always do the right thing for a pure deletion; > the point of the new routine is that it also moves instructions. It only fixes things up though, so that the RTL stream is valid again. Hence the question: why not retrofit it into delete_insn directly? -- Eric Botcazou
Re: [PATCH] Remove dead labels to increase superblock scope
Tom de Vries writes: > OK, factored out delete_label now. > > Bootstrapped and reg-tested on x86_64. > > Ok for next stage1? Looks good codewise. I'm just a bit worried about the name "delete_label". "delete_insn (label)" should always do the right thing for a pure deletion; the point of the new routine is that it also moves instructions. I'd prefer a name that differentiated it from delete_insn. E.g. "hide_label" or "decommission_label", although as you can tell I'm useless at naming things... Thanks, Richard
Re: [PATCH] Remove dead labels to increase superblock scope
On 02/12/11 11:40, Richard Sandiford wrote:
> Tom de Vries writes:
>> On 27/11/11 23:59, Eric Botcazou wrote:
No, DELETED_LABEL notes still work just fine. It depends on how you
remove the label and replace it with a note, and Tom isn't showing
what he did, so...
>>>
>>> I agree that there is no obvious reason why just calling delete_insn would
>>> not
>>> work, so this should be investigated first.
>>>
>>
>> The reason it didn't work, is because after turning a label into a
>> NOTE_INSN_DELETED_LABEL, one needs to move it to after the
>> NOTE_INSN_BASIC_BLOCK
>> as in cfgcleanup.c:try_optimize_cfg():
>> ...
>>delete_insn_chain (label, label, false);
>>/* If the case label is undeletable, move it after the
>> BASIC_BLOCK note. */
>>if (NOTE_KIND (BB_HEAD (b)) == NOTE_INSN_DELETED_LABEL)
>> {
>>rtx bb_note = NEXT_INSN (BB_HEAD (b));
>>
>>reorder_insns_nobb (label, label, bb_note);
>>BB_HEAD (b) = bb_note;
>>if (BB_END (b) == bb_note)
>> BB_END (b) = label;
>> }
>> ...
>>
>> Attached patch factors out this piece of code and reuses it in
>> fixup_reorder_chain.
>
> But isn't...
>
>> @@ -2637,15 +2658,7 @@ try_optimize_cfg (int mode)
>>delete_insn_chain (label, label, false);
>>/* If the case label is undeletable, move it after the
>> BASIC_BLOCK note. */
>> - if (NOTE_KIND (BB_HEAD (b)) == NOTE_INSN_DELETED_LABEL)
>> -{
>> - rtx bb_note = NEXT_INSN (BB_HEAD (b));
>> -
>> - reorder_insns_nobb (label, label, bb_note);
>> - BB_HEAD (b) = bb_note;
>> - if (BB_END (b) == bb_note)
>> -BB_END (b) = label;
>> -}
>> + fixup_deleted_label (b);
>
> ...this "delete_insn_chain (label, label, false);" call equivalent
> to "delete_insn (label)"?
Indeed, it is.
> Splitting the operation in two here and:
>
>> Index: gcc/cfglayout.c
>> ===
>> --- gcc/cfglayout.c (revision 181652)
>> +++ gcc/cfglayout.c (working copy)
>> @@ -857,6 +857,12 @@ fixup_reorder_chain (void)
>> (e_taken->src, e_taken->dest));
>>e_taken->flags |= EDGE_FALLTHRU;
>>update_br_prob_note (bb);
>> + if (LABEL_NUSES (ret_label) == 0
>> + && single_pred_p (e_taken->dest))
>> +{
>> + delete_insn (ret_label);
>> + fixup_deleted_label (e_taken->dest);
>> +}
>
> ...here seems a little odd.
>
> Richard
OK, factored out delete_label now.
Bootstrapped and reg-tested on x86_64.
Ok for next stage1?
Thanks,
- Tom
2011-12-03 Tom de Vries
* cfgcleanup.c (delete_label): New function, factored out of ...
(try_optimize_cfg): Use deleted_label.
* basic-block.h (delete_label): Declare.
* cfglayout.c (fixup_reorder_chain): Delete unused label using
delete_label.
* gcc.dg/superblock.c: New test.
Index: gcc/cfgcleanup.c
===
--- gcc/cfgcleanup.c (revision 181172)
+++ gcc/cfgcleanup.c (working copy)
@@ -2518,6 +2518,39 @@ trivially_empty_bb_p (basic_block bb)
}
}
+/* Delete the label at the head of BB, and if that results in a DELETED_LABEL
+ note, move it after the BASIC_BLOCK note of BB. */
+
+void
+delete_label (basic_block bb)
+{
+ rtx label = BB_HEAD (bb), deleted_label, bb_note;
+ gcc_assert (LABEL_P (label));
+
+ /* Delete the label. */
+ delete_insn (label);
+ if (dump_file)
+fprintf (dump_file, "Deleted label in block %i.\n",
+ bb->index);
+
+ /* Find the DELETED_LABEL note. */
+ deleted_label = BB_HEAD (bb);
+ if (deleted_label == NULL_RTX
+ || !NOTE_P (deleted_label)
+ || NOTE_KIND (deleted_label) != NOTE_INSN_DELETED_LABEL)
+return;
+
+ /* Find the BASIC_BLOCK note. */
+ bb_note = NEXT_INSN (deleted_label);
+ gcc_assert (bb_note != NULL_RTX && NOTE_INSN_BASIC_BLOCK_P (bb_note));
+
+ /* Move the DELETED_LABEL note after the BASIC_BLOCK note. */
+ reorder_insns_nobb (deleted_label, deleted_label, bb_note);
+ BB_HEAD (bb) = bb_note;
+ if (BB_END (bb) == bb_note)
+BB_END (bb) = deleted_label;
+}
+
/* Do simple CFG optimizations - basic block merging, simplifying of jump
instructions etc. Return nonzero if changes were made. */
@@ -2631,25 +2664,7 @@ try_optimize_cfg (int mode)
|| !JUMP_P (BB_END (single_pred (b)))
|| ! label_is_jump_target_p (BB_HEAD (b),
BB_END (single_pred (b)
- {
- rtx label = BB_HEAD (b);
-
- delete_insn_chain (label, label, false);
- /* If the case label is undeletable, move it aft
Re: [PATCH] Remove dead labels to increase superblock scope
Tom de Vries writes:
> On 27/11/11 23:59, Eric Botcazou wrote:
>>> No, DELETED_LABEL notes still work just fine. It depends on how you
>>> remove the label and replace it with a note, and Tom isn't showing
>>> what he did, so...
>>
>> I agree that there is no obvious reason why just calling delete_insn would
>> not
>> work, so this should be investigated first.
>>
>
> The reason it didn't work, is because after turning a label into a
> NOTE_INSN_DELETED_LABEL, one needs to move it to after the
> NOTE_INSN_BASIC_BLOCK
> as in cfgcleanup.c:try_optimize_cfg():
> ...
> delete_insn_chain (label, label, false);
> /* If the case label is undeletable, move it after the
>BASIC_BLOCK note. */
> if (NOTE_KIND (BB_HEAD (b)) == NOTE_INSN_DELETED_LABEL)
> {
> rtx bb_note = NEXT_INSN (BB_HEAD (b));
>
> reorder_insns_nobb (label, label, bb_note);
> BB_HEAD (b) = bb_note;
> if (BB_END (b) == bb_note)
> BB_END (b) = label;
> }
> ...
>
> Attached patch factors out this piece of code and reuses it in
> fixup_reorder_chain.
But isn't...
> @@ -2637,15 +2658,7 @@ try_optimize_cfg (int mode)
> delete_insn_chain (label, label, false);
> /* If the case label is undeletable, move it after the
>BASIC_BLOCK note. */
> - if (NOTE_KIND (BB_HEAD (b)) == NOTE_INSN_DELETED_LABEL)
> - {
> - rtx bb_note = NEXT_INSN (BB_HEAD (b));
> -
> - reorder_insns_nobb (label, label, bb_note);
> - BB_HEAD (b) = bb_note;
> - if (BB_END (b) == bb_note)
> - BB_END (b) = label;
> - }
> + fixup_deleted_label (b);
...this "delete_insn_chain (label, label, false);" call equivalent
to "delete_insn (label)"? Splitting the operation in two here and:
> Index: gcc/cfglayout.c
> ===
> --- gcc/cfglayout.c (revision 181652)
> +++ gcc/cfglayout.c (working copy)
> @@ -857,6 +857,12 @@ fixup_reorder_chain (void)
> (e_taken->src, e_taken->dest));
> e_taken->flags |= EDGE_FALLTHRU;
> update_br_prob_note (bb);
> + if (LABEL_NUSES (ret_label) == 0
> + && single_pred_p (e_taken->dest))
> + {
> + delete_insn (ret_label);
> + fixup_deleted_label (e_taken->dest);
> + }
...here seems a little odd.
Richard
Re: [PATCH] Remove dead labels to increase superblock scope
On 27/11/11 23:59, Eric Botcazou wrote:
>> No, DELETED_LABEL notes still work just fine. It depends on how you
>> remove the label and replace it with a note, and Tom isn't showing
>> what he did, so...
>
> I agree that there is no obvious reason why just calling delete_insn would
> not
> work, so this should be investigated first.
>
The reason it didn't work, is because after turning a label into a
NOTE_INSN_DELETED_LABEL, one needs to move it to after the NOTE_INSN_BASIC_BLOCK
as in cfgcleanup.c:try_optimize_cfg():
...
delete_insn_chain (label, label, false);
/* If the case label is undeletable, move it after the
BASIC_BLOCK note. */
if (NOTE_KIND (BB_HEAD (b)) == NOTE_INSN_DELETED_LABEL)
{
rtx bb_note = NEXT_INSN (BB_HEAD (b));
reorder_insns_nobb (label, label, bb_note);
BB_HEAD (b) = bb_note;
if (BB_END (b) == bb_note)
BB_END (b) = label;
}
...
Attached patch factors out this piece of code and reuses it in
fixup_reorder_chain.
Bootstrapped and reg-tested on x86_64.
OK for next stage1?
Thanks,
- Tom
2011-12-01 Tom de Vries
* cfgcleanup.c (fixup_deleted_label): New function, factored out of ...
(try_optimize_cfg): Use fixup_deleted_label.
* cfglayout.c (fixup_reorder_chain): Delete unused label, and fixup
using fixup_deleted_label.
* gcc.dg/superblock.c: New test.
Index: gcc/cfgcleanup.c
===
--- gcc/cfgcleanup.c (revision 181652)
+++ gcc/cfgcleanup.c (working copy)
@@ -2518,6 +2518,27 @@ trivially_empty_bb_p (basic_block bb)
}
}
+/* Move a DELETED_LABEL note after the BASIC_BLOCK note of BB. */
+
+void
+fixup_deleted_label (basic_block bb)
+{
+ rtx deleted_label = BB_HEAD (bb), bb_note;
+
+ if (deleted_label == NULL_RTX
+ || !NOTE_P (deleted_label)
+ || NOTE_KIND (deleted_label) != NOTE_INSN_DELETED_LABEL)
+return;
+
+ bb_note = NEXT_INSN (deleted_label);
+ gcc_assert (bb_note != NULL_RTX && NOTE_INSN_BASIC_BLOCK_P (bb_note));
+
+ reorder_insns_nobb (deleted_label, deleted_label, bb_note);
+ BB_HEAD (bb) = bb_note;
+ if (BB_END (bb) == bb_note)
+BB_END (bb) = deleted_label;
+}
+
/* Do simple CFG optimizations - basic block merging, simplifying of jump
instructions etc. Return nonzero if changes were made. */
@@ -2637,15 +2658,7 @@ try_optimize_cfg (int mode)
delete_insn_chain (label, label, false);
/* If the case label is undeletable, move it after the
BASIC_BLOCK note. */
- if (NOTE_KIND (BB_HEAD (b)) == NOTE_INSN_DELETED_LABEL)
- {
- rtx bb_note = NEXT_INSN (BB_HEAD (b));
-
- reorder_insns_nobb (label, label, bb_note);
- BB_HEAD (b) = bb_note;
- if (BB_END (b) == bb_note)
- BB_END (b) = label;
- }
+ fixup_deleted_label (b);
if (dump_file)
fprintf (dump_file, "Deleted label in block %i.\n",
b->index);
Index: gcc/cfglayout.c
===
--- gcc/cfglayout.c (revision 181652)
+++ gcc/cfglayout.c (working copy)
@@ -857,6 +857,12 @@ fixup_reorder_chain (void)
(e_taken->src, e_taken->dest));
e_taken->flags |= EDGE_FALLTHRU;
update_br_prob_note (bb);
+ if (LABEL_NUSES (ret_label) == 0
+ && single_pred_p (e_taken->dest))
+ {
+ delete_insn (ret_label);
+ fixup_deleted_label (e_taken->dest);
+ }
continue;
}
}
Index: gcc/basic-block.h
===
--- gcc/basic-block.h (revision 181652)
+++ gcc/basic-block.h (working copy)
@@ -813,6 +813,7 @@ extern void rtl_make_eh_edge (sbitmap, b
enum replace_direction { dir_none, dir_forward, dir_backward, dir_both };
/* In cfgcleanup.c. */
+extern void fixup_deleted_label (basic_block);
extern bool cleanup_cfg (int);
extern int flow_find_cross_jump (basic_block, basic_block, rtx *, rtx *,
enum replace_direction*);
Index: gcc/testsuite/gcc.dg/superblock.c
===
--- /dev/null (new file)
+++ gcc/testsuite/gcc.dg/superblock.c (revision 0)
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-asynchronous-unwind-tables -fsched2-use-superblocks -fdump-rtl-sched2 -fdump-rtl-bbro" } */
+
+typedef int aligned __attribute__ ((aligned (64)));
+extern void abort (void);
+
+int bar (void *p);
+
+void
+foo (void)
+{
+ char *p = __builtin_alloca (13);
+ aligned i;
+
+ if (bar (p) || bar (&i))
+abort ();
+}
+
+/* { dg-final { scan-rtl-dump-times "0 uses" 0 "bbro"} } */
+/* { dg-final { scan-rtl-dump-times "ADVANCING TO" 2 "sched2"} } */
+/* { dg-final { cleanup-rtl-dump "bbro" } } */
+/* { dg-final { cleanup-rtl-dump "sche
Re: [PATCH] Remove dead labels to increase superblock scope
> No, DELETED_LABEL notes still work just fine. It depends on how you > remove the label and replace it with a note, and Tom isn't showing > what he did, so... I agree that there is no obvious reason why just calling delete_insn would not work, so this should be investigated first. -- Eric Botcazou
Re: [PATCH] Remove dead labels to increase superblock scope
On 25/11/11 14:05, Steven Bosscher wrote: > On Fri, Nov 25, 2011 at 2:03 PM, Michael Matz wrote: >> Hi, >> >> On Fri, 25 Nov 2011, Tom de Vries wrote: >> Note that you actually can remove labels also if they are !can_delete_label_p, if you use delete_insn (which you do). It will replace such undeletable labels by a DELETED_LABEL note. >>> >>> I tried that as well but ran into these errors in rtl_verify_flow_info_1: >>> ... >>> libquadmath/printf/cmp.c:56:1: error: NOTE_INSN_BASIC_BLOCK is missing for >>> block 6 >>> libquadmath/printf/cmp.c:56:1: error: NOTE_INSN_BASIC_BLOCK 79 in middle of >>> basic block 6 >> >> Hmpf, probably bitrotted over time. Oh well, so be it. > > No, DELETED_LABEL notes still work just fine. It depends on how you > remove the label and replace it with a note, and Tom isn't showing > what he did, so... This is the patch with which I ran into the rtl_verify_flow_info_1 errors: ... Index: gcc/cfglayout.c === --- gcc/cfglayout.c (revision 181172) +++ gcc/cfglayout.c (working copy) @@ -857,6 +857,9 @@ fixup_reorder_chain (void) (e_taken->src, e_taken->dest)); e_taken->flags |= EDGE_FALLTHRU; update_br_prob_note (bb); + if (LABEL_NUSES (ret_label) == 0 + && single_pred_p (e_taken->dest)) + delete_insn (ret_label); continue; } } ... Thanks, - Tom > > Ciao! > Steven
Re: [PATCH] Remove dead labels to increase superblock scope
On Fri, Nov 25, 2011 at 2:03 PM, Michael Matz wrote: > Hi, > > On Fri, 25 Nov 2011, Tom de Vries wrote: > >> > Note that you actually can remove labels also if they are >> > !can_delete_label_p, if you use delete_insn (which you do). It will >> > replace such undeletable labels by a DELETED_LABEL note. >> >> I tried that as well but ran into these errors in rtl_verify_flow_info_1: >> ... >> libquadmath/printf/cmp.c:56:1: error: NOTE_INSN_BASIC_BLOCK is missing for >> block 6 >> libquadmath/printf/cmp.c:56:1: error: NOTE_INSN_BASIC_BLOCK 79 in middle of >> basic block 6 > > Hmpf, probably bitrotted over time. Oh well, so be it. No, DELETED_LABEL notes still work just fine. It depends on how you remove the label and replace it with a note, and Tom isn't showing what he did, so... Ciao! Steven
Re: [PATCH] Remove dead labels to increase superblock scope
Hi, On Fri, 25 Nov 2011, Tom de Vries wrote: > > Note that you actually can remove labels also if they are > > !can_delete_label_p, if you use delete_insn (which you do). It will > > replace such undeletable labels by a DELETED_LABEL note. > > I tried that as well but ran into these errors in rtl_verify_flow_info_1: > ... > libquadmath/printf/cmp.c:56:1: error: NOTE_INSN_BASIC_BLOCK is missing for > block 6 > libquadmath/printf/cmp.c:56:1: error: NOTE_INSN_BASIC_BLOCK 79 in middle of > basic block 6 Hmpf, probably bitrotted over time. Oh well, so be it. Ciao, Michael.
Re: [PATCH] Remove dead labels to increase superblock scope
On 21/11/11 17:13, Michael Matz wrote:
> Hi,
>
> On Sat, 19 Nov 2011, Tom de Vries wrote:
>
>> On 11/18/2011 10:29 PM, Eric Botcazou wrote:
For the test-case of PR50764, a dead label is introduced by
fixup_reorder_chain in cfg_layout_finalize, called from
pass_reorder_blocks.
>>>
>>> I presume that there is no reasonable way of preventing fixup_reorder_chain
>>> from introducing it or of teaching fixup_reorder_chain to remove it?
>>>
>>
>> This (untested) patch also removes the dead label for the PR, and I
>> think it is safe. ...
>
> cfgrtl.c has already code to delete labels (delete_insn) when appropriate
> (can_delete_label_p). Perhaps that can be reused somehow.
>
>> Index: cfglayout.c
>> === ---
>> cfglayout.c (revision 181377) +++ cfglayout.c (working copy) @@ -702,6
>> +702,21 @@ relink_block_chain (bool stay_in_cfglayo
>> }
>>
>>
>> +static bool
>> +forced_label_p (rtx label)
>> +{
>> + rtx insn, forced_label;
>> + for (insn = forced_labels; insn; insn = XEXP (insn, 1))
>> +{
>> + forced_label = XEXP (insn, 0);
>> + if (!LABEL_P (forced_label))
>> +continue;
>> + if (forced_label == label)
>> +return true;
>> +}
>> + return false;
>> +}
>
> That's in_expr_list_p().
>
>> @@ -857,6 +872,12 @@ fixup_reorder_chain (void)
>> (e_taken->src, e_taken->dest));
>>e_taken->flags |= EDGE_FALLTHRU;
>>update_br_prob_note (bb);
>> + if (LABEL_NUSES (ret_label) == 0
>
>> + && !LABEL_PRESERVE_P (ret_label)
>> + && LABEL_NAME (ret_label) == NULL
>> + && !forced_label_p (ret_label)
>
> And this is cfgrtl.c:can_delete_label_p.
Ok, using that in the new version.
> Note that you actually
> can remove labels also if they are !can_delete_label_p, if you use
> delete_insn (which you do). It will replace such undeletable labels by a
> DELETED_LABEL note.
>
I tried that as well but ran into these errors in rtl_verify_flow_info_1:
...
libquadmath/printf/cmp.c:56:1: error: NOTE_INSN_BASIC_BLOCK is missing for
block 6
libquadmath/printf/cmp.c:56:1: error: NOTE_INSN_BASIC_BLOCK 79 in middle of
basic block 6
libquadmath/printf/cmp.c:56:1: internal compiler error: verify_flow_info failed
a-direct.ads:460:9: error: NOTE_INSN_BASIC_BLOCK is missing for block 6
a-direct.ads:460:9: error: NOTE_INSN_BASIC_BLOCK 25 in middle of basic block 6
+===GNAT BUG DETECTED==+
| 4.7.0 2023 (experimental) (x86_64-unknown-linux-gnu) GCC error: |
| verify_flow_info failed |
| Error detected around a-direct.ads:460:9 |
...
Eric,
This new patch was bootstrapped and reg-tested on x86_64.
this new patch or old patch (
http://gcc.gnu.org/ml/gcc-patches/2011-11/msg01953.html ) ok for next stage1?
Thanks,
- Tom
>
> Ciao,
> Michael.
2011-11-25 Tom de Vries
* rtl.h (can_delete_label_p): Declare.
* cfgrtl.c (can_delete_label_p): Remove static.
* cfglayout.c (fixup_reorder_chain): Delete unused label if
can_delete_label_p.
* gcc.dg/superblock.c: New test.
Index: gcc/cfglayout.c
===
--- gcc/cfglayout.c (revision 181652)
+++ gcc/cfglayout.c (working copy)
@@ -857,6 +857,10 @@ fixup_reorder_chain (void)
(e_taken->src, e_taken->dest));
e_taken->flags |= EDGE_FALLTHRU;
update_br_prob_note (bb);
+ if (LABEL_NUSES (ret_label) == 0
+ && can_delete_label_p (ret_label)
+ && single_pred_p (e_taken->dest))
+ delete_insn (ret_label);
continue;
}
}
Index: gcc/rtl.h
===
--- gcc/rtl.h (revision 181652)
+++ gcc/rtl.h (working copy)
@@ -2482,6 +2482,9 @@ extern void dump_combine_total_stats (FI
/* In cfgcleanup.c */
extern void delete_dead_jumptables (void);
+/* In rtlcfg.c */
+int can_delete_label_p (const_rtx);
+
/* In sched-vis.c. */
extern void debug_bb_n_slim (int);
extern void debug_bb_slim (struct basic_block_def *);
Index: gcc/cfgrtl.c
===
--- gcc/cfgrtl.c (revision 181652)
+++ gcc/cfgrtl.c (working copy)
@@ -66,7 +66,6 @@ along with GCC; see the file COPYING3.
#include "df.h"
static int can_delete_note_p (const_rtx);
-static int can_delete_label_p (const_rtx);
static basic_block rtl_split_edge (edge);
static bool rtl_move_block_after (basic_block, basic_block);
static int rtl_verify_flow_info (void);
@@ -102,7 +101,7 @@ can_delete_note_p (const_rtx note)
/* True if a given label can be deleted. */
-static int
+int
can_delete_label_p (const_rtx label)
{
return (!LABEL_PRESERVE_P (label)
Index: gcc/testsuite/gcc.dg/superblock.c
==
Re: [PATCH] Remove dead labels to increase superblock scope
Hi,
On Sat, 19 Nov 2011, Tom de Vries wrote:
> On 11/18/2011 10:29 PM, Eric Botcazou wrote:
> >> For the test-case of PR50764, a dead label is introduced by
> >> fixup_reorder_chain in cfg_layout_finalize, called from
> >> pass_reorder_blocks.
> >
> > I presume that there is no reasonable way of preventing fixup_reorder_chain
> > from introducing it or of teaching fixup_reorder_chain to remove it?
> >
>
> This (untested) patch also removes the dead label for the PR, and I
> think it is safe. ...
cfgrtl.c has already code to delete labels (delete_insn) when appropriate
(can_delete_label_p). Perhaps that can be reused somehow.
> Index: cfglayout.c
> === ---
> cfglayout.c (revision 181377) +++ cfglayout.c (working copy) @@ -702,6
> +702,21 @@ relink_block_chain (bool stay_in_cfglayo
> }
>
>
> +static bool
> +forced_label_p (rtx label)
> +{
> + rtx insn, forced_label;
> + for (insn = forced_labels; insn; insn = XEXP (insn, 1))
> +{
> + forced_label = XEXP (insn, 0);
> + if (!LABEL_P (forced_label))
> + continue;
> + if (forced_label == label)
> + return true;
> +}
> + return false;
> +}
That's in_expr_list_p().
> @@ -857,6 +872,12 @@ fixup_reorder_chain (void)
> (e_taken->src, e_taken->dest));
> e_taken->flags |= EDGE_FALLTHRU;
> update_br_prob_note (bb);
> + if (LABEL_NUSES (ret_label) == 0
> + && !LABEL_PRESERVE_P (ret_label)
> + && LABEL_NAME (ret_label) == NULL
> + && !forced_label_p (ret_label)
And this is cfgrtl.c:can_delete_label_p. Note that you actually
can remove labels also if they are !can_delete_label_p, if you use
delete_insn (which you do). It will replace such undeletable labels by a
DELETED_LABEL note.
Ciao,
Michael.
Re: [PATCH] Remove dead labels to increase superblock scope
On 11/18/2011 10:29 PM, Eric Botcazou wrote:
>> For the test-case of PR50764, a dead label is introduced by
>> fixup_reorder_chain in cfg_layout_finalize, called from
>> pass_reorder_blocks.
>
> I presume that there is no reasonable way of preventing fixup_reorder_chain
> from introducing it or of teaching fixup_reorder_chain to remove it?
>
This (untested) patch also removes the dead label for the PR, and I think it is
safe.
...
Index: cfglayout.c
===
--- cfglayout.c (revision 181377)
+++ cfglayout.c (working copy)
@@ -702,6 +702,21 @@ relink_block_chain (bool stay_in_cfglayo
}
+static bool
+forced_label_p (rtx label)
+{
+ rtx insn, forced_label;
+ for (insn = forced_labels; insn; insn = XEXP (insn, 1))
+{
+ forced_label = XEXP (insn, 0);
+ if (!LABEL_P (forced_label))
+ continue;
+ if (forced_label == label)
+ return true;
+}
+ return false;
+}
+
/* Given a reorder chain, rearrange the code to match. */
static void
@@ -857,6 +872,12 @@ fixup_reorder_chain (void)
(e_taken->src, e_taken->dest));
e_taken->flags |= EDGE_FALLTHRU;
update_br_prob_note (bb);
+ if (LABEL_NUSES (ret_label) == 0
+ && !LABEL_PRESERVE_P (ret_label)
+ && LABEL_NAME (ret_label) == NULL
+ && !forced_label_p (ret_label)
+ && single_pred_p (e_taken->dest))
+ delete_insn (ret_label);
continue;
}
}
...
But I see 2 potential issues:
- it only catches this case in fixup_reorder_chain. I don't know if there are
more cases, but the earlier catch-all-afterwards patch surely will catch
those, this patch probably not.
- I'm not sure if the use count will drop always drop to 0 in
fixup_reorder_chain, that might only happen after rebuild_jump_labels.
Thanks,
- Tom
Re: [PATCH] Remove dead labels to increase superblock scope
> For the test-case of PR50764, a dead label is introduced by > fixup_reorder_chain in cfg_layout_finalize, called from > pass_reorder_blocks. I presume that there is no reasonable way of preventing fixup_reorder_chain from introducing it or of teaching fixup_reorder_chain to remove it? -- Eric Botcazou
Re: [PATCH] Remove dead labels to increase superblock scope
On 11/18/2011 09:16 PM, Andrew Pinski wrote:
> On Fri, Nov 18, 2011 at 5:11 AM, Tom de Vries wrote:
>> Hi Eric,
>>
>> this patch fixes an inefficiency problem I observed while working on PR50764.
>>
>> For the test-case of PR50764, a dead label is introduced by
>> fixup_reorder_chain
>> in cfg_layout_finalize, called from pass_reorder_blocks.
>> The dead label is removed by a cleanup_cfg in pass_duplicate_computed_gotos,
>> but
>> is still present during pass_sched2.
>> When constructing ebbs in schedule_ebbs, the dead label ends an ebb. If we
>> remove the label before pass_sched2, the ebb is allowed to grow larger.
>>
>> The patch tries to get rid of the dead label immediately after it's
>> introduced,
>> in cfg_layout_finalized.
>>
>> The new test gcc.dg/superblock.c uses -fno-asynchronous-unwind-tables to work
>> around PR50764.
>>
>> bootstrapped and reg-tested on x86_64.
>>
>> OK for next stage 1?
>
> ENOPATCH.
>
Thanks Andrew.
here it is.
Thanks,
- Tom
>>
>> 2011-11-18 Tom de Vries
>>
>>* rtl.h (delete_dead_labels): Declare.
>>* cfgcleanup.c (delete_dead_labels): New function.
>>* cfglayout.c (cfg_layout_finalize): Use delete_dead_labels.
>>
>>* gcc.dg/superblock.c: New test.
>>
Index: gcc/rtl.h
===
--- gcc/rtl.h (revision 181377)
+++ gcc/rtl.h (working copy)
@@ -2481,6 +2481,7 @@ extern void dump_combine_total_stats (FI
/* In cfgcleanup.c */
extern void delete_dead_jumptables (void);
+extern void delete_dead_labels (void);
/* In sched-vis.c. */
extern void debug_bb_n_slim (int);
Index: gcc/cfgcleanup.c
===
--- gcc/cfgcleanup.c (revision 181377)
+++ gcc/cfgcleanup.c (working copy)
@@ -2900,6 +2900,37 @@ delete_dead_jumptables (void)
}
}
+/* Delete labels which are dead and can be removed. */
+
+void
+delete_dead_labels (void)
+{
+ basic_block bb;
+
+ FOR_EACH_BB (bb)
+{
+ rtx insn;
+
+ FOR_BB_INSNS (bb, insn)
+ {
+ if (NOTE_INSN_BASIC_BLOCK_P (insn))
+ continue;
+
+ if (!LABEL_P (insn)
+ || LABEL_NUSES (insn) != 0
+ || LABEL_PRESERVE_P (insn)
+ || LABEL_NAME (insn) != 0)
+ break;
+
+ if (dump_file)
+ fprintf (dump_file, "Dead label %i removed\n",
+ INSN_UID (insn));
+
+ delete_insn (insn);
+ break;
+ }
+}
+}
/* Tidy the CFG by deleting unreachable code and whatnot. */
Index: gcc/cfglayout.c
===
--- gcc/cfglayout.c (revision 181377)
+++ gcc/cfglayout.c (working copy)
@@ -1369,6 +1369,7 @@ cfg_layout_finalize (void)
rebuild_jump_labels (get_insns ());
delete_dead_jumptables ();
+ delete_dead_labels ();
#ifdef ENABLE_CHECKING
verify_insn_chain ();
Index: gcc/testsuite/gcc.dg/superblock.c
===
--- /dev/null (new file)
+++ gcc/testsuite/gcc.dg/superblock.c (revision 0)
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-asynchronous-unwind-tables -fsched2-use-superblocks -fdump-rtl-sched2 -fdump-rtl-bbro" } */
+
+typedef int aligned __attribute__ ((aligned (64)));
+extern void abort (void);
+
+int bar (void *p);
+
+void
+foo (void)
+{
+ char *p = __builtin_alloca (13);
+ aligned i;
+
+ if (bar (p) || bar (&i))
+abort ();
+}
+
+/* { dg-final { scan-rtl-dump-times "0 uses" 0 "bbro"} } */
+/* { dg-final { scan-rtl-dump-times "ADVANCING TO" 2 "sched2"} } */
+/* { dg-final { cleanup-rtl-dump "bbro" } } */
+/* { dg-final { cleanup-rtl-dump "sched2" } } */
+
Re: [PATCH] Remove dead labels to increase superblock scope
On Fri, Nov 18, 2011 at 5:11 AM, Tom de Vries wrote: > Hi Eric, > > this patch fixes an inefficiency problem I observed while working on PR50764. > > For the test-case of PR50764, a dead label is introduced by > fixup_reorder_chain > in cfg_layout_finalize, called from pass_reorder_blocks. > The dead label is removed by a cleanup_cfg in pass_duplicate_computed_gotos, > but > is still present during pass_sched2. > When constructing ebbs in schedule_ebbs, the dead label ends an ebb. If we > remove the label before pass_sched2, the ebb is allowed to grow larger. > > The patch tries to get rid of the dead label immediately after it's > introduced, > in cfg_layout_finalized. > > The new test gcc.dg/superblock.c uses -fno-asynchronous-unwind-tables to work > around PR50764. > > bootstrapped and reg-tested on x86_64. > > OK for next stage 1? ENOPATCH. > > Thanks, > - Tom > > 2011-11-18 Tom de Vries > > * rtl.h (delete_dead_labels): Declare. > * cfgcleanup.c (delete_dead_labels): New function. > * cfglayout.c (cfg_layout_finalize): Use delete_dead_labels. > > * gcc.dg/superblock.c: New test. >
