diff mbox

Fix find_inc in the scheduler (PR target/62025)

Message ID 20140814155008.GH1784@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Aug. 14, 2014, 3:50 p.m. UTC 
On Thu, Aug 14, 2014 at 11:49:37AM +0200, Jakub Jelinek wrote:
> On Thu, Aug 14, 2014 at 11:34:04AM +0200, Jakub Jelinek wrote:
> > So, to set DEP_MULTIPLE even in the case where ask_depencency_caches
> > returns DEP_PRESENT, you'd need to find the old dependency anyway (isn't
> > that going to be expensive and totally kill all the effects of
> > true_dependency_cache?) and set DEP_MULTIPLE on it if not already set.
> 
> Something like (untested except for the openssl s390 testcase), haven't
> checked what effects it has on the number of successful find_inc cases.

Here is the same thing with ChangeLog entry, after bootstrap/regtest
on x86_64-linux and i686-linux.

With extra instrumentation (pretty much not removing the find_inc hunk and
adding the patch from yesterday too + statistics accumulation) shows
comparable amount of successful parse_add_or_inc in find_inc (around 1.2M), but to my
surprise still about 13000 cases where DEP_MULTIPLE is not set, yet there
are clearly more than one dependency between mem_inc and inc_insn.

The only testcase I've looked so far has been
-m32 -O3 -fomit-frame-pointer -funroll-loops -funroll-all-loops 990517-1.c
and what I see there is we have
sp += 24, clobber flags;
...
sp += 16, clobber flags;
...
flags = ([sp+1096] & eax) != 0;
The dep in between the middle insn and last insn has DEP_MULTIPLE set,
one dep there has been for sp setter vs. sp user and the other for
clobber flags vs. flags setter.  But for the first insn and third insn pair,
only the clobber flags vs. flags setter dependency has been recorded,
presumably the one on sp not, because there is some other setter after that.
I hope the scheduler doesn't attempt to swap sp += 24 with flags setter
because of the sp += 16 vs. flags setter dependency and sp += 24 vs. sp +=
16 dependency, but I feel kind of uneasy with find_inc assuming the recorded
dependency is the one for the mem_reg0, when in this case the dependency is
there for completely different register.

2014-08-14  Jakub Jelinek  <jakub@redhat.com>

	PR target/62025
	* sched-deps.c (add_or_update_dep_1): If ask_dependency_caches
	returned DEP_PRESENT, make sure to set DEP_MULTIPLE on present_dep.
	(find_inc): Revert 2014-08-13 change.



	Jakub

Comments

Bernd Schmidt Aug. 14, 2014, 3:59 p.m. UTC | #1 
On 08/14/2014 05:50 PM, Jakub Jelinek wrote:
> I hope the scheduler doesn't attempt to swap sp += 24 with flags setter
> because of the sp += 16 vs. flags setter dependency and sp += 24 vs. sp +=
> 16 dependency, but I feel kind of uneasy with find_inc assuming the recorded
> dependency is the one for the mem_reg0, when in this case the dependency is
> there for completely different register.

Let me think about that for a while. Thanks for debugging the cache problem.


Bernd
Jakub Jelinek Sept. 1, 2014, 9:03 a.m. UTC | #2 
On Thu, Aug 14, 2014 at 05:59:56PM +0200, Bernd Schmidt wrote:
> On 08/14/2014 05:50 PM, Jakub Jelinek wrote:
> >I hope the scheduler doesn't attempt to swap sp += 24 with flags setter
> >because of the sp += 16 vs. flags setter dependency and sp += 24 vs. sp +=
> >16 dependency, but I feel kind of uneasy with find_inc assuming the recorded
> >dependency is the one for the mem_reg0, when in this case the dependency is
> >there for completely different register.
> 
> Let me think about that for a while. Thanks for debugging the cache problem.

Did you have time to think about this?  Would prefer not to have this
unfixed for too long.

	Jakub
Bernd Schmidt Sept. 1, 2014, 9:52 a.m. UTC | #3 
On 09/01/2014 11:03 AM, Jakub Jelinek wrote:
> On Thu, Aug 14, 2014 at 05:59:56PM +0200, Bernd Schmidt wrote:
>> On 08/14/2014 05:50 PM, Jakub Jelinek wrote:
>>> I hope the scheduler doesn't attempt to swap sp += 24 with flags setter
>>> because of the sp += 16 vs. flags setter dependency and sp += 24 vs. sp +=
>>> 16 dependency, but I feel kind of uneasy with find_inc assuming the recorded
>>> dependency is the one for the mem_reg0, when in this case the dependency is
>>> there for completely different register.
>>
>> Let me think about that for a while. Thanks for debugging the cache problem.
>
> Did you have time to think about this?  Would prefer not to have this
> unfixed for too long.

Go ahead with your solution, I don't think I can really spare the time 
right now.


Bernd
diff mbox

Patch

--- gcc/sched-deps.c.jj	2014-08-12 17:06:26.000000000 +0200
+++ gcc/sched-deps.c	2014-08-14 11:46:25.509631874 +0200
@@ -1233,6 +1233,13 @@  add_or_update_dep_1 (dep_t new_dep, bool
       switch (ask_dependency_caches (new_dep))
 	{
 	case DEP_PRESENT:
+	  dep_t present_dep;
+	  sd_iterator_def sd_it;
+      
+	  present_dep = sd_find_dep_between_no_cache (DEP_PRO (new_dep),
+						      DEP_CON (new_dep),
+						      resolved_p, &sd_it);
+	  DEP_MULTIPLE (present_dep) = 1;
 	  return DEP_PRESENT;
 
 	case DEP_CHANGED:
@@ -4752,23 +4759,6 @@  find_inc (struct mem_inc_info *mii, bool
 		goto next;
 	      }
 
-	  /* The inc instruction could have clobbers, make sure those
-	     registers are not used in mem insn.  */
-	  FOR_EACH_INSN_DEF (def, mii->inc_insn)
-	    if (!reg_overlap_mentioned_p (DF_REF_REG (def), mii->mem_reg0))
-	      {
-		df_ref use;
-		FOR_EACH_INSN_USE (use, mii->mem_insn)
-		  if (reg_overlap_mentioned_p (DF_REF_REG (def),
-					       DF_REF_REG (use)))
-		    {
-		      if (sched_verbose >= 5)
-			fprintf (sched_dump,
-				 "inc clobber used in store failure.\n");
-		      goto next;
-		    }
-	      }
-
 	  newaddr = mii->inc_input;
 	  if (mii->mem_index != NULL_RTX)
 	    newaddr = gen_rtx_PLUS (GET_MODE (newaddr), newaddr,