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

[David Malcolm]

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.


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 

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

[Richard Henderson]

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~

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

[David Malcolm]

On Wed, 2014-08-27 at 09:11 -0700, Richard Henderson wrote:
 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?

Fair enough.

Update version of patch attached; again, only lightly tested so far.

* gcc/config/mn10300/mn10300.c (is_load_insn): Rename to...
(set_is_load_p): ...this, updating to work on a SET pattern rather
than an insn.
(is_store_insn): Rename to...
(set_is_store_p): ...this, updating to work on a SET pattern
rather than an insn.
(mn10300_adjust_sched_cost): Move call to get_attr_timings from
top of function to where it is needed.  Rewrite the bogus
condition that checks for insn and dep being PARALLEL to
instead use single_set, introducing locals insn_set and
dep_set.  Given that we only ever returned cost for a non-pair
of SETs, bail out early if we don't have a pair of SET.
Rewrite all uses of PATTERN (dep) and PATTERN (insn) to instead
use the new locals insn_set and dep_set, and update calls to
is_load_insn and is_store_insn to be calls to set_is_load_p and
set_is_store_p.

Index: gcc/config/mn10300/mn10300.c
===
--- gcc/config/mn10300/mn10300.c	(revision 214575)
+++ gcc/config/mn10300/mn10300.c	(working copy)
@@ -2742,21 +2742,15 @@
 }
 
 static inline bool
-is_load_insn (rtx insn)
+set_is_load_p (rtx set)
 {
-  if (GET_CODE (PATTERN (insn)) != SET)
-return false;
-
-  return MEM_P (SET_SRC (PATTERN (insn)));
+  return MEM_P (SET_SRC (set));
 }
 
 static inline bool
-is_store_insn (rtx insn)
+set_is_store_p (rtx set)
 {
-  if (GET_CODE (PATTERN (insn)) != SET)
-return false;
-
-  return MEM_P (SET_DEST (PATTERN (insn)));
+  return MEM_P (SET_DEST (set));
 }
 
 /* Update scheduling costs for situations that cannot be
@@ -2768,33 +2762,39 @@
 static int
 mn10300_adjust_sched_cost (rtx insn, rtx link, rtx dep, int cost)
 {
-  int timings = get_attr_timings (insn);
+  rtx insn_set;
+  rtx dep_set;
+  int timings;
 
   if (!TARGET_AM33)
 return 1;
 
-  if (GET_CODE (insn) == PARALLEL)
-insn = XVECEXP (insn, 0, 0);
+  /* We are only interested in pairs of SET. */
+  insn_set = single_set (insn);
+  if (!insn_set)
+return cost;
 
-  if (GET_CODE (dep) == PARALLEL)
-dep = XVECEXP (dep, 0, 0);
+  dep_set = single_set (dep);
+  if (!dep_set)
+return cost;
 
+  gcc_assert (GET_CODE (insn_set) == SET);
+  gcc_assert (GET_CODE (dep_set) == SET);
+
   /* 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))
+   set_is_load_p (dep_set)
+   set_is_store_p (insn_set))
 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)
+   ! set_is_store_p (insn_set)
! 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_MODE_CLASS (GET_MODE (SET_SRC (dep_set))) == MODE_FLOAT
+   GET_MODE_CLASS (GET_MODE (SET_SRC (insn_set))) == MODE_FLOAT)
 cost += 1;
 
   /*  Resolve the conflict described in section 1-7-4 of
@@ -2816,23 +2816,21 @@
 return cost;
 
   /* Check that the instruction 

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

[Richard Henderson]

On 08/27/2014 09:32 AM, David Malcolm wrote:
   * gcc/config/mn10300/mn10300.c (is_load_insn): Rename to...
   (set_is_load_p): ...this, updating to work on a SET pattern rather
   than an insn.
   (is_store_insn): Rename to...
   (set_is_store_p): ...this, updating to work on a SET pattern
   rather than an insn.
   (mn10300_adjust_sched_cost): Move call to get_attr_timings from
   top of function to where it is needed.  Rewrite the bogus
   condition that checks for insn and dep being PARALLEL to
   instead use single_set, introducing locals insn_set and
   dep_set.  Given that we only ever returned cost for a non-pair
   of SETs, bail out early if we don't have a pair of SET.
   Rewrite all uses of PATTERN (dep) and PATTERN (insn) to instead
   use the new locals insn_set and dep_set, and update calls to
   is_load_insn and is_store_insn to be calls to set_is_load_p and
   set_is_store_p.

Ok, if it passes your smoke tests.

 +  /* We are only interested in pairs of SET. */
 +  insn_set = single_set (insn);
 +  if (!insn_set)
 +return cost;
  
 +  dep_set = single_set (dep);
 +  if (!dep_set)
 +return cost;
  
 +  gcc_assert (GET_CODE (insn_set) == SET);
 +  gcc_assert (GET_CODE (dep_set) == SET);

I don't think you need the asserts; we should be able to trust single_set.


r~

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

[Richard Henderson]

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.


r~

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

[Jeff Law]

On 08/06/14 11:19, David Malcolm wrote:

gcc/
* config/mn10300/mn10300.c (mn10300_adjust_sched_cost): Fix the
handling of PARALLEL to work on PATTERN (insn) and PATTERN (dep),
rather than just on insn, dep themselves.  The latter are insns,
and thus can't be PARALLEL.

Approved.  Should go forward independently.

Jeff