On Thu, Dec 3, 2020 at 3:11 AM Jeff Law <l...@redhat.com> wrote:
>
>
>
> On 11/25/20 9:47 PM, Hongtao Liu wrote:
> > On Wed, Nov 25, 2020 at 7:37 PM Jakub Jelinek <ja...@redhat.com> wrote:
> >> On Wed, Nov 25, 2020 at 07:32:44PM +0800, Hongtao Liu wrote:
> >>> Update patch:
> >>> 1. ix86_expand_special_args_builtin is used for expanding mask load
> >>> intrinsics, this function will always convert the constant mask
> >>> operands into reg. So for the situation of all-ones mask, keep this
> >>> constant, and also change the mask operand predicate(of corresponding
> >>> expander) to register_or_constm1_operand.
> >>> 2. Delete last_arg_constant which is not used in
> >>> ix86_expand_special_args_builtin(maybe should be in a separate patch?)
> >> Yes, please make it a separate patch, it should go in first and
> >> should just drop last_arg_constant from that function plus the
> >> reindentation.
> >>
> >> Then post the PR97642 incremental to that.
> >>
> > Updated.
> >
> >> Thanks.
> >>
> >> Jakub
> >>
> >
> > 0002-Fix-incorrect-replacement-of-vmovdqu32-with-vpblendd.patch
> >
> > From b1256b6ef8f877244f4955b9205d53797424fc27 Mon Sep 17 00:00:00 2001
> > From: liuhongt <hongtao....@intel.com>
> > Date: Tue, 3 Nov 2020 17:26:43 +0800
> > Subject: [PATCH 2/2] Fix incorrect replacement of vmovdqu32 with vpblendd
> > which can cause fault.
> >
> > gcc/ChangeLog:
> >
> > PR target/97642
> > * config/i386/i386-expand.c
> > (ix86_expand_special_args_builtin): Don't move all-ones mask
> > operands into register.
> > * config/i386/sse.md (UNSPEC_MASKLOAD): New unspec.
> > (*<avx512>_load<mode>_mask): New define_insns for masked load
> > instructions.
> > (<avx512>_load<mode>_mask): Changed to define_expands which
> > specifically handle memory or all-ones mask operands.
> > (<avx512>_blendm<mode>): Changed to define_insns which are same
> > as original <avx512>_load<mode>_mask with adjustment of
> > operands order.
> > (*<avx512>_load<mode>): New define_insn_and_split which is
> > used to optimize for masked load with all one mask.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/i386/avx512bw-vmovdqu16-1.c: Adjust testcase to
> > make sure only masked load instruction is generated.
> > * gcc.target/i386/avx512bw-vmovdqu8-1.c: Ditto.
> > * gcc.target/i386/avx512f-vmovapd-1.c: Ditto.
> > * gcc.target/i386/avx512f-vmovaps-1.c: Ditto.
> > * gcc.target/i386/avx512f-vmovdqa32-1.c: Ditto.
> > * gcc.target/i386/avx512f-vmovdqa64-1.c: Ditto.
> > * gcc.target/i386/avx512vl-vmovapd-1.c: Ditto.
> > * gcc.target/i386/avx512vl-vmovaps-1.c: Ditto.
> > * gcc.target/i386/avx512vl-vmovdqa32-1.c: Ditto.
> > * gcc.target/i386/avx512vl-vmovdqa64-1.c: Ditto.
> > * gcc.target/i386/pr97642-1.c: New test.
> > * gcc.target/i386/pr97642-2.c: New test.
> > ---
> > [ ... ]
>
> > diff --git a/gcc/testsuite/gcc.target/i386/pr97642-2.c
> > b/gcc/testsuite/gcc.target/i386/pr97642-2.c
> > new file mode 100644
> > index 00000000000..eb06a2739b4
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/i386/pr97642-2.c
> > @@ -0,0 +1,77 @@
> > +/* PR target/97642 */
> > +/* { dg-do run } */
> > +/* { dg-options "-O2 -mavx512dq -mavx512vl -mavx512bw" } */
> > +/* { dg-require-effective-target avx512vl } */
> > +/* { dg-require-effective-target avx512dq } */
> > +/* { dg-require-effective-target avx512bw } */
> Given the uses of mprotect in this test, don't we need the test to be
> limited to systems where that's supported. Even just limiting to linux
> targets is probably sufficient.
>
> With that change, I think this is OK for the trunk.
Thanks for the review,committed.
>
> jeff
>
--
BR,
Hongtao
