[v2] x86/{,V}AES: adjust when to force EVEX encoding

  Commit a79d13a01f8c ("i386: Fix aes/vaes patterns [PR114576]") correctly
said "..., but we need to emit {evex} prefix in the assembly if AES ISA
is not enabled". Yet it did so only for the TARGET_AES insns. Going from
the alternative chosen in the TARGET_VAES insns isn't quite right: If
AES is (also) enabled, EVEX encoding would needlessly be forced.

gcc/

	* config/i386/sse.md (vaesdec_<mode>, vaesdeclast_<mode>,
	vaesenc_<mode>, vaesenclast_<mode>): Replace which_alternative
	check by TARGET_AES one.
---
As an aside - {evex} (and other) pseudo-prefixes would better be avoided
anyway whenever possible, as those are getting in the way of code
putting in place macro overrides for certain insns: gas 2.43 rejects
such bogus placement of pseudo-prefixes.

Is it, btw, correct that none of these insns have a "prefix" attribute?
---
v2: Adjust (shrink) description.
  

On Mon, Sep 30, 2024 at 3:33 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> Commit a79d13a01f8c ("i386: Fix aes/vaes patterns [PR114576]") correctly
> said "..., but we need to emit {evex} prefix in the assembly if AES ISA
> is not enabled". Yet it did so only for the TARGET_AES insns. Going from
> the alternative chosen in the TARGET_VAES insns isn't quite right: If
> AES is (also) enabled, EVEX encoding would needlessly be forced.
>
> gcc/
>
>         * config/i386/sse.md (vaesdec_<mode>, vaesdeclast_<mode>,
>         vaesenc_<mode>, vaesenclast_<mode>): Replace which_alternative
>         check by TARGET_AES one.
> ---
> As an aside - {evex} (and other) pseudo-prefixes would better be avoided
> anyway whenever possible, as those are getting in the way of code
> putting in place macro overrides for certain insns: gas 2.43 rejects
> such bogus placement of pseudo-prefixes.
>
> Is it, btw, correct that none of these insns have a "prefix" attribute?
There's some automatic in i386.md to determine the prefix, rough, not
very accurate.

  688;; Prefix used: original, VEX or maybe VEX.
  689(define_attr "prefix" "orig,vex,maybe_vex,evex,maybe_evex"
  690  (cond [(eq_attr "mode" "OI,V8SF,V4DF")
  691           (const_string "vex")
  692         (eq_attr "mode" "XI,V16SF,V8DF")
  693           (const_string "evex")
  694         (eq_attr "type" "ssemuladd")
  695           (if_then_else (eq_attr "isa" "fma4")
  696             (const_string "vex")
  697             (const_string "maybe_evex"))
  698         (eq_attr "type" "sse4arg")
  699           (const_string "vex")
  700        ]
  701        (const_string "orig")))
  702

> ---
> v2: Adjust (shrink) description.
Ok for the patch.
>
> --- a/gcc/config/i386/sse.md
> +++ b/gcc/config/i386/sse.md
> @@ -30802,7 +30802,7 @@
>           UNSPEC_VAESDEC))]
>    "TARGET_VAES"
>  {
> -  if (which_alternative == 0 && <MODE>mode == V16QImode)
> +  if (!TARGET_AES && <MODE>mode == V16QImode)
>      return "%{evex%} vaesdec\t{%2, %1, %0|%0, %1, %2}";
>    else
>      return "vaesdec\t{%2, %1, %0|%0, %1, %2}";
> @@ -30816,7 +30816,7 @@
>           UNSPEC_VAESDECLAST))]
>    "TARGET_VAES"
>  {
> -  if (which_alternative == 0 && <MODE>mode == V16QImode)
> +  if (!TARGET_AES && <MODE>mode == V16QImode)
>      return "%{evex%} vaesdeclast\t{%2, %1, %0|%0, %1, %2}";
>    else
>      return "vaesdeclast\t{%2, %1, %0|%0, %1, %2}";
> @@ -30830,7 +30830,7 @@
>           UNSPEC_VAESENC))]
>    "TARGET_VAES"
>  {
> -  if (which_alternative == 0 && <MODE>mode == V16QImode)
> +  if (!TARGET_AES && <MODE>mode == V16QImode)
>      return "%{evex%} vaesenc\t{%2, %1, %0|%0, %1, %2}";
>    else
>      return "vaesenc\t{%2, %1, %0|%0, %1, %2}";
> @@ -30844,7 +30844,7 @@
>           UNSPEC_VAESENCLAST))]
>    "TARGET_VAES"
>  {
> -  if (which_alternative == 0 && <MODE>mode == V16QImode)
> +  if (!TARGET_AES && <MODE>mode == V16QImode)
>      return "%{evex%} vaesenclast\t{%2, %1, %0|%0, %1, %2}";
>    else
>      return "vaesenclast\t{%2, %1, %0|%0, %1, %2}";
  

On 08.10.2024 08:54, Hongtao Liu wrote:
> On Mon, Sep 30, 2024 at 3:33 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> Commit a79d13a01f8c ("i386: Fix aes/vaes patterns [PR114576]") correctly
>> said "..., but we need to emit {evex} prefix in the assembly if AES ISA
>> is not enabled". Yet it did so only for the TARGET_AES insns. Going from
>> the alternative chosen in the TARGET_VAES insns isn't quite right: If
>> AES is (also) enabled, EVEX encoding would needlessly be forced.
>>
>> gcc/
>>
>>         * config/i386/sse.md (vaesdec_<mode>, vaesdeclast_<mode>,
>>         vaesenc_<mode>, vaesenclast_<mode>): Replace which_alternative
>>         check by TARGET_AES one.
>> ---
>> As an aside - {evex} (and other) pseudo-prefixes would better be avoided
>> anyway whenever possible, as those are getting in the way of code
>> putting in place macro overrides for certain insns: gas 2.43 rejects
>> such bogus placement of pseudo-prefixes.
>>
>> Is it, btw, correct that none of these insns have a "prefix" attribute?
> There's some automatic in i386.md to determine the prefix, rough, not
> very accurate.
> 
>   688;; Prefix used: original, VEX or maybe VEX.
>   689(define_attr "prefix" "orig,vex,maybe_vex,evex,maybe_evex"
>   690  (cond [(eq_attr "mode" "OI,V8SF,V4DF")
>   691           (const_string "vex")
>   692         (eq_attr "mode" "XI,V16SF,V8DF")
>   693           (const_string "evex")
>   694         (eq_attr "type" "ssemuladd")
>   695           (if_then_else (eq_attr "isa" "fma4")
>   696             (const_string "vex")
>   697             (const_string "maybe_evex"))
>   698         (eq_attr "type" "sse4arg")
>   699           (const_string "vex")
>   700        ]
>   701        (const_string "orig")))
>   702

I'm aware, and I raised the question because it seemed pretty clear to
me that it wouldn't get things right here.

>> ---
>> v2: Adjust (shrink) description.
> Ok for the patch.

Thanks. What about the 14.x branch?

Jan
  

On Tue, Oct 8, 2024 at 3:00 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.10.2024 08:54, Hongtao Liu wrote:
> > On Mon, Sep 30, 2024 at 3:33 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> Commit a79d13a01f8c ("i386: Fix aes/vaes patterns [PR114576]") correctly
> >> said "..., but we need to emit {evex} prefix in the assembly if AES ISA
> >> is not enabled". Yet it did so only for the TARGET_AES insns. Going from
> >> the alternative chosen in the TARGET_VAES insns isn't quite right: If
> >> AES is (also) enabled, EVEX encoding would needlessly be forced.
> >>
> >> gcc/
> >>
> >>         * config/i386/sse.md (vaesdec_<mode>, vaesdeclast_<mode>,
> >>         vaesenc_<mode>, vaesenclast_<mode>): Replace which_alternative
> >>         check by TARGET_AES one.
> >> ---
> >> As an aside - {evex} (and other) pseudo-prefixes would better be avoided
> >> anyway whenever possible, as those are getting in the way of code
> >> putting in place macro overrides for certain insns: gas 2.43 rejects
> >> such bogus placement of pseudo-prefixes.
> >>
> >> Is it, btw, correct that none of these insns have a "prefix" attribute?
> > There's some automatic in i386.md to determine the prefix, rough, not
> > very accurate.
> >
> >   688;; Prefix used: original, VEX or maybe VEX.
> >   689(define_attr "prefix" "orig,vex,maybe_vex,evex,maybe_evex"
> >   690  (cond [(eq_attr "mode" "OI,V8SF,V4DF")
> >   691           (const_string "vex")
> >   692         (eq_attr "mode" "XI,V16SF,V8DF")
> >   693           (const_string "evex")
> >   694         (eq_attr "type" "ssemuladd")
> >   695           (if_then_else (eq_attr "isa" "fma4")
> >   696             (const_string "vex")
> >   697             (const_string "maybe_evex"))
> >   698         (eq_attr "type" "sse4arg")
> >   699           (const_string "vex")
> >   700        ]
> >   701        (const_string "orig")))
> >   702
>
> I'm aware, and I raised the question because it seemed pretty clear to
> me that it wouldn't get things right here.
AFAIK It's mainly for attr length to determine the codesize of
instructions( ix86_min_insn_size), and the rough model should be
sufficient for most cases.
>
> >> ---
> >> v2: Adjust (shrink) description.
> > Ok for the patch.
>
> Thanks. What about the 14.x branch?
Also Ok.
>
> Jan