> -----Original Message-----
> From: Jakub Jelinek <ja...@redhat.com>
> Sent: Friday, February 7, 2025 4:08 PM
> To: Liu, Hongtao <hongtao....@intel.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: [PATCH] i386: Fix ICE with conditional QI/HI vector maxmin
> [PR118776]
>
> Hi!
>
> The following testcase ICEs starting with GCC 12 since r12-4526 although the
> bug has been introduced already in r12-2751.
> The problem was in the addition of cond_<code><mode> define_expand
> which uses nonimmediate_operand predicates for both maxmin operands for
> all VI1248_AVX512VLBW modes. It works fine with VI48_AVX512VL modes
> because the <code><mode>3_mask VI48_AVX512VL define_expand uses
> ix86_fixup_binary_operands_no_copy and the
> *avx512f_<code><mode>3<mask_name> VI48_AVX512VL define_insn uses %
> in constraint and !(MEM_P && MEM_P) check in condition (and
> <code><mode>3 define_expand with VI124_256_AVX512F_AVX512BW
> iterator does that too), but eventhough the 8-bit and 16-bit element maxmin
> is commutative too, the <mask_codefor><code><mode>3<mask_name>
> define_insn with VI12_AVX512VL iterator didn't use % in constraint to make it
> commutative. So, e.g. cond_umaxv32qi define_expand allowed
> nonimmediate_operand for both umax operands, but used
> gen_umaxv32qi_mask which wasn't commutative and only allowed
> nonimmediate_operand for the second operand.
>
> The following patch fixes it by keeping the <code><mode>3
> VI124_256_AVX512F_AVX512BW define_expand as is (it does
> ix86_fixup_binary_operands_no_copy) but extending the
> <code><mode>3_mask define_expand from VI48_AVX512VL to
> VI1248_AVX512VLBW which keeps the current modes with their ISA
> conditions and adds the VI12_AVX512VL modes under additional
> TARGET_AVX512BW condition, and turning the actual define_insn into an *
> prefixed name (which it was before just for the non-masked
> case) and having the same commutative operand handling as in other
> define_insns.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Ok, thanks.
>
> 2025-02-07 Jakub Jelinek <ja...@redhat.com>
>
> PR target/118776
> * config/i386/sse.md (<code><mode>3_mask): Use
> VI1248_AVX512VLBW
> iterator rather than VI48_AVX512VL.
> (<mask_codefor><code><mode>3<mask_name>): Rename to ...
> (*avx512bw_<code><mode>3<mask_name>): ... this. Use
> nonimmediate_operand rather than register_operand predicate
> and %v
> rather than v constraint for operand 1 and adjust condition to reject
> MEMs in both operand 1 and 2.
>
> * gcc.target/i386/pr118776.c: New test.
>
> --- gcc/config/i386/sse.md.jj 2025-01-23 15:54:53.160911648 +0100
> +++ gcc/config/i386/sse.md 2025-02-07 00:16:49.155363094 +0100
> @@ -17703,12 +17703,12 @@ (define_expand "cond_<code><mode>"
> })
>
> (define_expand "<code><mode>3_mask"
> - [(set (match_operand:VI48_AVX512VL 0 "register_operand")
> - (vec_merge:VI48_AVX512VL
> - (maxmin:VI48_AVX512VL
> - (match_operand:VI48_AVX512VL 1 "nonimmediate_operand")
> - (match_operand:VI48_AVX512VL 2 "nonimmediate_operand"))
> - (match_operand:VI48_AVX512VL 3 "nonimm_or_0_operand")
> + [(set (match_operand:VI1248_AVX512VLBW 0 "register_operand")
> + (vec_merge:VI1248_AVX512VLBW
> + (maxmin:VI1248_AVX512VLBW
> + (match_operand:VI1248_AVX512VLBW 1
> "nonimmediate_operand")
> + (match_operand:VI1248_AVX512VLBW 2
> "nonimmediate_operand"))
> + (match_operand:VI1248_AVX512VLBW 3
> "nonimm_or_0_operand")
> (match_operand:<avx512fmaskmode> 4 "register_operand")))]
> "TARGET_AVX512F"
> "ix86_fixup_binary_operands_no_copy (<CODE>, <MODE>mode,
> operands);") @@ -17724,12 +17724,12 @@ (define_insn
> "*avx512f_<code><mode>3<mas
> (set_attr "prefix" "maybe_evex")
> (set_attr "mode" "<sseinsnmode>")])
>
> -(define_insn "<mask_codefor><code><mode>3<mask_name>"
> +(define_insn "*avx512bw_<code><mode>3<mask_name>"
> [(set (match_operand:VI12_AVX512VL 0 "register_operand" "=v")
> (maxmin:VI12_AVX512VL
> - (match_operand:VI12_AVX512VL 1 "register_operand" "v")
> + (match_operand:VI12_AVX512VL 1 "nonimmediate_operand" "%v")
> (match_operand:VI12_AVX512VL 2 "nonimmediate_operand" "vm")))]
> - "TARGET_AVX512BW"
> + "TARGET_AVX512BW && !(MEM_P (operands[1]) && MEM_P
> (operands[2]))"
>
> "vp<maxmin_int><ssemodesuffix>\t{%2, %1, %0<mask_operand3>|%0<mask
> _operand3>, %1, %2}"
> [(set_attr "type" "sseiadd")
> (set_attr "prefix" "evex")
> --- gcc/testsuite/gcc.target/i386/pr118776.c.jj 2025-02-07
> 08:41:46.054157905 +0100
> +++ gcc/testsuite/gcc.target/i386/pr118776.c 2025-02-07
> 08:40:30.508196302 +0100
> @@ -0,0 +1,23 @@
> +/* PR target/118776 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mavx512bw -mavx512vl" } */
> +
> +void bar (unsigned char *);
> +
> +void
> +foo (unsigned char *x)
> +{
> + unsigned char b[32];
> + bar (b);
> + for (int i = 0; i < 32; i++)
> + {
> + unsigned char c = 8;
> + if (i > 3)
> + {
> + unsigned char d = b[i];
> + d = 1 > d ? 1 : d;
> + c = d;
> + }
> + x[i] = c;
> + }
> +}
>
> Jakub
