diff mbox series

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

Message ID c3e17c19-0cd4-9d07-bcfc-0312487f029a@suse.com
State Accepted
Headers
Series x86: vec_extract_* adjustments |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Jan Beulich July 5, 2023, 8 a.m. UTC 
  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.

Comments

Hongtao Liu July 5, 2023, 8:47 a.m. UTC | #1 
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")
>
Jan Beulich July 5, 2023, 9:03 a.m. UTC | #2 
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
Hongtao Liu July 5, 2023, 10:22 a.m. UTC | #3 
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
Hongtao Liu July 5, 2023, 10:32 a.m. UTC | #4 
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
diff mbox series

Patch

--- 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")