Re: [05/05] Fix PR 69102

[Andrey Belevantsev <abel@ispras.ru>]

[-- Attachment #1: Type: text/plain, Size: 2950 bytes --]

Hello,

On 14.03.2016 12:52, Andrey Belevantsev wrote:
> Hello,
>
> The problem here is readonly dependence contexts in selective scheduler.
> We're trying to cache the effect of initializing a dependence context with
> remembering that context and setting a readonly bit on it.  When first
> moving the insn 43 with REG_ARGS_SIZE note through the insn 3 (a simple eax
> set) we also set the last_args_size field of the context.  Later, when we
> make a copy of insn 43 and try to move it again through insn 3, we take the
> cached dependency context and notice the (fake) dep with last_args_size
> insn, which is the old insn 43.  Then the assert saying that we should be
> able to lift the bookkeeping copy up the same way as we did with the
> original insn breaks.
>
> Fixed by the attached patch that makes us notice only deps with the current
> producer insn.
>
> Ok for trunk?

We've discussed the patch with Alexander a bit and decided to take the 
different approach.  The core issue here is not handling the new 
last_args_size field in the readonly contexts.  In general, the readonly 
context approach, when analyzing an insn with a readonly context, would 
create the necessary dependencies with all of the last_ fields but refrain 
from modifying those fields.  The reason is we need to capture the effect 
of only the single producer in the readonly context.  Failing to do so may 
update the last_ fields with the effect of subsequent analyses and having 
the fake dependencies with the insns that got into those fields instead of 
having the dependencies with the currently used producer.

So the right fix here is to guard setting of the last_args_size field with 
!deps->readonly test as it is done elsewhere in the sched-deps.c.  In stage 
1 we will also want to set the asserts in the sel-sched dependency hooks 
(where I have placed early returns in the previous version of the patch) 
actually checking that the dependency is always created with the current 
producer, and such cases will be caught sooner.

The new patch bootstrapped and tested on x86-64 with selective scheduler 
forced enabled with no regressions.  It needs the maintainer outside of 
sel-sched as we touch sched-deps.c file.  Ok for trunk?  The test is the 
same as in previous patch.

Andrey

2016-03-15  Andrey Belevantsev  <abel@ispras.ru>

         PR rtl-optimization/69102
         * sched-deps.c (sched_analyze_insn): Do not set last_args_size field
         when we have a readonly dependency context.

>
> gcc/
>
> 2016-03-14  Andrey Belevantsev  <abel@ispras.ru>
>
>     PR rtl-optimization/69102
>     * sel-sched.c (has_dependence_note_dep): Only take into
>     account dependencies produced by the current producer insn.
>     (has_dependence_note_mem_dep): Likewise.
>
> testsuite/
>
> 2016-03-14  Andrey Belevantsev  <abel@ispras.ru>
>
>     PR rtl-optimization/69102
>     * gcc.c-torture/compile/pr69102.c: New test.
>
> Best,
> Andrey
>


[-- Attachment #2: 05-pr69102-2.diff --]
[-- Type: text/x-patch, Size: 424 bytes --]

diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c
index 3d4a1d5..77ffcd0 100644
--- a/gcc/sched-deps.c
+++ b/gcc/sched-deps.c
@@ -3495,7 +3495,8 @@ sched_analyze_insn (struct deps_desc *deps, rtx x, rtx_insn *insn)
     {
       if (deps->last_args_size)
 	add_dependence (insn, deps->last_args_size, REG_DEP_OUTPUT);
-      deps->last_args_size = insn;
+      if (!deps->readonly)
+	deps->last_args_size = insn;
     }
 }