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 <a...@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 <a...@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 <a...@ispras.ru>
PR rtl-optimization/69102
* gcc.c-torture/compile/pr69102.c: New test.
Best,
Andrey
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;
}
}
