On Mon, Jun 1, 2026 at 7:35 AM Dylan Rees <[email protected]> wrote:
>
> The fix to improve the folding of low-to-highpart builtins resulted
> in redundant duplication instructions. The compiler was placing the
> uniform vector arguments for low/highpart intrinsics in separate
> registers despite them duplicating the same scalar value. This turns
> out to be a result of the ordering of the duplication instructions,
> when a narrow duplication of a scalar value appears before a wider
> duplication of that same scalar the compiler is unable to link them
> together into a single operation.
>
> It is desirable in this case to create a single wider vector and
> take the lower half for narrower uses of the same vector, since we
> can then use a single load instruction. This patch fixes this logic
> within CSE by matching vector duplicates during 'cse_prescan', if
> they are duplicating the same pseudo into vectors of different
> widths but the same element size. In 'cse_main' these matches are
> processed and the wider vector duplicate insn is emitted before
> all matching duplications, narrower duplicate insns will
> be converted to a SUBREG of this initial duplication and duplicates
> of the same width will reuse it.
>
> Two test cases were updated to be more flexible with regexs since
> hardcoded values were causing unnecessary failures despite the
> code being logically correct.
>
> gcc/ChangeLog:
>
> * cse.cc (struct cse_vec_duplicate_match): New struct.
> (struct cse_basic_block_data): New member added.
> (cse_prescan_cache_vec_dup): New function to cache any vector
> duplications and check for potential matches.
> (cse_prescan_path): Extended pre_scan loop to cache vector
> duplication matches + New loop added to assign a list of
> matches to a block of data inside new struct member.
> (cse_main): New loop to emit wider vector duplication earlier
> and update narrower vector duplicate to SUBREG insn.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/aarch64/simd/fold_to_highpart_7.c:
> Extended test.
> * gcc.target/i386/pr81501-9a.c: Updated test.
> * gcc.target/i386/pr81501-9b.c: Updated test.
> ---
> gcc/cse.cc | 239 +++++++++++++++++-
> .../aarch64/simd/fold_to_highpart_7.c | 8 +-
> gcc/testsuite/gcc.target/i386/pr81501-9a.c | 4 +-
> gcc/testsuite/gcc.target/i386/pr81501-9b.c | 2 +-
> 4 files changed, 244 insertions(+), 9 deletions(-)
>
> diff --git a/gcc/cse.cc b/gcc/cse.cc
> index c1e0cc2167c..6c0e5d879bc 100644
> --- a/gcc/cse.cc
> +++ b/gcc/cse.cc
> @@ -483,6 +483,39 @@ struct branch_path
> basic_block bb;
> };
>
> +/* This data describes a pair of vec_duplicates in the same BB, which
> duplicate
> + the same pseudo to different vector lengths. The same structure is also
> + used while prescanning a basic block as a temporary cache entry. */
> +
> +struct cse_vec_duplicate_match
> +{
> + basic_block bb;
> + machine_mode widest_mode;
> + rtx scalar;
> + rtx_insn *first_insn;
> + rtx_insn *widest_insn;
> + auto_vec<rtx_insn *> related_dups;
> +
> + cse_vec_duplicate_match () = default;
Can this be declared as deleted instead? Or will that run into a vec issue?
> +
> + cse_vec_duplicate_match (basic_block bb_, machine_mode widest_mode_,
> + rtx scalar_, rtx_insn *first_insn_,
> + rtx_insn *widest_insn_)
> + : bb (bb_), widest_mode (widest_mode_), scalar (scalar_),
> + first_insn (first_insn_), widest_insn (widest_insn_)
> + {}
> +
> + cse_vec_duplicate_match (const cse_vec_duplicate_match &other)
> + : bb (other.bb), widest_mode (other.widest_mode), scalar (other.scalar),
> + first_insn (other.first_insn), widest_insn (other.widest_insn)
> + {
> + unsigned int i;
> + rtx_insn *insn;
> + FOR_EACH_VEC_ELT (other.related_dups, i, insn)
> + related_dups.safe_push (insn);
related_dups.reserve (other.related_dups.length ());
related_dups.slice (other.related_dups);
> + }
> +};
> +
> /* This data describes a block that will be processed by
> cse_extended_basic_block. */
>
> @@ -494,6 +527,10 @@ struct cse_basic_block_data
> int path_size;
> /* Current path, indicating which basic_blocks will be processed. */
> struct branch_path *path;
> + /* vec_duplicate sources seen in the current BB while prescanning. */
> + auto_vec<cse_vec_duplicate_match, 8> vec_duplicate_cache;
> + /* Syntactic vec_duplicate matches found in the same BB while prescanning.
> */
> + auto_vec<cse_vec_duplicate_match, 8> vec_duplicate_matches;
> };
>
>
> @@ -545,6 +582,8 @@ static rtx equiv_constant (rtx);
> static void record_jump_equiv (rtx_insn *, bool);
> static void record_jump_cond (enum rtx_code, machine_mode, rtx, rtx);
> static void cse_insn (rtx_insn *);
> +static void cse_prescan_cache_vec_dup (struct cse_basic_block_data *,
> + basic_block, rtx_insn *, rtx);
> static void cse_prescan_path (struct cse_basic_block_data *);
> static void invalidate_from_clobbers (rtx_insn *);
> static void invalidate_from_sets_and_clobbers (rtx_insn *);
> @@ -6527,9 +6566,63 @@ have_eh_succ_edges (basic_block bb)
> return false;
> }
>
> +/* Record vec_duplicate match for SET in the same basic block, if any. */
> +static void
> +cse_prescan_cache_vec_dup (struct cse_basic_block_data *data, basic_block bb,
> + rtx_insn *insn, rtx set)
> +{
> + rtx src = SET_SRC (set);
> + rtx dest = SET_DEST (set);
> + machine_mode mode = GET_MODE (src);
> + rtx scalar;
> +
> + /* Limit matching to duplicates of the same pseudo register, or an exact
> + SUBREG of such a pseudo. */
> + if (!vec_duplicate_p (src, &scalar)
> + || !(REG_P (scalar)
> + ? REGNO (scalar) >= FIRST_PSEUDO_REGISTER
> + : (GET_CODE (scalar) == SUBREG
> + && REG_P (SUBREG_REG (scalar))
> + && REGNO (SUBREG_REG (scalar)) >= FIRST_PSEUDO_REGISTER)))
> + return;
> +
> + /* Check for matching cached vec_duplicates and save the match if found.
> */
> + for (unsigned int i = 0; i < data->vec_duplicate_cache.length (); i++)
> + {
> + cse_vec_duplicate_match &entry = data->vec_duplicate_cache[i];
for (auto &entry : data->vec_duplicate_cache)
{
> +
> + /* Create a match with existing cache entry if both scalar register
> + pseudos are matching and the new vec_duplicate mode is wider. */
> + if (rtx_equal_p (entry.scalar, scalar)
> + && known_gt (GET_MODE_SIZE (mode), GET_MODE_SIZE
> (entry.widest_mode))
> + && GET_MODE_INNER (mode) == GET_MODE_INNER (entry.widest_mode)
> + && REG_P (dest))
Put the check for REG_P on dest first, it is cheapest one. Put the
check on the inner mode second for the same reason.
> + {
> + entry.widest_mode = mode;
> + entry.widest_insn = insn;
> + entry.related_dups.safe_push (insn);
> + return;
> + }
> + else if (rtx_equal_p (entry.scalar, scalar)
> + && known_le (GET_MODE_SIZE (mode),
> + GET_MODE_SIZE (entry.widest_mode))
> + && GET_MODE_INNER (mode) == GET_MODE_INNER (entry.widest_mode))
Put the check on the inner mode first.
> + {
> + entry.related_dups.safe_push (insn);
> + return;
> + }
> + }
> +
> + /* Cache vec_duplicate as a new entry if no match was found. */
> + cse_vec_duplicate_match new_entry (bb, mode, scalar, insn, NULL);
> + new_entry.related_dups.safe_push (insn);
> + data->vec_duplicate_cache.safe_push (new_entry);
> +}
> +
>
> /* Scan to the end of the path described by DATA. Return an estimate of
> - the total number of SETs of all insns in the path. */
> + the total number of SETs of all insns in the path. Also record any
> + matching vec_duplicate SETs of the same scalar value for different modes.
> */
>
> static void
> cse_prescan_path (struct cse_basic_block_data *data)
> @@ -6538,6 +6631,8 @@ cse_prescan_path (struct cse_basic_block_data *data)
> int path_size = data->path_size;
> int path_entry;
>
> + data->vec_duplicate_matches.truncate (0);
> +
> /* Scan to end of each basic block in the path. */
> for (path_entry = 0; path_entry < path_size; path_entry++)
> {
> @@ -6545,18 +6640,47 @@ cse_prescan_path (struct cse_basic_block_data *data)
> rtx_insn *insn;
>
> bb = data->path[path_entry].bb;
> + data->vec_duplicate_cache.truncate (0);
>
> FOR_BB_INSNS (bb, insn)
> {
> if (!INSN_P (insn))
> continue;
>
> + rtx pattern = PATTERN (insn);
> +
> /* A PARALLEL can have lots of SETs in it,
> especially if it is really an ASM_OPERANDS. */
> - if (GET_CODE (PATTERN (insn)) == PARALLEL)
> - nsets += XVECLEN (PATTERN (insn), 0);
> + if (GET_CODE (pattern) == PARALLEL)
> + {
> + int len = XVECLEN (pattern, 0);
> +
> + nsets += len;
> + for (int i = 0; i < len; i++)
> + {
> + rtx elt = XVECEXP (pattern, 0, i);
> + if (GET_CODE (elt) == SET)
> + cse_prescan_cache_vec_dup (data, bb, insn, elt);
> + }
> + }
> else
> - nsets += 1;
> + {
> + if (GET_CODE (pattern) == SET)
> + cse_prescan_cache_vec_dup (data, bb, insn, pattern);
> + nsets += 1;
> + }
> + }
> +
> + /* Record any vec_duplicate matches from this basic block. */
> + for (unsigned int i = 0; i < data->vec_duplicate_cache.length (); i++)
> + {
> + cse_vec_duplicate_match &entry = data->vec_duplicate_cache[i];
for (auto &entry : data->vec_duplicate_cache)
> +
> + /* If we recorded no wider vec_duplicate match then skip. */
> + if (entry.widest_insn == NULL)
NULL_RTX
> + continue;
> +
> + data->vec_duplicate_matches.safe_push (entry);
> }
> }
>
> @@ -6803,6 +6927,113 @@ cse_main (rtx_insn *f ATTRIBUTE_UNUSED, int nregs)
> if (ebb_data.nsets == 0)
> continue;
>
> + /* If prescan discovers any vec_duplicate pairs where a wider
> duplicate
> + appears after a narrow duplicate of the same scalar, move the
> + widest duplicate before the first vec_duplicate of this scalar and
> + rewrite all related duplicates to reuse it. */
> + for (unsigned int i = 0;
> + i < ebb_data.vec_duplicate_matches.length ();
> + i++)
> + {
> + cse_vec_duplicate_match &match =
> ebb_data.vec_duplicate_matches[i];
for (auto &match : ebb_data.vec_duplicate_matches)
> +
> + rtx wide_set = single_set (match.widest_insn);
> + if (!wide_set)
> + continue;
> + rtx wide_reg = SET_DEST (wide_set);
> + rtx_insn *insert_after = PREV_INSN (match.first_insn);
> + if (!REG_P (wide_reg) || !insert_after)
> + continue;
> +
> + /* Safety check that WIDE_REG is not otherwise used or redefined
> + before its original defining insn. */
> + if (reg_used_between_p (wide_reg, match.first_insn,
> + match.widest_insn)
> + || reg_set_between_p (wide_reg, match.first_insn,
> + match.widest_insn))
> + continue;
> +
> + /* Rewrite all related duplicates as a group so we either keep the
> + whole transformation or none of it. */
> + int prev_changes = num_changes_pending ();
> + bool abort_changes = false;
> + unsigned int j;
> + rtx_insn *dup_insn;
> +
> + FOR_EACH_VEC_ELT (match.related_dups, j, dup_insn)
> + {
for (rtx_insn *dup_insn : match.related_dups)
> + rtx dup_set;
> + rtx dup_src;
> + machine_mode dup_mode;
> +
> + if (dup_insn == match.widest_insn)
> + continue;
> +
> + /* The earlier safety check already covered the range from
> + FIRST_INSN up to WIDEST_INSN. Only check the remaining
> + suffix for duplicates that come later in the block. This
> + also covers call-clobbered hard registers via
> + reg_set_between_p. */
> + for (rtx_insn *scan = NEXT_INSN (match.widest_insn);
> + scan != NULL_RTX; scan = NEXT_INSN (scan))
> + if (scan == dup_insn)
> + {
> + if (reg_set_between_p (wide_reg,
> + match.widest_insn,
> + dup_insn))
> + continue;
> + break;
> + }
The above loop looks funny. Because it seems not to do anything except
call reg_set_between_p which I don't think has any side effects. Was
the innermost continue going to skip to the next match.related_dups
entry?
Also I suspect you should stop at the basic block boundary so you
don't continue all the way to the end of the function.
> +
> +
> + dup_set = single_set (dup_insn);
> + if (!dup_set)
> + {
> + abort_changes = true;
> + break;
> + }
> +
> + dup_mode = GET_MODE (SET_DEST (dup_set));
> + if (dup_mode == GET_MODE (wide_reg))
> + dup_src = copy_rtx (wide_reg);
Do you want copy_rtx or unshare_rtx here?
> + else
> + dup_src = gen_lowpart (dup_mode, wide_reg);
> +
> + if (!dup_src)
Seems like this can only happen with the lowpart, maybe put it under there.
> + {
> + abort_changes = true;
> + break;
> + }
> +
> + if (rtx_equal_p (dup_src, SET_DEST (dup_set)))
> + continue;
> +
> + if (!validate_change (dup_insn, &SET_SRC (dup_set), dup_src,
> 1))
> + {
> + abort_changes = true;
> + break;
> + }
> + }
> +
> + if (abort_changes)
> + {
> + cancel_changes (prev_changes);
> + continue;
> + }
> +
> + if (num_changes_pending () == prev_changes)
> + continue;
> +
> + if (!apply_change_group ())
> + continue;
> +
> + reorder_insns (match.widest_insn, match.widest_insn,
> insert_after);
> +
> + FOR_EACH_VEC_ELT (match.related_dups, j, dup_insn)
> + if (dup_insn != match.widest_insn)
> + df_insn_rescan (dup_insn);
if (match.related_dups.contains (match.widest_insn))
df_insn_rescan (match.widest_insn);
Note I didn't do a full algorithm review. But it does not seen too bad.
Thanks,
Andrea
> + }
> +
> /* Get a reasonable estimate for the maximum number of qty's
> needed for this path. For this, we take the number of sets
> and multiply that by MAX_RECOG_OPERANDS. */
> diff --git a/gcc/testsuite/gcc.target/aarch64/simd/fold_to_highpart_7.c
> b/gcc/testsuite/gcc.target/aarch64/simd/fold_to_highpart_7.c
> index 941dab3e9b9..ae7c368da8c 100644
> --- a/gcc/testsuite/gcc.target/aarch64/simd/fold_to_highpart_7.c
> +++ b/gcc/testsuite/gcc.target/aarch64/simd/fold_to_highpart_7.c
> @@ -4,7 +4,9 @@
> #include <arm_neon.h>
>
> /* We should fold to the highpart builtin when the multiplying the highpart
> of a
> - 128b vector with a uniform vector which can be widened.
> + 128b vector with a uniform vector. Also the uniform vector should be
> loaded
> + as a single wider vector duplicate load (ld1r) using the lower half for
> the
> + lowpart builtin and the full vector duplicate for the highpart builtin.
>
> Use vdup_n_u8 to create an 8x8 splat vector. */
>
> @@ -15,4 +17,6 @@ uint16x8_t foo(uint8_t *a, uint8_t *b) {
> }
>
> /* { dg-final { scan-assembler {mull2\t} } } */
> -/* { dg-final { scan-assembler-not {mull\t} } } */
> \ No newline at end of file
> +/* { dg-final { scan-assembler-not {mull\t} } } */
> +/* { dg-final { scan-assembler {\tld1r\t} } } */
> +/* { dg-final { scan-assembler-not {\tdup\tv} } } */
> diff --git a/gcc/testsuite/gcc.target/i386/pr81501-9a.c
> b/gcc/testsuite/gcc.target/i386/pr81501-9a.c
> index 66a2768a22c..ea7772dbb21 100644
> --- a/gcc/testsuite/gcc.target/i386/pr81501-9a.c
> +++ b/gcc/testsuite/gcc.target/i386/pr81501-9a.c
> @@ -7,10 +7,10 @@
> **foo:
> **.LFB[0-9]+:
> **...
> -** vpbroadcastb %edi, %zmm0
> -**...
> ** call __tls_get_addr@PLT
> **...
> +** vpbroadcastb %(edi|ebx), %zmm\d
> +**...
> */
>
> #include <immintrin.h>
> diff --git a/gcc/testsuite/gcc.target/i386/pr81501-9b.c
> b/gcc/testsuite/gcc.target/i386/pr81501-9b.c
> index 711b177bc1e..bd1b65d3e87 100644
> --- a/gcc/testsuite/gcc.target/i386/pr81501-9b.c
> +++ b/gcc/testsuite/gcc.target/i386/pr81501-9b.c
> @@ -8,7 +8,7 @@
> **foo:
> **.LFB[0-9]+:
> **...
> -** vpbroadcastb %edi, %zmm0
> +** vpbroadcastb %(edi|r\d+d), %zmm\d
> **...
> ** lea(l|q) var@TLSDESC\(%rip\), %(e|r)ax
> **...
> --
> 2.43.0
>
