AMD General Hi,
> -----Original Message----- > From: Jiang, Haochen <[email protected]> > Sent: 28 July 2026 12:36 > To: Sharma, Dipesh <[email protected]>; [email protected] > Cc: Liu, Hongtao <[email protected]>; [email protected]; > [email protected]; Kumar, Venkataramanan > <[email protected]> > Subject: RE: [PATCH v3 7/7] [X86]: Add Sub-byte element extration and > Symmetric-signed saturation narrow support. > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > > From: Dipesh Sharma <[email protected]> > > Sent: Monday, July 27, 2026 5:38 PM > > > > diff --git a/gcc/config/i386/avx10v2auxintrin.h > > b/gcc/config/i386/avx10v2auxintrin.h > > index bab9cb1475b..9dccb8bec93 100644 > > --- a/gcc/config/i386/avx10v2auxintrin.h > > +++ b/gcc/config/i386/avx10v2auxintrin.h > > +// VUNPACKB - 128-bit > > +#ifdef __OPTIMIZE__ > > +extern __inline __m128i > > +__attribute__ ((__gnu_inline__, __always_inline__, __artificial__)) > > +_mm_unpackb_epi8 (__m128i __A, const int __B) > > We do not need "b" in intrin name, it has already been mentioned as epi8. It > should be __mm_unpack_epi8. Okay. > > > diff --git a/gcc/config/i386/i386-expand.cc > > b/gcc/config/i386/i386-expand.cc index b82a4913b4e..e8bf4ee1ba8 > 100644 > > --- a/gcc/config/i386/i386-expand.cc > > +++ b/gcc/config/i386/i386-expand.cc > > @@ -13321,6 +13321,9 @@ ix86_expand_args_builtin (const struct > > builtin_description *d, > > case V4DF_FTYPE_V8DF_INT_V4DF_UQI: > > case V4SF_FTYPE_V16SF_INT_V4SF_UQI: > > case V8DI_FTYPE_V8DI_INT_V8DI_UQI: > > + case V16QI_FTYPE_V16QI_INT_V16QI_UHI: > > + case V32QI_FTYPE_V32QI_INT_V32QI_USI: > > + case V64QI_FTYPE_V64QI_INT_V64QI_UDI: > > nargs = 4; > > mask_pos = 2; > > nargs_constant = 1; > > @@ -13571,6 +13574,11 @@ ix86_expand_args_builtin (const struct > > builtin_description *d, > > case CODE_FOR_avx512fp16_cmpv8hf3_mask: > > error ("the last argument must be a 5-bit immediate"); > > return const0_rtx; These cases and following default are gated by if (!match) block. As per my understanding now, we should put the stricter check before predicate mismatch block start for loose predicate const_0_to_63_operand. > > + case CODE_FOR_vunpackbv16qi_mask: > > + case CODE_FOR_vunpackbv32qi_mask: > > + case CODE_FOR_vunpackbv64qi_mask: > > + error ("the last argument must not use reserved value > > immediate"); > > + return const0_rtx; > Since we will be adding the stricter check before the if (!match) block, we would not need to add these cases again. > We still need the stricter restrictions like I mentioned in v2 comment here > since > in documentation, it explicitly mentioned: > > Software should refrain from using RESERVED encodings, however no #UD or > #GP is enforced. > > Thus, it is software's job to prohibit those reserved immediates. > > It is hard for predicates to do that complicated thing and at least for now, > everyone needs to use intrin for unpack, and will use this code path to > generate the instructions. So currently, this is the best place to add the > check. > > You even do not need to add a new const_8_to_63_operand in predicates if > you use a stricter check here since all the code will pass here, then > patterns. I agree. > > > > default: > > switch (nargs_constant) > > diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md index > > de5b7770332..1e032d39172 100644 > > --- a/gcc/config/i386/sse.md > > +++ b/gcc/config/i386/sse.md > > + > > +;; VPMOVSSDB - Symmetric signed saturation narrow (32-bit to 8-bit) > > + > > +(define_mode_attr pmovss_dest > > + [(V4SI "V4QI") (V8SI "V8QI") (V16SI "V16QI")]) > > + > > +(define_expand "avx10v2aux_sym_truncatev4siv4qi2" > > + [(set (match_operand:V16QI 0 "register_operand") > > + (vec_concat:V16QI > > + (unspec:V4QI > > + [(match_operand:V4SI 1 "register_operand")] > > + UNSPEC_VPMOVSSDB) > > + (match_dup 2)))] > > + "TARGET_AVX10V2AUX" > > + "operands[2] = CONST0_RTX (V12QImode);") > > + > > +(define_insn "*avx10v2aux_sym_truncatev4siv4qi2" > > + [(set (match_operand:V16QI 0 "register_operand" "=v") > > + (vec_concat:V16QI > > + (unspec:V4QI > > + [(match_operand:V4SI 1 "register_operand" "v")] > > + UNSPEC_VPMOVSSDB) > > + (match_operand:V12QI 2 "const0_operand")))] > > + "TARGET_AVX10V2AUX" > > + "vpmovssdb\t{%1, %0|%0, %1}" > > + [(set_attr "prefix" "evex") > > + (set_attr "mode" "V4SI")]) > > The mode here and nearly all the occurrence for vpmovssdb is not matching > <sseinsnmode>. > > Also, we still need to use nonimmediate_operand with corresponding m > constraint to let common intrin could also generate memory insts. But be > cautious ... > > > + > > +(define_expand "avx10v2aux_sym_truncatev8siv8qi2" > > + [(set (match_operand:V16QI 0 "register_operand") > > + (vec_concat:V16QI > > + (unspec:V8QI > > + [(match_operand:V8SI 1 "register_operand")] > > + UNSPEC_VPMOVSSDB) > > + (match_dup 2)))] > > + "TARGET_AVX10V2AUX" > > + "operands[2] = CONST0_RTX (V8QImode);") > > + > > +(define_insn "*avx10v2aux_sym_truncatev8siv8qi2" > > + [(set (match_operand:V16QI 0 "register_operand" "=v") > > + (vec_concat:V16QI > > + (unspec:V8QI > > + [(match_operand:V8SI 1 "register_operand" "v")] > > + UNSPEC_VPMOVSSDB) > > + (match_operand:V8QI 2 "const0_operand")))] > > + "TARGET_AVX10V2AUX" > > + "vpmovssdb\t{%1, %0|%0, %1}" > > + [(set_attr "prefix" "evex") > > + (set_attr "mode" "V8SI")]) > > + > > +(define_expand "avx10v2aux_sym_truncatev4siv4qi2_mask" > > + [(set (match_operand:V16QI 0 "register_operand") > > + (vec_concat:V16QI > > + (vec_merge:V4QI > > + (unspec:V4QI > > + [(match_operand:V4SI 1 "register_operand")] > > + UNSPEC_VPMOVSSDB) > > + (vec_select:V4QI > > + (match_operand:V16QI 2 "nonimm_or_0_operand") > > + (parallel [(const_int 0) (const_int 1) > > + (const_int 2) (const_int 3)])) > > + (match_operand:QI 3 "register_operand")) > > + (match_dup 4)))] > > + "TARGET_AVX10V2AUX" > > + "operands[4] = CONST0_RTX (V12QImode);") > > + > > +(define_insn "*avx10v2aux_sym_truncatev4siv4qi2_mask" > > + [(set (match_operand:V16QI 0 "register_operand" "=v") > > + (vec_concat:V16QI > > + (vec_merge:V4QI > > + (unspec:V4QI > > + [(match_operand:V4SI 1 "register_operand" "v")] > > + UNSPEC_VPMOVSSDB) > > + (vec_select:V4QI > > + (match_operand:V16QI 2 "nonimm_or_0_operand" "0C") > > ... you will need to use 0 for m constraint here. Thanks, ill take care of it. > > > + (parallel [(const_int 0) (const_int 1) > > + (const_int 2) (const_int 3)])) > > + (match_operand:QI 3 "register_operand" "Yk")) > > + (match_operand:V12QI 4 "const0_operand")))] > > + "TARGET_AVX10V2AUX" > > + "vpmovssdb\t{%1, %0%{%3%}%N2|%0%{%3%}%N2, %1}" > > + [(set_attr "prefix" "evex") > > + (set_attr "mode" "V4SI")]) > > + > > > > +(define_insn > > "avx10v2aux_sym_truncate<mode>v<ssescalarnum>qi2_mask_store_1" > > + [(set (match_operand:<pmovss_dest> 0 "memory_operand" "=m") > > + (vec_merge:<pmovss_dest> > > + (unspec:<pmovss_dest> > > + [(match_operand:VI4_AVX512F 1 "register_operand" "v")] > > + UNSPEC_VPMOVSSDB) > > + (match_dup 0) > > + (match_operand:<avx512fmaskmode> 2 "register_operand" "Yk")))] > > + "TARGET_AVX10V2AUX" > > + "vpmovssdb\t{%1, %0%{%2%}|%0%{%2%}, %1}" > > + [(set_attr "type" "ssemov") > > + (set_attr "memory" "store") > > + (set_attr "prefix" "evex") > > + (set_attr "mode" "<sseinsnmode>")]) > > + > > +(define_expand > > "avx10v2aux_sym_truncate<mode>v<ssescalarnum>qi2_mask_store_2" > > + [(match_operand:<pmovss_dest> 0 "memory_operand") > > + (unspec:<pmovss_dest> > > + [(match_operand:VI4_AVX2 1 "register_operand")] > > + UNSPEC_VPMOVSSDB) > > + (match_operand:<avx512fmaskmode> 2 "register_operand")] > > + "TARGET_AVX10V2AUX" > > +{ > > + operands[0] = adjust_address_nv (operands[0], <pmovss_dest>mode, > > +0); > > + emit_insn > > (gen_avx10v2aux_sym_truncate<mode>v<ssescalarnum>qi2_mask_store_1 > > + (operands[0], operands[1], operands[2])); > > + DONE; > > +}) > > It is a little tricky but okayish to me, pending for other inputs. > > Thx, > Haochen Thanks, Dipesh.
