On Tue, Jul 28, 2026 at 10:44 AM Hongyu Wang <[email protected]> wrote:
>
> Hi Uros,
>
> Thanks for noting the duplicated "preferred_for_speed" attr issue. Below
> patch is bootstrapped and regtested under x86_64-pc-linux-gnu{-m32,}.
>
> Ok for trunk?
>
> Several QImode integer patterns has two separate attr "preferred_for_speed"
> set, one gating the NDD memory-form alternatives with
> TARGET_ENABLE_NDD_MEM, and the existing one is for !TARGET_PARTIAL_REG_STALL.
> The later one will make genattrtab regenerate the cond based on
> !PARTIAL_REG_STALL only and override the TARGET_ENABLE_NDD_MEM one, so the NDD
> memory alternatives were always preferred for speed regardless of the
> X86_TUNE_ENABLE_NDD_MEM enabled or disabled.
>
> Combine both conditions into a single preferred_for_speed cond per
> pattern. For *ashlqi3_1 alternative 5 belongs to both sets, so it is
> gated on TARGET_ENABLE_NDD_MEM && !TARGET_PARTIAL_REG_STALL.
>
> gcc/ChangeLog:
>
> * config/i386/i386.md (*addqi_1<nf_name>): Merge the two
> preferred_for_speed attributes into one.
> (*andqi_1<nf_name>): Likewise.
> (*andqi_2_maybe_si): Likewise.
> (*<code>qi_1<nf_name>): Likewise.
> (*notxorqi_1): Likewise.
> (*one_cmplqi2_1): Likewise.
> (*ashlqi3_1<nf_name>): Likewise.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/i386/apx-ndd-mem-tune.c: New test.
OK with the new comments removed, as argued below.
BTW: As evident from the commit message, the last RTX will overwrite
previous attributes with the same name. The build should IMO error out
when two or more attributes with the same name are specified in the
attribute section of the insn. CC added.
Thanks,
Uros.
> ---
> gcc/config/i386/i386.md | 65 +++++++++----------
> .../gcc.target/i386/apx-ndd-mem-tune.c | 53 +++++++++++++++
> 2 files changed, 82 insertions(+), 36 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/i386/apx-ndd-mem-tune.c
>
> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index f762b2ccea2..dc27bcf7e8d 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -7139,10 +7139,6 @@ (define_insn "*addqi_1<nf_name>"
> }
> }
> [(set_attr "isa" "*,*,*,*,*,*,apx_ndd,apx_ndd,apx_ndd")
> - (set (attr "preferred_for_speed")
> - (cond [(eq_attr "alternative" "7,8")
> - (symbol_ref "TARGET_ENABLE_NDD_MEM")]
> - (symbol_ref "true")))
> (set (attr "type")
> (cond [(eq_attr "alternative" "5")
> (const_string "lea")
> @@ -7158,9 +7154,12 @@ (define_insn "*addqi_1<nf_name>"
> (set_attr "has_nf" "1")
> (set_attr "mode" "QI,QI,QI,SI,SI,SI,QI,QI,QI")
> ;; Potential partial reg stall on alternatives 3 and 4.
> + ;; NDD memory form (alternatives 7,8) is gated by TARGET_ENABLE_NDD_MEM.
There is no need for a comment describing *what* the code does. It is
evident from the code, so just remove this and other added comments.
> (set (attr "preferred_for_speed")
> (cond [(eq_attr "alternative" "3,4")
> - (symbol_ref "!TARGET_PARTIAL_REG_STALL")]
> + (symbol_ref "!TARGET_PARTIAL_REG_STALL")
> + (eq_attr "alternative" "7,8")
> + (symbol_ref "TARGET_ENABLE_NDD_MEM")]
> (symbol_ref "true")))])
>
> ;; Alternative 1 is needed to work around LRA limitation, see PR82524.
> @@ -13577,10 +13576,6 @@ (define_insn "*andqi_1<nf_name>"
> #"
> [(set_attr "type" "alu,alu,alu,alu,alu,alu,msklog")
> (set_attr "isa" "*,*,*,apx_ndd,apx_ndd,apx_ndd,*")
> - (set (attr "preferred_for_speed")
> - (cond [(eq_attr "alternative" "4,5")
> - (symbol_ref "TARGET_ENABLE_NDD_MEM")]
> - (symbol_ref "true")))
> (set_attr "has_nf" "1")
> (set (attr "mode")
> (cond [(eq_attr "alternative" "2")
> @@ -13591,9 +13586,12 @@ (define_insn "*andqi_1<nf_name>"
> ]
> (const_string "QI")))
> ;; Potential partial reg stall on alternative 2.
> + ;; NDD memory form (alternatives 4,5) is gated by TARGET_ENABLE_NDD_MEM.
> (set (attr "preferred_for_speed")
> (cond [(eq_attr "alternative" "2")
> - (symbol_ref "!TARGET_PARTIAL_REG_STALL")]
> + (symbol_ref "!TARGET_PARTIAL_REG_STALL")
> + (eq_attr "alternative" "4,5")
> + (symbol_ref "TARGET_ENABLE_NDD_MEM")]
> (symbol_ref "true")))])
>
> ;; Alternative 1 is needed to work around LRA limitation, see PR82524.
> @@ -13841,10 +13839,6 @@ (define_insn "*andqi_2_maybe_si"
> }
> [(set_attr "type" "alu")
> (set_attr "isa" "*,*,*,apx_ndd,apx_ndd,apx_ndd")
> - (set (attr "preferred_for_speed")
> - (cond [(eq_attr "alternative" "4,5")
> - (symbol_ref "TARGET_ENABLE_NDD_MEM")]
> - (symbol_ref "true")))
> (set (attr "mode")
> (cond [(eq_attr "alternative" "3,4,5")
> (const_string "QI")
> @@ -13857,9 +13851,12 @@ (define_insn "*andqi_2_maybe_si"
> ]
> (const_string "QI")))
> ;; Potential partial reg stall on alternative 2.
> + ;; NDD memory form (alternatives 4,5) is gated by TARGET_ENABLE_NDD_MEM.
> (set (attr "preferred_for_speed")
> (cond [(eq_attr "alternative" "2")
> - (symbol_ref "!TARGET_PARTIAL_REG_STALL")]
> + (symbol_ref "!TARGET_PARTIAL_REG_STALL")
> + (eq_attr "alternative" "4,5")
> + (symbol_ref "TARGET_ENABLE_NDD_MEM")]
> (symbol_ref "true")))])
>
> (define_insn "*and<mode>_2"
> @@ -14679,10 +14676,6 @@ (define_insn "*<code>qi_1<nf_name>"
> <nf_prefix><logic>{b}\t{%2, %1, %0|%0, %1, %2}
> #"
> [(set_attr "isa" "*,*,*,apx_ndd,apx_ndd,apx_ndd,avx512f")
> - (set (attr "preferred_for_speed")
> - (cond [(eq_attr "alternative" "4,5")
> - (symbol_ref "TARGET_ENABLE_NDD_MEM")]
> - (symbol_ref "true")))
> (set_attr "type" "alu,alu,alu,alu,alu,alu,msklog")
> (set_attr "has_nf" "1")
> (set (attr "mode")
> @@ -14694,9 +14687,12 @@ (define_insn "*<code>qi_1<nf_name>"
> ]
> (const_string "QI")))
> ;; Potential partial reg stall on alternative 2.
> + ;; NDD memory form (alternatives 4,5) is gated by TARGET_ENABLE_NDD_MEM.
> (set (attr "preferred_for_speed")
> (cond [(eq_attr "alternative" "2")
> - (symbol_ref "!TARGET_PARTIAL_REG_STALL")]
> + (symbol_ref "!TARGET_PARTIAL_REG_STALL")
> + (eq_attr "alternative" "4,5")
> + (symbol_ref "TARGET_ENABLE_NDD_MEM")]
> (symbol_ref "true")))])
>
> (define_insn_and_split "*notxorqi_1"
> @@ -14722,10 +14718,6 @@ (define_insn_and_split "*notxorqi_1"
> }
> }
> [(set_attr "isa" "*,*,*,apx_ndd,apx_ndd,apx_ndd,avx512f")
> - (set (attr "preferred_for_speed")
> - (cond [(eq_attr "alternative" "4,5")
> - (symbol_ref "TARGET_ENABLE_NDD_MEM")]
> - (symbol_ref "true")))
> (set_attr "type" "alu,alu,alu,alu,alu,alu,msklog")
> (set (attr "mode")
> (cond [(eq_attr "alternative" "2")
> @@ -14736,9 +14728,12 @@ (define_insn_and_split "*notxorqi_1"
> ]
> (const_string "QI")))
> ;; Potential partial reg stall on alternative 2.
> + ;; NDD memory form (alternatives 4,5) is gated by TARGET_ENABLE_NDD_MEM.
> (set (attr "preferred_for_speed")
> (cond [(eq_attr "alternative" "2")
> - (symbol_ref "!TARGET_PARTIAL_REG_STALL")]
> + (symbol_ref "!TARGET_PARTIAL_REG_STALL")
> + (eq_attr "alternative" "4,5")
> + (symbol_ref "TARGET_ENABLE_NDD_MEM")]
> (symbol_ref "true")))])
>
> ;; convert (sign_extend:WIDE (any_logic:NARROW (memory, immediate)))
> @@ -16047,10 +16042,6 @@ (define_insn "*one_cmplqi2_1"
> not{b}\t{%1, %0|%0, %1}
> #"
> [(set_attr "isa" "*,*,apx_ndd,apx_ndd,avx512f")
> - (set (attr "preferred_for_speed")
> - (cond [(eq_attr "alternative" "3")
> - (symbol_ref "TARGET_ENABLE_NDD_MEM")]
> - (symbol_ref "true")))
> (set_attr "type" "negnot,negnot,negnot,negnot,msklog")
> (set (attr "mode")
> (cond [(eq_attr "alternative" "1")
> @@ -16061,9 +16052,12 @@ (define_insn "*one_cmplqi2_1"
> ]
> (const_string "QI")))
> ;; Potential partial reg stall on alternative 1.
> + ;; NDD memory form (alternative 3) is gated by TARGET_ENABLE_NDD_MEM.
> (set (attr "preferred_for_speed")
> (cond [(eq_attr "alternative" "1")
> - (symbol_ref "!TARGET_PARTIAL_REG_STALL")]
> + (symbol_ref "!TARGET_PARTIAL_REG_STALL")
> + (eq_attr "alternative" "3")
> + (symbol_ref "TARGET_ENABLE_NDD_MEM")]
> (symbol_ref "true")))])
>
> ;; Alternative 1 is needed to work around LRA limitation, see PR82524.
> @@ -17214,10 +17208,6 @@ (define_insn "*ashlqi3_1<nf_name>"
> }
> }
> [(set_attr "isa" "*,*,*,avx512dq,apx_ndd,apx_ndd")
> - (set (attr "preferred_for_speed")
> - (cond [(eq_attr "alternative" "5")
> - (symbol_ref "TARGET_ENABLE_NDD_MEM")]
> - (symbol_ref "true")))
> (set (attr "type")
> (cond [(eq_attr "alternative" "2")
> (const_string "lea")
> @@ -17240,9 +17230,12 @@ (define_insn "*ashlqi3_1<nf_name>"
> (const_string "*")))
> (set_attr "has_nf" "1")
> (set_attr "mode" "QI,SI,SI,QI,QI,QI")
> - ;; Potential partial reg stall on alternative 1.
> + ;; NDD memory form (alternative 5) is gated by TARGET_ENABLE_NDD_MEM.
> + ;; Potential partial reg stall on alternatives 1, 4 and 5.
> (set (attr "preferred_for_speed")
> - (cond [(eq_attr "alternative" "1,4,5")
> + (cond [(eq_attr "alternative" "5")
> + (symbol_ref "TARGET_ENABLE_NDD_MEM &&
> !TARGET_PARTIAL_REG_STALL")
> + (eq_attr "alternative" "1,4")
> (symbol_ref "!TARGET_PARTIAL_REG_STALL")]
> (symbol_ref "true")))])
>
> diff --git a/gcc/testsuite/gcc.target/i386/apx-ndd-mem-tune.c
> b/gcc/testsuite/gcc.target/i386/apx-ndd-mem-tune.c
> new file mode 100644
> index 00000000000..5726390be89
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/apx-ndd-mem-tune.c
> @@ -0,0 +1,53 @@
> +/* Verify that the memory form of NDD instructions is guarded by the
> + X86_TUNE_ENABLE_NDD_MEM tune properly. The duplicated preferred_for_speed
> + attr will be true when partial_reg_stall enabled for qi patterns, and
> + override the ENABLE_NDD_MEM set at first, so there should be only one attr
> + remaining by combining the two conditions together. */
> +/* { dg-do compile { target { ! ia32 } } } */
> +/* { dg-options "-mapxf -march=x86-64 -O2 -mtune-ctrl=partial_reg_stall" } */
> +
> +unsigned char gc;
> +
> +unsigned char
> +add_qi (unsigned char *p, unsigned char b)
> +{
> + unsigned char r = *p + b;
> + gc = r;
> + return r;
> +}
> +
> +unsigned char
> +and_qi (unsigned char *p, unsigned char b)
> +{
> + unsigned char r = *p & b;
> + gc = r;
> + return r;
> +}
> +
> +unsigned char
> +xor_qi (unsigned char *p, unsigned char b)
> +{
> + unsigned char r = *p ^ b;
> + gc = r;
> + return r;
> +}
> +
> +unsigned char
> +not_qi (unsigned char *p)
> +{
> + unsigned char r = ~*p;
> + gc = r;
> + return r;
> +}
> +
> +unsigned char
> +shl_qi (unsigned char *p, unsigned char c)
> +{
> + unsigned char r = (unsigned char) (*p << c);
> + gc = r;
> + return r;
> +}
> +
> +/* None of the QImode ops above should use the NDD memory-source form
> + when partial_reg_stall && !enable_ndd_mem. */
> +/* { dg-final { scan-assembler-not {b[ \t]+%[a-z0-9]+, \(%[a-z0-9]+\),} } }
> */
> --
> 2.31.1
>
