[PATCH 0/2] x86: vec_extract_* adjustments

[PATCH 0/2] x86: vec_extract_* adjustments

[Jan Beulich]

1: correct / simplify @vec_extract_hi_<mode> and vec_extract_hi_v32qi
2: slightly correct / simplify *vec_extractv2ti

Jan

[PATCH 1/2] x86: correct / simplify @vec_extract_hi_<mode> and vec_extract_hi_v32qi

[Jan Beulich]

The middle alternative each was unusable without enabling AVX512DQ (in
addition to AVX512VL), which is entirely unrelated here. The last
alternative is usable with AVX512VL only (due to type restrictions on
what may be put in the upper 16 YMM registers), and hence is pointlessly
forcing 512-bit mode (without actually reflecting that in the "mode"
attribute).

gcc/

	* config/i386/sse.md (@vec_extract_hi_<mode>): Drop last
	alternative. Switch new last alternative's "isa" attribute to
	"avx512vl".
	(vec_extract_hi_v32qi): Likewise.
---
Like elsewhere I suspect "prefix_extra" is bogus here and should be
dropped.

Is "sselog1" actually appropriate here? Extracts are special forms of
moves after all, not logical operations. Even "sseshuf1" would seem to
come closer.

--- a/gcc/config/i386/sse.md
+++ b/gcc/config/i386/sse.md
@@ -12029,9 +12029,9 @@
   "operands[1] = gen_lowpart (<ssehalfvecmode>mode, operands[1]);")
 
 (define_insn "@vec_extract_hi_<mode>"
-  [(set (match_operand:<ssehalfvecmode> 0 "nonimmediate_operand" "=xm,vm,vm")
+  [(set (match_operand:<ssehalfvecmode> 0 "nonimmediate_operand" "=xm,vm")
 	(vec_select:<ssehalfvecmode>
-	  (match_operand:V16_256 1 "register_operand" "x,v,v")
+	  (match_operand:V16_256 1 "register_operand" "x,v")
 	  (parallel [(const_int 8) (const_int 9)
 		     (const_int 10) (const_int 11)
 		     (const_int 12) (const_int 13)
@@ -12039,13 +12039,12 @@
   "TARGET_AVX"
   "@
    vextract%~128\t{$0x1, %1, %0|%0, %1, 0x1}
-   vextracti32x4\t{$0x1, %1, %0|%0, %1, 0x1}
-   vextracti32x4\t{$0x1, %g1, %0|%0, %g1, 0x1}"
+   vextracti32x4\t{$0x1, %1, %0|%0, %1, 0x1}"
   [(set_attr "type" "sselog1")
    (set_attr "prefix_extra" "1")
    (set_attr "length_immediate" "1")
-   (set_attr "isa" "*,avx512dq,avx512f")
-   (set_attr "prefix" "vex,evex,evex")
+   (set_attr "isa" "*,avx512vl")
+   (set_attr "prefix" "vex,evex")
    (set_attr "mode" "OI")])
 
 (define_insn_and_split "vec_extract_lo_v64qi"
@@ -12144,9 +12143,9 @@
   "operands[1] = gen_lowpart (V16QImode, operands[1]);")
 
 (define_insn "vec_extract_hi_v32qi"
-  [(set (match_operand:V16QI 0 "nonimmediate_operand" "=xm,vm,vm")
+  [(set (match_operand:V16QI 0 "nonimmediate_operand" "=xm,vm")
 	(vec_select:V16QI
-	  (match_operand:V32QI 1 "register_operand" "x,v,v")
+	  (match_operand:V32QI 1 "register_operand" "x,v")
 	  (parallel [(const_int 16) (const_int 17)
 		     (const_int 18) (const_int 19)
 		     (const_int 20) (const_int 21)
@@ -12158,13 +12157,12 @@
   "TARGET_AVX"
   "@
    vextract%~128\t{$0x1, %1, %0|%0, %1, 0x1}
-   vextracti32x4\t{$0x1, %1, %0|%0, %1, 0x1}
-   vextracti32x4\t{$0x1, %g1, %0|%0, %g1, 0x1}"
+   vextracti32x4\t{$0x1, %1, %0|%0, %1, 0x1}"
   [(set_attr "type" "sselog1")
    (set_attr "prefix_extra" "1")
    (set_attr "length_immediate" "1")
-   (set_attr "isa" "*,avx512dq,avx512f")
-   (set_attr "prefix" "vex,evex,evex")
+   (set_attr "isa" "*,avx512vl")
+   (set_attr "prefix" "vex,evex")
    (set_attr "mode" "OI")])
 
 ;; NB: *vec_extract<mode>_0 must be placed before *vec_extracthf.

[PATCH 2/2] x86: slightly correct / simplify *vec_extractv2ti

[Jan Beulich]

V2TImode values cannot appear in the upper 16 YMM registers without
AVX512VL being enabled. Therefore forcing 512-bit mode (also not
reflected in the "mode" attribute) is pointless.

gcc/

	* config/i386/sse.md (*vec_extractv2ti): Drop g modifiers.

--- a/gcc/config/i386/sse.md
+++ b/gcc/config/i386/sse.md
@@ -20115,7 +20115,7 @@
   "TARGET_AVX"
   "@
    vextract%~128\t{%2, %1, %0|%0, %1, %2}
-   vextracti32x4\t{%2, %g1, %0|%0, %g1, %2}"
+   vextracti32x4\t{%2, %1, %0|%0, %1, %2}"
   [(set_attr "type" "sselog")
    (set_attr "prefix_extra" "1")
    (set_attr "length_immediate" "1")

Re: [PATCH 1/2] x86: correct / simplify @vec_extract_hi_<mode> and vec_extract_hi_v32qi

[Hongtao Liu]

On Wed, Jul 5, 2023 at 4:00 PM Jan Beulich via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> The middle alternative each was unusable without enabling AVX512DQ (in
> addition to AVX512VL), which is entirely unrelated here. The last
> alternative is usable with AVX512VL only (due to type restrictions on
> what may be put in the upper 16 YMM registers), and hence is pointlessly
> forcing 512-bit mode (without actually reflecting that in the "mode"
> attribute).
Ok.
>
> gcc/
>
>         * config/i386/sse.md (@vec_extract_hi_<mode>): Drop last
>         alternative. Switch new last alternative's "isa" attribute to
>         "avx512vl".
>         (vec_extract_hi_v32qi): Likewise.
> ---
> Like elsewhere I suspect "prefix_extra" is bogus here and should be
> dropped.
>
> Is "sselog1" actually appropriate here? Extracts are special forms of
> moves after all, not logical operations. Even "sseshuf1" would seem to
> come closer.
Honestly, I don't know why it's marked as sselog1, but looking at the
code,  almost all vec_extract patterns are marked as sselog1, guess
it's originally from pextr.
Agree that it's should be more close to shuffle instructions.
>
> --- a/gcc/config/i386/sse.md
> +++ b/gcc/config/i386/sse.md
> @@ -12029,9 +12029,9 @@
>    "operands[1] = gen_lowpart (<ssehalfvecmode>mode, operands[1]);")
>
>  (define_insn "@vec_extract_hi_<mode>"
> -  [(set (match_operand:<ssehalfvecmode> 0 "nonimmediate_operand" "=xm,vm,vm")
> +  [(set (match_operand:<ssehalfvecmode> 0 "nonimmediate_operand" "=xm,vm")
>         (vec_select:<ssehalfvecmode>
> -         (match_operand:V16_256 1 "register_operand" "x,v,v")
> +         (match_operand:V16_256 1 "register_operand" "x,v")
>           (parallel [(const_int 8) (const_int 9)
>                      (const_int 10) (const_int 11)
>                      (const_int 12) (const_int 13)
> @@ -12039,13 +12039,12 @@
>    "TARGET_AVX"
>    "@
>     vextract%~128\t{$0x1, %1, %0|%0, %1, 0x1}
> -   vextracti32x4\t{$0x1, %1, %0|%0, %1, 0x1}
> -   vextracti32x4\t{$0x1, %g1, %0|%0, %g1, 0x1}"
> +   vextracti32x4\t{$0x1, %1, %0|%0, %1, 0x1}"
>    [(set_attr "type" "sselog1")
>     (set_attr "prefix_extra" "1")
>     (set_attr "length_immediate" "1")
> -   (set_attr "isa" "*,avx512dq,avx512f")
> -   (set_attr "prefix" "vex,evex,evex")
> +   (set_attr "isa" "*,avx512vl")
> +   (set_attr "prefix" "vex,evex")
>     (set_attr "mode" "OI")])
>
>  (define_insn_and_split "vec_extract_lo_v64qi"
> @@ -12144,9 +12143,9 @@
>    "operands[1] = gen_lowpart (V16QImode, operands[1]);")
>
>  (define_insn "vec_extract_hi_v32qi"
> -  [(set (match_operand:V16QI 0 "nonimmediate_operand" "=xm,vm,vm")
> +  [(set (match_operand:V16QI 0 "nonimmediate_operand" "=xm,vm")
>         (vec_select:V16QI
> -         (match_operand:V32QI 1 "register_operand" "x,v,v")
> +         (match_operand:V32QI 1 "register_operand" "x,v")
>           (parallel [(const_int 16) (const_int 17)
>                      (const_int 18) (const_int 19)
>                      (const_int 20) (const_int 21)
> @@ -12158,13 +12157,12 @@
>    "TARGET_AVX"
>    "@
>     vextract%~128\t{$0x1, %1, %0|%0, %1, 0x1}
> -   vextracti32x4\t{$0x1, %1, %0|%0, %1, 0x1}
> -   vextracti32x4\t{$0x1, %g1, %0|%0, %g1, 0x1}"
> +   vextracti32x4\t{$0x1, %1, %0|%0, %1, 0x1}"
>    [(set_attr "type" "sselog1")
>     (set_attr "prefix_extra" "1")
>     (set_attr "length_immediate" "1")
> -   (set_attr "isa" "*,avx512dq,avx512f")
> -   (set_attr "prefix" "vex,evex,evex")
> +   (set_attr "isa" "*,avx512vl")
> +   (set_attr "prefix" "vex,evex")
>     (set_attr "mode" "OI")])
>
>  ;; NB: *vec_extract<mode>_0 must be placed before *vec_extracthf.
>


-- 
BR,
Hongtao

Re: [PATCH 2/2] x86: slightly correct / simplify *vec_extractv2ti

[Hongtao Liu]

On Wed, Jul 5, 2023 at 4:01 PM Jan Beulich via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> V2TImode values cannot appear in the upper 16 YMM registers without
> AVX512VL being enabled. Therefore forcing 512-bit mode (also not
> reflected in the "mode" attribute) is pointless.
Please set isa attribute for alternative 1 to avx512vl.
>
> gcc/
>
>         * config/i386/sse.md (*vec_extractv2ti): Drop g modifiers.
>
> --- a/gcc/config/i386/sse.md
> +++ b/gcc/config/i386/sse.md
> @@ -20115,7 +20115,7 @@
>    "TARGET_AVX"
>    "@
>     vextract%~128\t{%2, %1, %0|%0, %1, %2}
> -   vextracti32x4\t{%2, %g1, %0|%0, %g1, %2}"
> +   vextracti32x4\t{%2, %1, %0|%0, %1, %2}"
>    [(set_attr "type" "sselog")
>     (set_attr "prefix_extra" "1")
>     (set_attr "length_immediate" "1")
>


-- 
BR,
Hongtao

Re: [PATCH 1/2] x86: correct / simplify @vec_extract_hi_<mode> and vec_extract_hi_v32qi

[Jan Beulich]

On 05.07.2023 10:40, Hongtao Liu wrote:
> On Wed, Jul 5, 2023 at 4:00 PM Jan Beulich via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> The middle alternative each was unusable without enabling AVX512DQ (in
>> addition to AVX512VL), which is entirely unrelated here. The last
>> alternative is usable with AVX512VL only (due to type restrictions on
>> what may be put in the upper 16 YMM registers), and hence is pointlessly
>> forcing 512-bit mode (without actually reflecting that in the "mode"
>> attribute).
> Ok.

Thanks.

>> ---
>> Like elsewhere I suspect "prefix_extra" is bogus here and should be
>> dropped.
>>
>> Is "sselog1" actually appropriate here? Extracts are special forms of
>> moves after all, not logical operations. Even "sseshuf1" would seem to
>> come closer.
> Honestly, I don't know why it's marked as sselog1, but looking at the
> code,  almost all vec_extract patterns are marked as sselog1, guess
> it's originally from pextr.
> Agree that it's should be more close to shuffle instructions.

Yet as said I think these are special forms of moves. To me "shuffle"
involves more than one element. Yet then I don't really know what
the "type" attributes are used for (other than vaguely "for
scheduling"), and hence whether treating extracts as shuffles would
be more appropriate. (IOW I'd be happy to make a patch to convert all
extracts, but I'd need to know whether the conversion should be to
"sseshuf", "sseshuf1", or "ssemov". In the former two cases knowing
the "Why?" would also help, especially for writing a sensible
description. I also haven't found any explanation towards the
difference between sse<type> and sse<type>1: The "memory" attribute
evaluates to "both" for the 1 forms if operand 1 is in memory, yet
that doesn't seem to fit any of the uses here.)

Jan

Re: [PATCH 2/2] x86: slightly correct / simplify *vec_extractv2ti

[Jan Beulich]

On 05.07.2023 10:47, Hongtao Liu wrote:
> On Wed, Jul 5, 2023 at 4:01 PM Jan Beulich via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> V2TImode values cannot appear in the upper 16 YMM registers without
>> AVX512VL being enabled. Therefore forcing 512-bit mode (also not
>> reflected in the "mode" attribute) is pointless.
> Please set isa attribute for alternative 1 to avx512vl.

Since that looks redundant to me (as per the description), would you
mind explaining why that's necessary / wanted? It also feels orthogonal
to the change I'm making, as there was no "isa" attribute so far (which
would have wanted to be "avx512f" as per what you ask for, prior to the
change I'm making). Again me asking back is primarily to properly
describe the changes I'm making, of course along with me still needing
to properly understand when what attribute needs specifying explicitly.

Jan

Re: [PATCH 1/2] x86: correct / simplify @vec_extract_hi_<mode> and vec_extract_hi_v32qi

[Hongtao Liu]

On Wed, Jul 5, 2023 at 4:55 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 05.07.2023 10:40, Hongtao Liu wrote:
> > On Wed, Jul 5, 2023 at 4:00 PM Jan Beulich via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> The middle alternative each was unusable without enabling AVX512DQ (in
> >> addition to AVX512VL), which is entirely unrelated here. The last
> >> alternative is usable with AVX512VL only (due to type restrictions on
> >> what may be put in the upper 16 YMM registers), and hence is pointlessly
> >> forcing 512-bit mode (without actually reflecting that in the "mode"
> >> attribute).
> > Ok.
>
> Thanks.
>
> >> ---
> >> Like elsewhere I suspect "prefix_extra" is bogus here and should be
> >> dropped.
> >>
> >> Is "sselog1" actually appropriate here? Extracts are special forms of
> >> moves after all, not logical operations. Even "sseshuf1" would seem to
> >> come closer.
> > Honestly, I don't know why it's marked as sselog1, but looking at the
> > code,  almost all vec_extract patterns are marked as sselog1, guess
> > it's originally from pextr.
> > Agree that it's should be more close to shuffle instructions.
>
> Yet as said I think these are special forms of moves. To me "shuffle"
> involves more than one element. Yet then I don't really know what
I think if it only extracts from the low part, it's close to a move,
otherwise it's more like shuffle(shuffle the specific elements to the
low part).
I guess one possible reason it's marked as sselog1 is from port usage
perspective, it's more close to vector logic instructions?
> the "type" attributes are used for (other than vaguely "for
> scheduling"), and hence whether treating extracts as shuffles would
AFAI, it's only used by scheduling, I don't know if there're tools
based on GCC schedule model.
> be more appropriate. (IOW I'd be happy to make a patch to convert all
> extracts, but I'd need to know whether the conversion should be to
> "sseshuf", "sseshuf1", or "ssemov". In the former two cases knowing
> the "Why?" would also help, especially for writing a sensible
> description. I also haven't found any explanation towards the
> difference between sse<type> and sse<type>1: The "memory" attribute
> evaluates to "both" for the 1 forms if operand 1 is in memory, yet
> that doesn't seem to fit any of the uses here.)
I think sse<type>1 only has one input operand, but sse<type> may have
two or more.
For instruction perspective,  they're the same type, sse<type>1 is
introduced to avoid Segment Fault in define_memory_attr which will
check operands[2] or operands[3].
(Similar for other attribute default setting)
>
> Jan




--
BR,
Hongtao

Re: [PATCH 2/2] x86: slightly correct / simplify *vec_extractv2ti

[Hongtao Liu]

On Wed, Jul 5, 2023 at 5:03 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 05.07.2023 10:47, Hongtao Liu wrote:
> > On Wed, Jul 5, 2023 at 4:01 PM Jan Beulich via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> V2TImode values cannot appear in the upper 16 YMM registers without
> >> AVX512VL being enabled. Therefore forcing 512-bit mode (also not
> >> reflected in the "mode" attribute) is pointless.
> > Please set isa attribute for alternative 1 to avx512vl.
>
> Since that looks redundant to me (as per the description), would you
> mind explaining why that's necessary / wanted? It also feels orthogonal
> to the change I'm making, as there was no "isa" attribute so far (which
> would have wanted to be "avx512f" as per what you ask for, prior to the
> change I'm making). Again me asking back is primarily to properly
> describe the changes I'm making, of course along with me still needing
> to properly understand when what attribute needs specifying explicitly.
I checked ix86_hard_regno_ok, TImode/V2TImode will be allocated
with evex sse register only under TARGET_AVX512VL. otherwise
alternative 0 is matched.
So yes, no need to set isa attribute here, patch LGTM.
>
> Jan




--
BR,
Hongtao

Re: [PATCH 2/2] x86: slightly correct / simplify *vec_extractv2ti

[Hongtao Liu]

On Wed, Jul 5, 2023 at 6:22 PM Hongtao Liu <crazylht@gmail.com> wrote:
>
> On Wed, Jul 5, 2023 at 5:03 PM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 05.07.2023 10:47, Hongtao Liu wrote:
> > > On Wed, Jul 5, 2023 at 4:01 PM Jan Beulich via Gcc-patches
> > > <gcc-patches@gcc.gnu.org> wrote:
> > >>
> > >> V2TImode values cannot appear in the upper 16 YMM registers without
> > >> AVX512VL being enabled. Therefore forcing 512-bit mode (also not
> > >> reflected in the "mode" attribute) is pointless.
> > > Please set isa attribute for alternative 1 to avx512vl.
> >
> > Since that looks redundant to me (as per the description), would you
> > mind explaining why that's necessary / wanted? It also feels orthogonal
> > to the change I'm making, as there was no "isa" attribute so far (which
> > would have wanted to be "avx512f" as per what you ask for, prior to the
> > change I'm making). Again me asking back is primarily to properly
> > describe the changes I'm making, of course along with me still needing
> > to properly understand when what attribute needs specifying explicitly.
It's decided by many factors: instruction isa requirement, possible
register allocation for the alternative, also how
recog_memoized(constrain_operands) decide which_alternative.
For *vec_extractv2ti the alternative is implicitly guarded by
ix86_hard_regno_ok and no need for explicit isa attribute.
> I checked ix86_hard_regno_ok, TImode/V2TImode will be allocated
> with evex sse register only under TARGET_AVX512VL. otherwise
> alternative 0 is matched.
> So yes, no need to set isa attribute here, patch LGTM.
> >
> > Jan
>
>
>
>
> --
> BR,
> Hongtao



-- 
BR,
Hongtao