Re: [05/05] Fix PR 69102

[Jeff Law <law@redhat.com>]

On 03/15/2016 09:55 AM, Andrey Belevantsev wrote:
> 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.
OK.
jeff