On Tue, Aug 12, 2014 at 05:12:41PM -0400, Vladimir Makarov wrote:
> On 08/12/2014 03:35 PM, Jakub Jelinek wrote:
> > Hi!
> >
> > As detailed in the PR, find_inc ignored any possible clobbers on
> > inc_insn (typically %cc/flags/etc. register) and thus we could ignore
> > all register dependencies between mem_insn and inc_insn even when
> > we could only safely ignore the mem_reg0 register dependency.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > Is slightly modified version (just for different iteration over defs and
> > uses) ok for 4.9?
> >
> >
> Ok. Thanks for fixing this PR, Jakub.
Unfortunately, after looking at the code some more, there are further issues
(theoretical only, don't have testcases).
Here is an incremental patch that I'm bootstrapping/regtesting with
additional instrumentation.
Apparently find_inc handles both the cases where the increment can initially
dominate the mem insn or vice versa.
My patch from yesterday was trying to fix the case where we have:
mem_insn uses %rX in memory address, uses %flags
...
inc_insn %rX += const, clobbers %flags
where it is wrong to move inc_insn before mem_insn, because if it is moved
in between %flags setter before mem_insn and mem_insn, %flags might have
wrong value.
I think giving up for this if mem_insn is dominated originally by inc_insn
is unnecessary, there should be a dependency between %flags setter and
inc_insn which should prevent the move.
find_inc checks that inc_insn is single_set doing %rX += const or
%rX = %rY + const, so the only defs other than %rX IMHO must be clobbers.
But what I see as a problematic case (in my statistics dumps) is:
inc_insn %rX += const, clobbers %flags
...
mem_insn uses %rX in memory address, sets %flags
In this case, moving inc_insn after mem_insn is wrong, if inc_insn is moved
in between mem_insn and the user of %flags after it. This (potential)
wrong-code I've seen 9611 times in 32-bit code and so far 766 times in
64-bit code during the two bootstraps/regtests.
The case when both inc_insn and mem_insn both clobber the same reg (seems to
happen fairly often, with %flags) is IMHO not a problem.
Also, we've discussed with Vlad on IRC the (theoretical?) possibility that
inc_insn could have also USE rtxs in the pattern beyond the single set and
possible clobbers. In that case, we don't want to move it either way if
mem_insn defines the reg used in the USE. Note, no cases found in
x86_64-linux and i686-linux bootstrap statistics dumps.
So, does this make sense?
2014-08-14 Jakub Jelinek <ja...@redhat.com>
PR target/62025
* sched-deps.c (find_inc): Limit the test for inc_insn defs
vs. mem_insn uses to !backwards case only. Give up also if
any mem_insn def is used by inc_insn or if non-clobber
mem_insn def in backwards case is clobbered by inc_insn.
--- gcc/sched-deps.c.jj 2014-08-12 17:06:26.000000000 +0200
+++ gcc/sched-deps.c 2014-08-14 00:09:38.000000000 +0200
@@ -4751,24 +4751,54 @@ find_inc (struct mem_inc_info *mii, bool
"inc conflicts with store failure.\n");
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))
+ else
{
- df_ref use;
- FOR_EACH_INSN_USE (use, mii->mem_insn)
+ df_ref use, def2;
+ FOR_EACH_INSN_USE (use, mii->inc_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");
+ "mem def conflict with inc use failure.\n");
goto next;
}
+ /* DEFS in inc_insn other than mem_reg0 should be always
+ clobbers. If both inc_insn and mem_insn clobber the same
+ register, it is fine, but avoid the case where mem_insn e.g.
+ sets CC and originally earlier inc_insn clobbers it. */
+ if ((DF_REF_FLAGS (def) & DF_REF_MUST_CLOBBER) == 0
+ && backwards)
+ FOR_EACH_INSN_DEF (def2, mii->inc_insn)
+ if (reg_overlap_mentioned_p (DF_REF_REG (def),
+ DF_REF_REG (def2)))
+ {
+ if (sched_verbose >= 5)
+ fprintf (sched_dump,
+ "mem def conflict with inc def failure.\n");
+ goto next;
+ }
}
+ /* The inc instruction could have clobbers, make sure those
+ registers are not used in mem insn, if mem_insn is originally
+ earlier than inc_insn. */
+ if (!backwards)
+ 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 def conflict with mem use failure.\n");
+ goto next;
+ }
+ }
+
newaddr = mii->inc_input;
if (mii->mem_index != NULL_RTX)
newaddr = gen_rtx_PLUS (GET_MODE (newaddr), newaddr,
Jakub
