Patchwork PATCH: Properly check the end of basic block

login
register
mail settings
Submitter Uros Bizjak
Date Nov. 18, 2010, 10:15 p.m.
Message ID <AANLkTinQTxLLJqxgc2UYLBYnuJtLRwZgS14YJ8yLREgA@mail.gmail.com>
Download mbox | patch
Permalink /patch/72156/
State New
Headers show

Comments

Uros Bizjak - Nov. 18, 2010, 10:15 p.m.
On Thu, Nov 18, 2010 at 8:43 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Thu, Nov 18, 2010 at 8:37 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>
>>>> ix86_pad_returns forgot to update BB_END when it
>>>> replaces it with a new one. OK for trunk?
>>>
>>> IMO,  you should just move the call to vzeroupper optimization to be
>>> the first thing in ix86_reorg. This way, ix86_pad_short_function,
>>> ix86_pad_returns and ix86_avoid_jump_mispredict will also count
>>> emitted vzeroupper.
>>
>> But it will leave bad BB_END in place.  Any uses of BB_END later
>> will still be screwed.
>
> I think that Jan or Richard (CC'd) can provide better answer on this issue.

Got it.

It is a pass ordering problem, we free CFG before machine reorg pass,
so BLOCK_FOR_INSN in machine reorg pass does not work anymore (it
returns 0).

remove_insn (called from delete_insn) can correctly fixup BB_END (see
emit-rtl.c, line 3883), but it needs data from BLOCK_FOR_INSN to
figure out BB_END of the bb of the insn it processes.

The fix is then trivial.

2010-11-18  Uros Bizjak  <ubizjak@gmail.com>

	PR middle-end/46546
	* passes.c (init_optimization_passes): Move machine_reorg pass before
	free_cfg pass.

Tested on x86_64-pc-linux-gnu {,-m32}. OK for mainline and release branches?

Uros.
Andrew Pinski - Nov. 18, 2010, 10:19 p.m.
On Thu, Nov 18, 2010 at 2:15 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> 2010-11-18  Uros Bizjak  <ubizjak@gmail.com>
>
>        PR middle-end/46546
>        * passes.c (init_optimization_passes): Move machine_reorg pass before
>        free_cfg pass.
>
> Tested on x86_64-pc-linux-gnu {,-m32}. OK for mainline and release branches?

I don't think this will work because machine_reorg on some target
recreate the BB's anyways.  I think x86 should do that.

-- Pinski
Jakub Jelinek - Nov. 18, 2010, 10:24 p.m.
On Thu, Nov 18, 2010 at 11:15:47PM +0100, Uros Bizjak wrote:
> On Thu, Nov 18, 2010 at 8:43 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> > On Thu, Nov 18, 2010 at 8:37 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> The fix is then trivial.

Not so much.

> 
> 2010-11-18  Uros Bizjak  <ubizjak@gmail.com>
> 
> 	PR middle-end/46546
> 	* passes.c (init_optimization_passes): Move machine_reorg pass before
> 	free_cfg pass.
> 
> Tested on x86_64-pc-linux-gnu {,-m32}. OK for mainline and release branches?

I'm afraid this is going to break various targets, such change can't be
taken lightly and testing just on 2 targets is definitely not sufficient.

Definitely not something that should be applied ever to release branches,
not sure if it is something that should be done in stage3 for 4.6.

Targets that need cfg in the reorg pass compute it themselves (e.g. ia64),
other targets could depend on that the CFG is gone.

Why does i?86 actually care about CFG in its reorg pass, unlike targets
that do scheduling etc. I don't see why it should care.

E.g. ia64's comments say:
static void
ia64_reorg (void)
{ 
  /* We are freeing block_for_insn in the toplev to keep compatibility
     with old MDEP_REORGS that are not CFG based.  Recompute it now.  */
  compute_bb_for_insn ();
...

I'd say that applying this patch after testing/converting all targets
would be nice thing to do for stage1.

	Jakub
Richard Henderson - Nov. 18, 2010, 10:32 p.m.
On 11/18/2010 02:15 PM, Uros Bizjak wrote:
> Tested on x86_64-pc-linux-gnu {,-m32}. OK for mainline and release branches?

Definitely not, as mentioned elsewhere.

I just thought that I'd add that, for next stage1, it would be nice
to have a targetm boolean that says whether pass_machine_reorg needs
the cfg.  Because it does seem silly to free the cfg only to have
to immeidately re-compute it.

But that's not something to fix now.


r~
Uros Bizjak - Nov. 18, 2010, 10:33 p.m.
On Thu, Nov 18, 2010 at 11:24 PM, Jakub Jelinek <jakub@redhat.com> wrote:

>> 2010-11-18  Uros Bizjak  <ubizjak@gmail.com>
>>
>>       PR middle-end/46546
>>       * passes.c (init_optimization_passes): Move machine_reorg pass before
>>       free_cfg pass.
>>
>> Tested on x86_64-pc-linux-gnu {,-m32}. OK for mainline and release branches?
>
> I'm afraid this is going to break various targets, such change can't be
> taken lightly and testing just on 2 targets is definitely not sufficient.
>
> Definitely not something that should be applied ever to release branches,
> not sure if it is something that should be done in stage3 for 4.6.

Note taken.

> Targets that need cfg in the reorg pass compute it themselves (e.g. ia64),
> other targets could depend on that the CFG is gone.
>
> Why does i?86 actually care about CFG in its reorg pass, unlike targets
> that do scheduling etc. I don't see why it should care.

Just for the sole delete_insn of the insn at the BB_END in
ix86_pad_returns. We can in fact manually update BB_END, as H.J.
proposed.

Uros.
Jan Hubicka - Nov. 19, 2010, 8:10 a.m.
> On Thu, Nov 18, 2010 at 8:43 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> > On Thu, Nov 18, 2010 at 8:37 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> >>>> ix86_pad_returns forgot to update BB_END when it
> >>>> replaces it with a new one. OK for trunk?
> >>>
> >>> IMO,  you should just move the call to vzeroupper optimization to be
> >>> the first thing in ix86_reorg. This way, ix86_pad_short_function,
> >>> ix86_pad_returns and ix86_avoid_jump_mispredict will also count
> >>> emitted vzeroupper.
> >>
> >> But it will leave bad BB_END in place.  Any uses of BB_END later
> >> will still be screwed.
> >
> > I think that Jan or Richard (CC'd) can provide better answer on this issue.
> 
> Got it.
> 
> It is a pass ordering problem, we free CFG before machine reorg pass,
> so BLOCK_FOR_INSN in machine reorg pass does not work anymore (it
> returns 0).

If the question was why machine reorg runs after cfg freeing, the reason is
that all the machine reorgs except for Itanium one (at least last time I looked)
were not aware of CFG.  I never felt like getting all of them updated as it is bit
tricky (they do ugly stuff like inserting constant pool right into code that has CFG
representation issues)

Honza

Patch

Index: passes.c
===================================================================
--- passes.c	(revision 166920)
+++ passes.c	(working copy)
@@ -1051,8 +1051,8 @@  init_optimization_passes (void)
 	  NEXT_PASS (pass_compute_alignments);
 	  NEXT_PASS (pass_duplicate_computed_gotos);
 	  NEXT_PASS (pass_variable_tracking);
-	  NEXT_PASS (pass_free_cfg);
 	  NEXT_PASS (pass_machine_reorg);
+	  NEXT_PASS (pass_free_cfg);
 	  NEXT_PASS (pass_cleanup_barriers);
 	  NEXT_PASS (pass_delay_slots);
 	  NEXT_PASS (pass_split_for_shorten_branches);