diff mbox

[003/236] config/mn10300: Fix missing PATTERN in PARALLEL handling

Message ID 1409154506.24896.75.camel@surprise
State New
Headers show

Commit Message

David Malcolm Aug. 27, 2014, 3:48 p.m. UTC 
On Tue, 2014-08-19 at 11:02 -0700, Richard Henderson wrote:
> On 08/06/2014 10:19 AM, David Malcolm wrote:
> > @@ -2772,11 +2772,11 @@ mn10300_adjust_sched_cost (rtx insn, rtx link, rtx dep, int cost)
> >    if (!TARGET_AM33)
> >      return 1;
> >  
> > -  if (GET_CODE (insn) == PARALLEL)
> > -    insn = XVECEXP (insn, 0, 0);
> > +  if (GET_CODE (PATTERN (insn)) == PARALLEL)
> > +    insn = XVECEXP (PATTERN (insn), 0, 0);
> >  
> > -  if (GET_CODE (dep) == PARALLEL)
> > -    dep = XVECEXP (dep, 0, 0);
> > +  if (GET_CODE (PATTERN (dep)) == PARALLEL)
> > +    dep = XVECEXP (PATTERN (dep), 0, 0);
> 
> I think these tests are simply wrong and should be removed.
> 
> Certainly one can't expect to extract the first element of an insn's pattern
> and then a few lines later test the pattern vs JUMP_P.

You're correct: both old and new versions of the code are confusing
insns with patterns.

I was able to trigger these lines of code by running with optimization
enabled with -fschedule-insns -mam33 (mn10300_processor >=
PROCESSOR_AM33 is needed to avoid mn10300_option_override from turning
off -fschedule-insns).

It does encounter insns with PARALLEL patterns e.g.:

(gdb) call debug(dep)
(insn 90 4 75 2 (parallel [
            (set (reg:SI 0 d0 [orig:58 D.1504 ] [58])
                (const_int 0 [0]))
            (clobber (reg:CC 51 EPSW))
        ]) /home/david/coding/gcc-python/smoketest/smoketest.c:30 7 {*movsi_clr}
     (expr_list:REG_UNUSED (reg:CC 51 EPSW)
        (nil)))

and my patch updates dep to the inner SET within the PARALLEL:
(gdb) call debug(dep)
(set (reg:SI 0 d0 [orig:58 D.1504 ] [58])
    (const_int 0 [0]))

which is a pattern, not an insn.  I think I was confusing PARALLEL with
SEQUENCE, where the elements of the latter are insns, but the elements
of the former are patterns.

I had a go at reworking the logic here to (I hope) properly handle a SET
within a PARALLEL (assuming some clobbers); patch attached.  Appears to
work (in light smoketesting); am working on running the full testsuite
with this build.  Alternatively, should this simply use "single_set"?
(though I think that's a more invasive change, especially since some of
the logic is for non-SETs).

	* gcc/config/mn10300/mn10300.c (is_load_insn): Rename to...
	(is_load_pat): ...this, updating to work on a pattern rather than
	an insn.
	(is_store_insn): Rename to...
	(is_store_pat): ...this, updating to work on a pattern rather than
	an insn.
	(mn10300_adjust_sched_cost): Rewrite the bogus condition that
	checks for "insn" and "dep" being PARALLEL to work on their patterns
	instead, introducing locals "insn_pat" and "dep_pat" to track
	these, updating if needed to the initial pattern within any such
	PARALLEL.  Rewrite all uses of PATTERN (dep) and PATTERN (insn) to
	instead use these new locals, and update calls to
	is_load_insn/is_store_insn to be calls to is_load_pat/is_store_pat.

Comments

Richard Henderson Aug. 27, 2014, 4:11 p.m. UTC | #1 
On 08/27/2014 08:48 AM, David Malcolm wrote:
> Alternatively, should this simply use "single_set"?

Yes.

> (though I think that's a more invasive change, especially since some of
> the logic is for non-SETs).

I don't think that's the case.  Take the tests in order:

  if (mn10300_tune_cpu == PROCESSOR_AM34
      && is_load_insn (dep)
      && is_store_insn (insn))
    cost += 1;

Requires sets for both.

  else if (mn10300_tune_cpu == PROCESSOR_AM34
      && ! is_store_insn (insn)
      && ! JUMP_P (insn)
      && GET_CODE (PATTERN (dep)) == SET
      && GET_CODE (PATTERN (insn)) == SET

Duh.

  if (GET_CODE (PATTERN (dep)) != SET)
    return cost;

Filtering out non-sets from dep.

  /* Now check to see if the previous instruction is a load or store.  */
  if (! is_load_insn (insn) && ! is_store_insn (insn))
    return cost;

Filtering out non-sets from insn.

Thus in no case do we return anything but the original "cost" when either the
dep or insn pattern is not a set.

Oh, and while you're massaging this function...

mn10300_adjust_sched_cost (rtx insn, rtx link, rtx dep, int cost)
{
  int timings = get_attr_timings (insn);
...
  /* Extract the latency value from the timings attribute.  */
  return timings < 100 ? (timings % 10) : (timings % 100);
}


Will you please move the (expensive) get_attr_timings call to the end, after
we've discarded all of the cases in which it isn't used?


r~
diff mbox

Patch

Index: gcc/config/mn10300/mn10300.c
===================================================================
--- gcc/config/mn10300/mn10300.c	(revision 214575)
+++ gcc/config/mn10300/mn10300.c	(working copy)
@@ -2742,21 +2742,21 @@ 
 }
 
 static inline bool
-is_load_insn (rtx insn)
+is_load_pat (rtx pat)
 {
-  if (GET_CODE (PATTERN (insn)) != SET)
+  if (GET_CODE (pat) != SET)
     return false;
 
-  return MEM_P (SET_SRC (PATTERN (insn)));
+  return MEM_P (SET_SRC (pat));
 }
 
 static inline bool
-is_store_insn (rtx insn)
+is_store_pat (rtx pat)
 {
-  if (GET_CODE (PATTERN (insn)) != SET)
+  if (GET_CODE (pat) != SET)
     return false;
 
-  return MEM_P (SET_DEST (PATTERN (insn)));
+  return MEM_P (SET_DEST (pat));
 }
 
 /* Update scheduling costs for situations that cannot be
@@ -2768,33 +2768,38 @@ 
 static int
 mn10300_adjust_sched_cost (rtx insn, rtx link, rtx dep, int cost)
 {
+  rtx insn_pat;
+  rtx dep_pat;
+
   int timings = get_attr_timings (insn);
 
   if (!TARGET_AM33)
     return 1;
 
-  if (GET_CODE (insn) == PARALLEL)
-    insn = XVECEXP (insn, 0, 0);
+  insn_pat = PATTERN (insn);
+  if (GET_CODE (insn_pat) == PARALLEL)
+    insn_pat = XVECEXP (insn_pat, 0, 0);
 
-  if (GET_CODE (dep) == PARALLEL)
-    dep = XVECEXP (dep, 0, 0);
+  dep_pat = PATTERN (dep);
+  if (GET_CODE (dep_pat) == PARALLEL)
+    dep_pat = XVECEXP (dep_pat, 0, 0);
 
   /* For the AM34 a load instruction that follows a
      store instruction incurs an extra cycle of delay.  */
   if (mn10300_tune_cpu == PROCESSOR_AM34
-      && is_load_insn (dep)
-      && is_store_insn (insn))
+      && is_load_pat (dep_pat)
+      && is_store_pat (insn_pat))
     cost += 1;
 
   /* For the AM34 a non-store, non-branch FPU insn that follows
      another FPU insn incurs a one cycle throughput increase.  */
   else if (mn10300_tune_cpu == PROCESSOR_AM34
-      && ! is_store_insn (insn)
+      && ! is_store_pat (insn_pat)
       && ! JUMP_P (insn)
-      && GET_CODE (PATTERN (dep)) == SET
-      && GET_CODE (PATTERN (insn)) == SET
-      && GET_MODE_CLASS (GET_MODE (SET_SRC (PATTERN (dep)))) == MODE_FLOAT
-      && GET_MODE_CLASS (GET_MODE (SET_SRC (PATTERN (insn)))) == MODE_FLOAT)
+      && GET_CODE (dep_pat) == SET
+      && GET_CODE (insn_pat) == SET
+      && GET_MODE_CLASS (GET_MODE (SET_SRC (dep_pat))) == MODE_FLOAT
+      && GET_MODE_CLASS (GET_MODE (SET_SRC (insn_pat))) == MODE_FLOAT)
     cost += 1;
 
   /*  Resolve the conflict described in section 1-7-4 of
@@ -2816,20 +2821,20 @@ 
     return cost;
 
   /* Check that the instruction about to scheduled is an FPU instruction.  */
-  if (GET_CODE (PATTERN (dep)) != SET)
+  if (GET_CODE (dep_pat) != SET)
     return cost;
 
-  if (GET_MODE_CLASS (GET_MODE (SET_SRC (PATTERN (dep)))) != MODE_FLOAT)
+  if (GET_MODE_CLASS (GET_MODE (SET_SRC (dep_pat))) != MODE_FLOAT)
     return cost;
 
   /* Now check to see if the previous instruction is a load or store.  */
-  if (! is_load_insn (insn) && ! is_store_insn (insn))
+  if (! is_load_pat (insn_pat) && ! is_store_pat (insn_pat))
     return cost;
 
   /* XXX: Verify: The text of 1-7-4 implies that the restriction
      only applies when an INTEGER load/store precedes an FPU
      instruction, but is this true ?  For now we assume that it is.  */
-  if (GET_MODE_CLASS (GET_MODE (SET_SRC (PATTERN (insn)))) != MODE_INT)
+  if (GET_MODE_CLASS (GET_MODE (SET_SRC (insn_pat))) != MODE_INT)
     return cost;
 
   /* Extract the latency value from the timings attribute.  */