Jakub Jelinek <ja...@redhat.com> writes:
> On Fri, Apr 04, 2025 at 01:56:31PM +0100, Richard Sandiford wrote:
>> My suggestion isn't elegant, but: once we've decided not to delete
>> an instruction, we need to keep all definitions of all registers used
>> by the instruction (given the way that the algorithm works). So how
>> about instead just iterating over DF_INSN_USES in the else arm of:
>>
>> if (! live_insn && dbg_cnt (delete_trivial_dead))
>>
>> and incrementing counts for each register listed? That will
>> double-count in some cases, but only once we've decided to keep
>> (all definitions of) a register. And nothing should rely on the
>> final counts being zero.
>
> I think incrementing everything is unnecessary, there we could easily also
> overflow the counters quickly.
Is that likely though? We should only count each use twice in total,
once during the mark and once during the sweep. So we'd lose 1 bit
from the int. If that's a concern, we could switch to unsigned int
instead. That said...
> But just incrementing the self-references might be good enough.
>
> So what about the following patch then? Fixes the testcase too.
>
> Or shall we go with Eric's suggestion?
>
> And for both possibilities, without or with the extra
> else if (GET_CODE (PATTERN (x)) == PARALLEL) hunk from my previous version
> of the patch with the nsets stuff removed (i.e. ensure dest is pc_rtx
> whenever the insn is certainly never removable)?
>
> 2025-04-04 Jakub Jelinek <ja...@redhat.com>
>
> PR rtl-optimization/119594
> * cse.cc (count_reg_usage): If INCR is 0, increment by 1 counts
> of encountered REGs equal to dest rather than incrementing by INCR
> counts of REGs not equal to dest.
> (delete_trivially_dead_insns): For kept non-DEBUG instructions
> call count_reg_usage with INCR 0.
>
> * gcc.dg/pr119594.c: New test.
...this looks ok to me as well. The reason for suggesting DF_INSN_USES
was that I thought it might be faster, and since it would be ok in that
context to err on the side of counting, without the precision needed
by count_reg_usage.
On the PARALLEL thing: maybe at this stage it would be better to do the
minimum. Not a strong opinion though.
So OK from my POV, but please give others time to comment.
Thanks,
Richard
> --- gcc/cse.cc.jj 2025-04-03 13:17:56.506158871 +0200
> +++ gcc/cse.cc 2025-04-04 15:28:36.675315825 +0200
> @@ -6755,7 +6755,9 @@ cse_main (rtx_insn *f ATTRIBUTE_UNUSED,
>
> /* Count the number of times registers are used (not set) in X.
> COUNTS is an array in which we accumulate the count, INCR is how much
> - we count each register usage.
> + we count each register usage. INCR 0 has special meaning to increment
> + by 1 only registers which have not been incremented because they appear
> + in a SET_SRC of a SET which sets the same register.
>
> Don't count a usage of DEST, which is the SET_DEST of a SET which
> contains X in its SET_SRC. This is because such a SET does not
> @@ -6778,7 +6780,12 @@ count_reg_usage (rtx x, int *counts, rtx
> switch (code = GET_CODE (x))
> {
> case REG:
> - if (x != dest)
> + if (incr == 0)
> + {
> + if (x == dest)
> + ++counts[REGNO (x)];
> + }
> + else if (x != dest)
> counts[REGNO (x)] += incr;
> return;
>
> @@ -7143,6 +7150,11 @@ delete_trivially_dead_insns (rtx_insn *i
> later_debug_set_vars.safe_push (INSN_VAR_LOCATION_DECL (insn));
> TREE_VISITED (INSN_VAR_LOCATION_DECL (insn)) = 1;
> }
> + /* If we keep an instruction, make sure to increment even the
> + usages of regs in SET_SRC when SET_DEST is the same register.
> + See PR119594. */
> + if (!DEBUG_INSN_P (insn))
> + count_reg_usage (insn, counts, NULL_RTX, 0);
> }
> }
>
> --- gcc/testsuite/gcc.dg/pr119594.c.jj 2025-04-03 14:19:35.444575677
> +0200
> +++ gcc/testsuite/gcc.dg/pr119594.c 2025-04-03 14:19:13.962871101 +0200
> @@ -0,0 +1,32 @@
> +/* PR rtl-optimization/119594 */
> +/* { dg-do run } */
> +/* { dg-options "-Os -fno-dce -fno-tree-dce -fno-tree-dse" } */
> +
> +int a, b;
> +static unsigned c = -1;
> +
> +void
> +foo (int e)
> +{
> + a = a ^ e;
> +}
> +
> +void
> +bar (long e)
> +{
> + foo (e >> 1);
> +}
> +
> +int
> +main ()
> +{
> + int g[2];
> + for (int h = 0; h < 2; h++)
> + g[h] = -1;
> + for (; b; b++)
> + ;
> + g[1] = 0;
> + bar (c);
> + if (!a)
> + __builtin_abort ();
> +}
>
>
> Jakub
