Re: Revisiting the use of cselib in alias.c for scheduling

2010-07-23 Thread Steven Bosscher 
On Thu, Jul 22, 2010 at 9:29 AM, Maxim Kuvyrkov ma...@codesourcery.com wrote:
 index 89743c3..047b717 100644
 --- a/gcc/sched-rgn.c
 +++ b/gcc/sched-rgn.c
 @@ -2935,6 +2935,9 @@ schedule_region (int rgn)
   if (sched_is_disabled_for_current_region_p ())
     return;

 +  gcc_assert (!reload_completed || current_nr_blocks == 1);
 +  rgn_sched_deps_info.use_cselib = (current_nr_blocks == 1);
 +
   sched_rgn_compute_dependencies (rgn);

   sched_rgn_local_init (rgn);


This passes bootstrap on x86_64-unknown-linux-gnu. But bug 43494 will
have to be addressed before a patch like this can go in.

Ciao!
Steven

Re: Revisiting the use of cselib in alias.c for scheduling

2010-07-22 Thread Maxim Kuvyrkov 

On 7/22/10 3:34 AM, Steven Bosscher wrote:

On Wed, Jul 21, 2010 at 10:09 PM, Maxim Kuvyrkovma...@codesourcery.com  wrote:

Cselib can /always/ be used during second scheduling pass


Except with the selective scheduler when it works on regions that are
not extended basic blocks, I suppose?


Right, I was considering sched-rgn scheduler, not sel-sched.




and on
single-block regions during the first scheduling pass (after RA sched-rgn
operates on single-block regions).

Modulo the bugs enabling cselib might surface, the only reason not to enable
cselib for single-block regions in sched-rgn may be increased compile time.
  That requires some benchmarking, but my gut feeling is that the benefits
would outweigh the compile-time cost.


So something like the following _should_ work? If so, I'll give it a
try on x86*.

Ciao!
Steven

Index: sched-rgn.c
===
--- sched-rgn.c (revision 162355)
+++ sched-rgn.c (working copy)
@@ -3285,8 +3285,11 @@
  rgn_setup_sched_infos (void)
  {
if (!sel_sched_p ())
-memcpy (rgn_sched_deps_info,rgn_const_sched_deps_info,
-   sizeof (rgn_sched_deps_info));
+{
+  memcpy (rgn_sched_deps_info,rgn_const_sched_deps_info,
+ sizeof (rgn_sched_deps_info));
+  rgn_sched_deps_info.use_cselib = reload_completed;



Yes, this should work.  You can also enable cselib for single-block 
regions for first scheduling pass too.  I.e.,


index 89743c3..047b717 100644
--- a/gcc/sched-rgn.c
+++ b/gcc/sched-rgn.c
@@ -2935,6 +2935,9 @@ schedule_region (int rgn)
   if (sched_is_disabled_for_current_region_p ())
 return;

+  gcc_assert (!reload_completed || current_nr_blocks == 1);
+  rgn_sched_deps_info.use_cselib = (current_nr_blocks == 1);
+
   sched_rgn_compute_dependencies (rgn);

   sched_rgn_local_init (rgn);

Thanks,

--
Maxim Kuvyrkov
CodeSourcery
ma...@codesourcery.com
(650) 331-3385 x724

Re: Revisiting the use of cselib in alias.c for scheduling

2010-07-21 Thread Bernd Schmidt 
On 07/21/2010 03:06 PM, Steven Bosscher wrote:

 It looks like ~9% extra !true_dependence cases are found with cselib,
 with is not insignificant:
 
 situationcalls   depends ratio
 with_cselib  186764  70463   0.377284
 asis 186764  76375   0.408939 (i.e. no cselib)
 
 On the other hand, the difference in instruction count is really small
 (408 instructions more, or 0.02%):
 
 situation# insns  # bundles  # stops
 with_cselib  1984191  611991 530006
 asis 1984599  612127 530337
  +0.021%   +0.022%   +0.062%
 (insns counted with egrep ^\s+[a-z].* *.s
 bundles counted with egrep ^\s+\.[a-z]{3}\s*$ *.s
 stops counted with egrep \s+;;\s*$ *.s)

Isn't that completely the wrong thing to measure when examining
schedules?  The former is meaningful, as it shows how much room the
scheduler has to move memory accesses around.

 I would like to remove the cselib code from the scheduler (and,
 perhaps, later also from alias.c).

I do not like removing working, useful code.  If there weren't any users
of sched-ebb left, then yes.


Bernd

Re: Revisiting the use of cselib in alias.c for scheduling

2010-07-21 Thread Bernd Schmidt 
On 07/21/2010 03:06 PM, Steven Bosscher wrote:
 3. GCC now has better alias analysis than it used to, especially with
 the alias-exporting stuff that exports the GIMPLE points-to analysis
 results, but also just all the other little things that were
 contributed over the last 10 years (little things like tree-ssa :)
[...]
 It looks like ~9% extra !true_dependence cases are found with cselib,
 with is not insignificant:

So, if you want to do something useful in this area, try finding out why
cselib is still useful despite your point 3 above.  Maybe alias analysis
can be improved?

If that can't be improved, I think that rather than remove cselib from
the scheduler, the question should be: if it's useful, why don't we use
it for other schedulers rather than only sched-ebb?


Bernd

Re: Revisiting the use of cselib in alias.c for scheduling

2010-07-21 Thread Steven Bosscher 
On Wed, Jul 21, 2010 at 4:44 PM, Bernd Schmidt ber...@codesourcery.com wrote:
 On 07/21/2010 03:06 PM, Steven Bosscher wrote:
 3. GCC now has better alias analysis than it used to, especially with
 the alias-exporting stuff that exports the GIMPLE points-to analysis
 results, but also just all the other little things that were
 contributed over the last 10 years (little things like tree-ssa :)
 [...]
 It looks like ~9% extra !true_dependence cases are found with cselib,
 with is not insignificant:

 So, if you want to do something useful in this area, try finding out why
 cselib is still useful despite your point 3 above.  Maybe alias analysis
 can be improved?

Yes, this is what I planned to do anyway.

 If that can't be improved, I think that rather than remove cselib from
 the scheduler, the question should be: if it's useful, why don't we use
 it for other schedulers rather than only sched-ebb?

Well, for one thing: It currently breaks things. See PRs I referenced.
We end up not translating the VALUE rtx'en to normal addresses, and
missing real true dependencies.

Ciao!
Steven

Re: Revisiting the use of cselib in alias.c for scheduling

2010-07-21 Thread Jakub Jelinek 
On Wed, Jul 21, 2010 at 04:57:10PM +0200, Steven Bosscher wrote:
 On Wed, Jul 21, 2010 at 4:44 PM, Bernd Schmidt ber...@codesourcery.com 
 wrote:
  On 07/21/2010 03:06 PM, Steven Bosscher wrote:
  3. GCC now has better alias analysis than it used to, especially with
  the alias-exporting stuff that exports the GIMPLE points-to analysis
  results, but also just all the other little things that were
  contributed over the last 10 years (little things like tree-ssa :)
  [...]
  It looks like ~9% extra !true_dependence cases are found with cselib,
  with is not insignificant:
 
  So, if you want to do something useful in this area, try finding out why
  cselib is still useful despite your point 3 above.  Maybe alias analysis
  can be improved?
 
 Yes, this is what I planned to do anyway.
 
  If that can't be improved, I think that rather than remove cselib from
  the scheduler, the question should be: if it's useful, why don't we use
  it for other schedulers rather than only sched-ebb?
 
 Well, for one thing: It currently breaks things. See PRs I referenced.

Just because some part of GCC contains a bug doesn't mean we need to nuke
everything related to that bug.  There are bugs in lots of parts of GCC.
There were several bugs fixed in -fsched2-use-superblocks support in the
past year or two, this bug is just something that needs analysing and
fixing.

 We end up not translating the VALUE rtx'en to normal addresses, and
 missing real true dependencies.

I don't think that is a bug, if a VALUE lost all locations, what else
should get_addr return?  I guess the bug is how we are handling such
location-less VALUE.

Jakub

Re: Revisiting the use of cselib in alias.c for scheduling

2010-07-21 Thread Steven Bosscher 
On Wed, Jul 21, 2010 at 5:14 PM, Jakub Jelinek ja...@redhat.com wrote:
  If that can't be improved, I think that rather than remove cselib from
  the scheduler, the question should be: if it's useful, why don't we use
  it for other schedulers rather than only sched-ebb?

 Well, for one thing: It currently breaks things. See PRs I referenced.

 Just because some part of GCC contains a bug doesn't mean we need to nuke
 everything related to that bug.

Agreed, and I never suggested that.

However, IMHO the situation *may* be different for code that has known
bugs, *and* is de facto target specific,  *and* receives little
testing, *and* gives no measurable benefit in generated code,  *and*
receives little attention from developers (both bugs I referenced are
wrong-code bugs that took very long even to just confirm). I know this
opinion is not shared by most people in the GCC community.

Bernd's suggestion to enable this in other schedulers is a good idea
IMHO because at least that way the code gets much wider test coverage.
There are very few people who fix ia64-specific problems, but if this
feature is enabled in other schedulers then it's suddenly not
ia64-specific anymore. Good.

(NB: Alexander Monakov already explained why it's hard for sel-sched
to use cselib, see http://gcc.gnu.org/PR43494#c21)

But there is a bug to be fixed first.

I'll just sit back, enjoy myself, and watch to see if anyone besides
me cares enough to have this bug fixed in the not-too-distant future
:-)


  There are bugs in lots of parts of GCC.
 There were several bugs fixed in -fsched2-use-superblocks support in the
 past year or two, this bug is just something that needs analysing and
 fixing.

Note, I already did my best to analyze the bug, or this whole
discussion wouldn't have come up in the first place. But I don't fully
understand the problem and so I also don't know how to fix it.


 We end up not translating the VALUE rtx'en to normal addresses, and
 missing real true dependencies.

 I don't think that is a bug, if a VALUE lost all locations, what else
 should get_addr return?  I guess the bug is how we are handling such
 location-less VALUE.

Well, I don't understand how we can end up with a VALUE in sched-deps
that has lost all its locations in the first place. The VALUE without
locations is in a MEM of the insn that sched_analyze_insn is working
on, so there should be a location for this VALUE (i.e. in the insn
that's being analyzed).

Ciao!
Steven

Re: Revisiting the use of cselib in alias.c for scheduling

2010-07-21 Thread Maxim Kuvyrkov 

On 7/21/10 6:44 PM, Bernd Schmidt wrote:

On 07/21/2010 03:06 PM, Steven Bosscher wrote:

3. GCC now has better alias analysis than it used to, especially with
the alias-exporting stuff that exports the GIMPLE points-to analysis
results, but also just all the other little things that were
contributed over the last 10 years (little things like tree-ssa :)

[...]

It looks like ~9% extra !true_dependence cases are found with cselib,
with is not insignificant:

...

If that can't be improved, I think that rather than remove cselib from
the scheduler, the question should be: if it's useful, why don't we use
it for other schedulers rather than only sched-ebb?


Cselib can /always/ be used during second scheduling pass and on 
single-block regions during the first scheduling pass (after RA 
sched-rgn operates on single-block regions).


Modulo the bugs enabling cselib might surface, the only reason not to 
enable cselib for single-block regions in sched-rgn may be increased 
compile time.  That requires some benchmarking, but my gut feeling is 
that the benefits would outweigh the compile-time cost.


--
Maxim Kuvyrkov
CodeSourcery
ma...@codesourcery.com
(650) 331-3385 x724

Re: Revisiting the use of cselib in alias.c for scheduling

2010-07-21 Thread Steven Bosscher 
On Wed, Jul 21, 2010 at 10:09 PM, Maxim Kuvyrkov ma...@codesourcery.com wrote:
 Cselib can /always/ be used during second scheduling pass

Except with the selective scheduler when it works on regions that are
not extended basic blocks, I suppose?

 and on
 single-block regions during the first scheduling pass (after RA sched-rgn
 operates on single-block regions).

 Modulo the bugs enabling cselib might surface, the only reason not to enable
 cselib for single-block regions in sched-rgn may be increased compile time.
  That requires some benchmarking, but my gut feeling is that the benefits
 would outweigh the compile-time cost.

So something like the following _should_ work? If so, I'll give it a
try on x86*.

Ciao!
Steven

Index: sched-rgn.c
===
--- sched-rgn.c (revision 162355)
+++ sched-rgn.c (working copy)
@@ -3285,8 +3285,11 @@
 rgn_setup_sched_infos (void)
 {
   if (!sel_sched_p ())
-memcpy (rgn_sched_deps_info, rgn_const_sched_deps_info,
-   sizeof (rgn_sched_deps_info));
+{
+  memcpy (rgn_sched_deps_info, rgn_const_sched_deps_info,
+ sizeof (rgn_sched_deps_info));
+  rgn_sched_deps_info.use_cselib = reload_completed;
+}
   else
 memcpy (rgn_sched_deps_info, rgn_const_sel_sched_deps_info,
sizeof (rgn_sched_deps_info));