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

Submitted by Alexander Monakov on June 2, 2011, 11:29 a.m.

Details

Message ID alpine.LNX.2.00.1106021513450.6250@monoid.intra.ispras.ru
State New
Headers show

Commit Message

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.

Comments

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 hide | download patch | download mbox

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;