This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 003/236] config/mn10300: Fix missing PATTERN in PARALLEL handling
- From: David Malcolm <dmalcolm at redhat dot com>
- To: Richard Henderson <rth at redhat dot com>, law at redhat dot com
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Wed, 27 Aug 2014 11:48:26 -0400
- Subject: Re: [PATCH 003/236] config/mn10300: Fix missing PATTERN in PARALLEL handling
- Authentication-results: sourceware.org; auth=none
- References: <1407345815-14551-1-git-send-email-dmalcolm at redhat dot com> <1407345815-14551-4-git-send-email-dmalcolm at redhat dot com> <53F3912D dot 6080403 at redhat dot com>
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 (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. */