| Message ID | alpine.LNX.2.00.1106021513450.6250@monoid.intra.ispras.ru |
|---|---|
| State | New |
| Headers | show |
On 06/02/2011 01:29 PM, Alexander Monakov wrote: > Bernd, > > The problem is INSN_COND should be reset when initializing a new deps > structure, otherwise instructions may get stale conditions from other > previously analyzed instructions. Presuming that sd_init_insn is the > proper place for that, I'll test the following patch. > > > 2011-06-02 Alexander Monakov <amonakov@ispras.ru> > > * sched-deps.c (sd_init_insn): Initialize INSN_COND. > * sel-sched.c (move_op): Use correct type for 'res'. Verify that > code_motion_path_driver returned 0 or 1. Ok. Although I wonder how sel-sched can end up reusing an entry in h_d_i_d? How does it use this machinery? If it's not doing a normal forward scan as in sched_analyze, the INSN_COND mechanism may break in other ways. Bernd
On Thu, 2011-06-02 at 15:29 +0400, Alexander Monakov wrote: > Bernd, > > The problem is INSN_COND should be reset when initializing a new deps > structure, otherwise instructions may get stale conditions from other > previously analyzed instructions. Presuming that sd_init_insn is the > proper place for that, I'll test the following patch. > > > 2011-06-02 Alexander Monakov <amonakov@ispras.ru> > > * sched-deps.c (sd_init_insn): Initialize INSN_COND. > * sel-sched.c (move_op): Use correct type for 'res'. Verify that > code_motion_path_driver returned 0 or 1. I tested this patch on my IA64 HP-UX box and did not see any regressions. It fixed the problem I was having. Steve Ellcey sje@cup.hp.com
On Thu, 2011-06-02 at 15:30 -0700, Steve Ellcey wrote: > On Thu, 2011-06-02 at 15:29 +0400, Alexander Monakov wrote: > > Bernd, > > > > The problem is INSN_COND should be reset when initializing a new deps > > structure, otherwise instructions may get stale conditions from other > > previously analyzed instructions. Presuming that sd_init_insn is the > > proper place for that, I'll test the following patch. > > > > > > 2011-06-02 Alexander Monakov <amonakov@ispras.ru> > > > > * sched-deps.c (sd_init_insn): Initialize INSN_COND. > > * sel-sched.c (move_op): Use correct type for 'res'. Verify that > > code_motion_path_driver returned 0 or 1. > > I tested this patch on my IA64 HP-UX box and did not see any > regressions. It fixed the problem I was having. > > Steve Ellcey > sje@cup.hp.com Alexander, It may have spoke too soon. Last night's testing showed a regression that I didn't see yesterday. gfortran.dg/array_constructor_9.f90 is failing on IA64 HP-UX in 32 bit mode but not in 64 bit mode. It is also not failing on IA64 Linux. It fails with the options: -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions or -O3 -fomit-frame-pointer -funroll-loops The failure mode is: array_constructor_9.f90: In function 'gen': array_constructor_9.f90:14:0: internal compiler error: in code_motion_path_driver, at sel-sched.c:6573 Please submit a full bug report, with preprocessed source if appropriate. See <http://gcc.gnu.org/bugs.html> for instructions. Note that I am using r174594 sources with your patch *AND* with the patch Bernd proposed for PR 48673 (http://gcc.gnu.org/ml/gcc-patches/2011-05/msg02470.html). So this regression may be due to the other change. I will look into it some more to see if I can figure out which change is causing this regression. I did not see any other regressions. Steve Ellcey sje@cup.hp.com
On Thu, 2 Jun 2011, Bernd Schmidt wrote: > Ok. Although I wonder how sel-sched can end up reusing an entry in > h_d_i_d? How does it use this machinery? If it's not doing a normal > forward scan as in sched_analyze, the INSN_COND mechanism may break in > other ways. Indeed, that patch did not completely solve the problem, as stale INSN_CONDs can still be seen (I have a testcase reduced from combine.c). Selective scheduler's dependency analysis is certainly not limited to a single pass, as it will test more dependencies on the fly after e.g. creating bookkeeping. Was there some particular breakage you had in mind when mentioning that INSN_CONDs may break in other ways? Can you tell why you chose to place INSN_CONDs into HDID instead of HID? HID seems a bit more natural choice to me, and as it explicitely populated with zeros, it fixes the above combine.c testcase for me. However, moving INSN_CONDs to HID breaks sel-sched in a different way because sometimes HID is not extended early enough; if that approach is generally ok for you, I can see whether I can produce a working patch in that vein. FWIW, we have a patch that adds predication support for the selective scheduler (allowing the scheduler to transform insns into predicated form) that we plan to submit during this stage1. Thanks. Alexander
On 06/03/2011 07:44 PM, Alexander Monakov wrote: > > > On Thu, 2 Jun 2011, Bernd Schmidt wrote: > >> Ok. Although I wonder how sel-sched can end up reusing an entry in >> h_d_i_d? How does it use this machinery? If it's not doing a normal >> forward scan as in sched_analyze, the INSN_COND mechanism may break in >> other ways. > > Indeed, that patch did not completely solve the problem, as stale INSN_CONDs > can still be seen (I have a testcase reduced from combine.c). Selective > scheduler's dependency analysis is certainly not limited to a single pass, as > it will test more dependencies on the fly after e.g. creating bookkeeping. > > Was there some particular breakage you had in mind when mentioning that > INSN_CONDs may break in other ways? Well, sched-deps expects that we walk the insns sequentially, setting INSN_COND when an insn is seen for the first time, and then setting it to const_true_rtx once the condition is no longer valid for mutex_p comparisons. > Can you tell why you chose to place INSN_CONDs into HDID instead of HID? It's only used in sched-deps.c, during the scan. > FWIW, we have a patch that adds predication support for the selective > scheduler (allowing the scheduler to transform insns into predicated form) > that we plan to submit during this stage1. I have such a patch for sched-ebb. Bernd
> Note that I am using r174594 sources with your patch *AND* with the patch > Bernd proposed for PR 48673 (http://gcc.gnu.org/ml/gcc-patches/2011-05/msg02470.html). > So this regression may be due to the other change. I will look into it some > more to see if I can figure out which change is causing this regression. I did not > see any other regressions. > > Steve Ellcey > sje@cup.hp.com Alexander, Following up to my own email, removing Bernd's patch for PR 48673 did not make this failure go away. But Bernd's patch does fix the gfortran.dg/sms-* failures I reported in PR 48673 so unless you or Andrey have an objection to it, I would like to approve that patch for checkin. Steve Ellcey sje@cup.hp.com
diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c index 343d03c..e50f7ab 100644 --- a/gcc/sched-deps.c +++ b/gcc/sched-deps.c @@ -737,6 +737,7 @@ sd_init_insn (rtx insn) INSN_RESOLVED_BACK_DEPS (insn) = create_deps_list (); INSN_FORW_DEPS (insn) = create_deps_list (); INSN_RESOLVED_FORW_DEPS (insn) = create_deps_list (); + INSN_COND (insn) = NULL_RTX; /* ??? It would be nice to allocate dependency caches here. */ } diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c index 3f22a3c..cb95f44 100644 --- a/gcc/sel-sched.c +++ b/gcc/sel-sched.c @@ -6675,7 +6675,7 @@ move_op (insn_t insn, av_set_t orig_ops, expr_t expr_vliw, { struct moveop_static_params sparams; struct cmpd_local_params lparams; - bool res; + int res; /* Init params for code_motion_path_driver. */ sparams.dest = dest; @@ -6694,6 +6694,8 @@ move_op (insn_t insn, av_set_t orig_ops, expr_t expr_vliw, code_motion_path_driver_info = &move_op_hooks; res = code_motion_path_driver (insn, orig_ops, NULL, &lparams, &sparams); + gcc_assert (res != -1); + if (sparams.was_renamed) EXPR_WAS_RENAMED (expr_vliw) = true;