Re: [PATCH][PR target/97642] Fix incorrect replacement of vmovdqu32 with vpblendd.

[Hongtao Liu <crazylht@gmail.com>]

On Tue, Nov 24, 2020 at 4:27 AM Jeff Law <law@redhat.com> wrote:
>
>
>
> On 11/4/20 2:19 AM, Hongtao Liu via Gcc-patches wrote:
> > Hi:
> >   When programmers explicitly use mask loaded intrinsics, don't
> > transform the instruction to vpblend{b,w,d,q} since If mem_addr points
> > to a memory region with less than whole vector size of accessible
> > memory,  the mask would prevent reading the inaccessible bytes which
> > could avoid fault.
> >
> >   Bootstrap is ok, gcc regress test for i386/x86_64 backend is ok.
> >   Ok for trunk?
> >
> > gcc/ChangeLog:
> >
> >         PR target/97642
> >         * 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 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.
> >
> >
> > 0001-Fix-incorrect-replacement-of-vmovdqu32-with-vpblendd.patch
> >
> > From 48cf0adcd55395653891888f4768b8bdc19786f2 Mon Sep 17 00:00:00 2001
> > From: liuhongt <hongtao.liu@intel.com>
> > Date: Tue, 3 Nov 2020 17:26:43 +0800
> > Subject: [PATCH] Fix incorrect replacement of vmovdqu32 with vpblendd which
> >  can cause fault.
> >
> > gcc/ChangeLog:
> >
> >       PR target/97642
> >       * 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 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.
> So in the BZ Jakub asked for the all-ones mask case to be specially
> handled to emit a normal load.  I don't see where we're handling that.
> ISTM that we'd want a test for that too.  Right?
>

all-ones mask would be simplified to a simple load but with unspec in
set_src and would be handled by the following

+(define_insn_and_split "*<avx512>_load<mode>"
+  [(set (match_operand:V48_AVX512VL 0 "register_operand")
+ (unspec:V48_AVX512VL
+   [(match_operand:V48_AVX512VL 1 "memory_operand")]
+   UNSPEC_MASKLOAD))]
+  "TARGET_AVX512F"
+  "#"
+  "&& 1"
+  [(set (match_dup 0) (match_dup 1))])

and the corresponding testcase is

new file   gcc/testsuite/gcc.target/i386/pr97642-1.c
@@ -0,0 +1,23 @@
+/* PR target/97642 */
+/* { dg-do compile } */
+/* { dg-options "-mavx512vl -O2" } */
+/* { dg-final { scan-assembler-not { k[0-8] } } } */
+
+#include <immintrin.h>
+__m128i
+foo1 (__m128i src, void const* P)
+{
+  return _mm_mask_loadu_epi32 (src, 15, P);
+}
+
+__m256i
+foo2 (__m256i src, void const* P)
+{
+  return _mm256_mask_loadu_epi32 (src, 255, P);
+}
+
+__m512i
+foo3 (__m512i src, void const* P)
+{
+  return _mm512_mask_loadu_epi32 (src, 65535 , P);
+}


> WIth that in place and tested, this is probably ready for the trunk.
>
> jeff
>
>


-- 
BR,
Hongtao