Re: [PATCH] i386: Omit clobbers from vzeroupper until final [PR92190]

[Uros Bizjak <ubizjak@gmail.com>]

On Wed, Feb 5, 2020 at 11:05 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Feb 04, 2020 at 02:15:04PM +0100, Uros Bizjak wrote:
> > On Tue, Feb 4, 2020 at 2:13 PM Jakub Jelinek <jakub@redhat.com> wrote:
> > >
> > > On Tue, Feb 04, 2020 at 01:38:51PM +0100, Uros Bizjak wrote:
> > > > As Richard advised, let's put this safety stuff back. Usually, in
> > > > i386.md, these kind of splitters are implemented as two patterns, one
> > > > (define_insn_and_split) having "#", and the other (define_insn) with a
> > > > real insn. My opinion is, that this separation avoids confusion as
> > > > much as possible.
> > >
> > > Okay.  So like this if it passes bootstrap/regtest then?
> >
> > Yes.
> >
> > > 2020-02-04  Jakub Jelinek  <jakub@redhat.com>
> > >
> > >         PR target/92190
> > >         * config/i386/i386-features.c (ix86_add_reg_usage_to_vzeroupper): Only
> > >         include sets and not clobbers in the vzeroupper pattern.
> > >         * config/i386/sse.md (*avx_vzeroupper): Require in insn condition that
> > >         the parallel has 17 (64-bit) or 9 (32-bit) elts.
> > >         (*avx_vzeroupper_1): New define_insn_and_split.
> > >
> > >         * gcc.target/i386/pr92190.c: New test.
> >
> > OK.
>
> Unfortunately, it breaks
> +FAIL: gcc.target/i386/avx-2.c (internal compiler error)
> +FAIL: gcc.target/i386/avx-2.c (test for excess errors)
> +FAIL: gcc.target/i386/avx-vzeroupper-10.c (internal compiler error)
> +FAIL: gcc.target/i386/avx-vzeroupper-10.c (test for excess errors)
> +UNRESOLVED: gcc.target/i386/avx-vzeroupper-10.c scan-assembler-times avx_vzeroupper 3
> +FAIL: gcc.target/i386/avx-vzeroupper-11.c (internal compiler error)
> +FAIL: gcc.target/i386/avx-vzeroupper-11.c (test for excess errors)
> +UNRESOLVED: gcc.target/i386/avx-vzeroupper-11.c scan-assembler-times \\\\*avx_vzeroall 1
> +UNRESOLVED: gcc.target/i386/avx-vzeroupper-11.c scan-assembler-times avx_vzeroupper 3
> +FAIL: gcc.target/i386/avx-vzeroupper-12.c (internal compiler error)
> +FAIL: gcc.target/i386/avx-vzeroupper-12.c (test for excess errors)
> +UNRESOLVED: gcc.target/i386/avx-vzeroupper-12.c scan-assembler-times \\\\*avx_vzeroall 1
> +UNRESOLVED: gcc.target/i386/avx-vzeroupper-12.c scan-assembler-times avx_vzeroupper 4
> +FAIL: gcc.target/i386/avx-vzeroupper-5.c (internal compiler error)
> +FAIL: gcc.target/i386/avx-vzeroupper-5.c (test for excess errors)
> +UNRESOLVED: gcc.target/i386/avx-vzeroupper-5.c scan-assembler-times avx_vzeroupper 1
> +FAIL: gcc.target/i386/avx-vzeroupper-7.c (internal compiler error)
> +FAIL: gcc.target/i386/avx-vzeroupper-7.c (test for excess errors)
> +UNRESOLVED: gcc.target/i386/avx-vzeroupper-7.c scan-assembler-times avx_vzeroupper 1
> +FAIL: gcc.target/i386/avx-vzeroupper-8.c (internal compiler error)
> +FAIL: gcc.target/i386/avx-vzeroupper-8.c (test for excess errors)
> +UNRESOLVED: gcc.target/i386/avx-vzeroupper-8.c scan-assembler-times avx_vzeroupper 1
> +FAIL: gcc.target/i386/avx-vzeroupper-9.c (internal compiler error)
> +FAIL: gcc.target/i386/avx-vzeroupper-9.c (test for excess errors)
> +UNRESOLVED: gcc.target/i386/avx-vzeroupper-9.c scan-assembler-times avx_vzeroupper 4
> +FAIL: gcc.target/i386/sse-14.c (internal compiler error)
> +FAIL: gcc.target/i386/sse-14.c (test for excess errors)
> +FAIL: gcc.target/i386/sse-22.c (internal compiler error)
> +FAIL: gcc.target/i386/sse-22.c (test for excess errors)
> +FAIL: gcc.target/i386/sse-22a.c (internal compiler error)
> +FAIL: gcc.target/i386/sse-22a.c (test for excess errors)
>
> The problem is that x86 is the only target that defines HAVE_ATTR_length and
> doesn't schedule any splitting pass at -O0 after pro_and_epilogue.
>
> So, either we go back to handling this during vzeroupper output
> (unconditionally, rather than flag_ipa_ra guarded), or we need to tweak the
> split* passes for x86.
>
> Seems there are 5 split passes, split1 is run unconditionally before reload,
> split2 is run for optimize > 0 or STACK_REGS (x86) after ra but before
> epilogue_completed, split3 is run before regstack for STACK_REGS and
> optimize and -fno-schedule-insns2, split4 is run before sched2 if sched2 is
> run and split5 is run before shorten_branches if HAVE_ATTR_length and not
> STACK_REGS.
>
> Attached are 3 possible incremental patches for recog.c, all of them fix
> all the above regressions, but haven't fully bootstrapped/regtested any of
> them yet.  My preference would be the last one, which for -O0 and x86
> disables split2 and enables split3, as it doesn't add any extra passes.
> The first one just enables split3 for -O0 on x86, the second one enables
> split5 for -O0 on x86.

Please note that in i386.md we expect that the check for
"epilogue_completed" means split4 point. There are some places with:

  "TARGET_64BIT && ((optimize > 0 && flag_peephole2)
            ? epilogue_completed : reload_completed)

so for flag_peephole2, we split after peephole2 pass was performed.

Apparently, we already hit this problem in the past, so the check for
"optimize > 0" was added to solve -O0 -fpeephole2 combo.

Another one is with

   && TARGET_SSE_PARTIAL_REG_DEPENDENCY && epilogue_completed

where we assume that allocated registers won't change anymore when
breaking SSE partial register stall.

I think we should just enable split4 also for -O0. This would also
allow us to remove the "optimize > 0" check above and allow us to
generate a bit more optimal code even with -O0 for
TARGET_SSE_PARTIAL_REG_DEPENDENCY and TARGET_AVOID_FALSE_DEP_FOR_BMI.

Uros.

Uros.


>
>         Jakub