Patchwork Initialize INSN_COND (was: C6X port 5/11: Track predication conditions more accurately)

login
register
mail settings
Submitter Alexander Monakov
Date June 2, 2011, 11:29 a.m.
Message ID <alpine.LNX.2.00.1106021513450.6250@monoid.intra.ispras.ru>
Download mbox | patch
Permalink /patch/98366/
State New
Headers show

Comments

Alexander Monakov - June 2, 2011, 11:29 a.m.
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.
Bernd Schmidt - June 2, 2011, 11:35 a.m.
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
Steve Ellcey - June 2, 2011, 10:30 p.m.
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
Steve Ellcey - June 3, 2011, 5:37 p.m.
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
Alexander Monakov - June 3, 2011, 5:44 p.m.
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
Bernd Schmidt - June 3, 2011, 5:49 p.m.
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
Steve Ellcey - June 3, 2011, 6:04 p.m.
> 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

Patch

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;