Message ID | 20140813223059.GB1784@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
On 2014-08-13, 6:30 PM, Jakub Jelinek wrote: > 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? > What a brain damaged excercise to analyze cases for the patch especially at the end of day:) But I believe the patch is ok. Only minor thing. Please, read my comments below. > 2014-08-14 Jakub Jelinek <jakub@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. It can be unused set too (not only clobbers) and single_set will still return non-null. inc_insn: def R, def R2, unused R2 mem_insn: use R, def R2 ... use R2 But the code still works in this case. So I'd only modify the comment. 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 >
--- 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,