[Bug c/101188] New: [AVR] Miscompilation and function pointers

[Bug c/101188] New: [AVR] Miscompilation and function pointers

[joel.bertrand at systella dot fr]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

            Bug ID: 101188
           Summary: [AVR] Miscompilation and function pointers
           Product: gcc
           Version: 11.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: joel.bertrand at systella dot fr
  Target Milestone: ---

Hello,

I'm writing a firmware that runs on an ATmega1284. To build this bare metal
firmware, I have used avr-gcc 5.4.0 and I have seen a strange pointer
corruption. Thus, I have built from scratch a new gcc (11.1.0) that triggers
the same bug.

In a first time, my code is downloadable at
https://hilbert.systella.fr/public/firmware.tar.gz

I have tried to make a simple testcase, but when I remove some unused code,
sometimes, firmware runs as expected. That being said, a lot of code is not
executed. In main.c, I only initialize my board and I only try to send LoRaWAN
packets.

In lorawan/ldl_mac.c, you will find the following code :

#if defined(LDL_FIX_GCC_BUG)
            void app_handler(void *app, enum ldl_mac_response_type type,
                    const union ldl_mac_response_arg *arg);
            if (self->handler != app_handler) for(;;);
#endif

            self->handler(self->app, LDL_MAC_DEV_NONCE_UPDATED, &arg);

If I undefined LDL_FIX_GCC_BUG, firmware crashes as self->handler seems to be
nullified. With LDL_FIX_GCC_BUG, firmware never stalls in for(;;) and runs as
expected. Of course, self->handler should be equal to app_handler.

If I replace self->handler(self->app, LDL_MAC_DEV_NONCE_UPDATED, &arg) by
app_handler(self->app, LDL_MAC_DEV_NONCE_UPDATED, &arg), even if I delete code
between #if and #endif, firmware runs also as expected.

Thus, I have checked assembly output.

good_2.S : (https://hilbert.systella.fr/public/good_2.S)

    a318: f6 01       movw r30, r12
...
    a33c: e3 50       subi r30, 0x03 ; 3
    a33e: ff 4f       sbci r31, 0xFF ; 255
    a340: 01 90       ld r0, Z+
    a342: f0 81       ld r31, Z
    a344: e0 2d       mov r30, r0

This loads Z with R13:R12, and then later offsets it by -0xFF03 to
obtain the address of self->handler, which is then used by the icall
instruction.

bad.S : (https://hilbert.systella.fr/public/bad.S)


    a31c: f6 01       movw r30, r12
...
    a340: 68 01       movw r12, r16
    a342: ff e4       ldi r31, 0x4F ; 79
    a344: cf 1a       sub r12, r31
    a346: fe ef       ldi r31, 0xFE ; 254
    a348: df 0a       sbc r13, r31
    a34a: e3 50       subi r30, 0x03 ; 3
    a34c: ff 4f       sbci r31, 0xFF ; 255
    a34e: 01 90       ld r0, Z+
    a350: f0 81       ld r31, Z
    a352: e0 2d       mov r30, r0

Note the clobbering of R31:R30 with immediate values *before* the offsetting is
done. I think this is a codegen bug - the compiler should have either picked a
different set of regs to subtract R13:R12 from, or should have restored Z with
a movw r30, r12 before 0xa34a.

Regards,

JN

[Bug target/101188] [AVR] Miscompilation and function pointers

[saaadhu at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

Senthil Kumar Selvaraj <saaadhu at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
                 CC|                            |saaadhu at gcc dot gnu.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-06-25

--- Comment #1 from Senthil Kumar Selvaraj <saaadhu at gcc dot gnu.org> ---
Confirmed with 12.0.0 20210625

Here's a reduced testcase that hangs indefinitely with avrtest - the log shows
call to address 0.

$ cat fail.c
#include <stdint.h>
#include <stdlib.h>

typedef uint8_t (*fn1)(void *a);
typedef void (*fn2)(void *a, const uint32_t *arg);

struct S {
    uint8_t buffer[64];
    uint16_t n;
    fn2 f2;
    void *a;
    fn1 f1;
};

volatile uint16_t x;
void __attribute__((noinline))
foo(uint16_t n)
{
  x = n;
}

void __attribute__((noinline)) testfn(struct S *self)
{
    uint32_t arg;

    foo(self->n);
    self->n++;
    self->f2(self->a, &arg);
    self->buffer[0] = self->f1(self->a);
}

static unsigned char myfn2_called = 0;
static void
myfn2(void *a, const uint32_t * arg)
{
  myfn2_called = 1;  
}

static uint8_t
myfn1(void *a)
{ }

int main() {
  struct S s;
  s.n = 0; s.f2 = myfn2; s.f1 = myfn1;
  testfn(&s);
  if (myfn2_called != 1)
    abort();
  return 0;
}

$ avr-gcc -mmcu=atmega128 fail.c -O2 ~/code/avrtest/exit-atmega128.o --version
avr-gcc (GCC) 12.0.0 20210625 (experimental)
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ ~/code/avrtest/avrtest -mmcu=avr51 a.out 
^C

The below code is at fault - there's an ldi to r31, followed by a load to Z
using R31:R30, and then an icall. The clobbering of r31 at 0x138 causes junk
values (0) to be read into Z, and icall calls address 0. 

 138:   f4 e4           ldi     r31, 0x44       ; 68
 13a:   ef 0e           add     r14, r31
 13c:   f1 1c           adc     r15, r1
 13e:   32 96           adiw    r30, 0x02       ; 2
 140:   01 90           ld      r0, Z+
 142:   f0 81           ld      r31, Z
 144:   e0 2d           mov     r30, r0
 146:   be 01           movw    r22, r28
 148:   6f 5f           subi    r22, 0xFF       ; 255
 14a:   7f 4f           sbci    r23, 0xFF       ; 255
 14c:   d7 01           movw    r26, r14
 14e:   8d 91           ld      r24, X+
 150:   9c 91           ld      r25, X
 152:   09 95           icall

[Bug target/101188] [AVR] Miscompilation and function pointers

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |gjl at gcc dot gnu.org

--- Comment #2 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
Created attachment 55147
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55147&action=edit
C test case (compile only)

Same test case like from comment #1, but only with the affected function.

$ avr-gcc fail1.c -save-temps -dp -dumpbase "" -mmcu=atmega128 -c -Os -da

For all version that I tested (v5.4, v8.5, v11.3, v12.3, v13.1, master 14.0.0
20230523) I am getting wrong code.

[Bug rtl-optimization/101188] [AVR] Miscompilation and function pointers

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|2021-06-25 00:00:00         |2023-5-24
          Component|target                      |rtl-optimization
                 CC|                            |vmakarov at gcc dot gnu.org
           Keywords|                            |ra

--- Comment #3 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
With the test case from comment #2

$ avr-gcc fail1.c -save-temps -dp -dumpbase "" -mmcu=atmega128 -c -Os -da

typedef __UINT8_TYPE__ uint8_t;
typedef __UINT16_TYPE__ uint16_t;
typedef __UINT32_TYPE__ uint32_t;

typedef uint8_t (*fn1)(void *a);
typedef void (*fn2)(void *a, const uint32_t *arg);

struct S
{
    uint8_t buffer[64];
    uint16_t n;
    fn2 f2;
    void *a;
    fn1 f1;
};

volatile uint16_t x;

void foo (uint16_t);

void __attribute__((noinline)) testfn (struct S *self)
{
    uint32_t arg;

    foo (self->n);
    self->n++;
    self->f2 (self->a, &arg);
    self->buffer[0] = self->f1 (self->a);
}

For all what I can tell, this looks like a bug in the register allocator, not a
backend issue. Allow me to CC Vladimir.

With current master 14.0.0 20230523 the .postreload dump shows that insn 14
uses HI:30, and insn 17 continues using HI:30.  But for insn 15, reload
provides scratch QI:31 which shreds the value in HI:30:


(insn 14 13 44 2 (set (mem:HI (reg:HI 30 r30) [1 self_10(D)->n+0 S2 A8])
        (reg:HI 24 r24 [56])) "fail1.c":28:12 101 {*movhi_split}
     (nil))
(insn 44 14 15 2 (set (reg/f:HI 14 r14 [58])
        (reg/v/f:HI 16 r16 [orig:51 self ] [51])) "fail1.c":29:5 101
{*movhi_split}
     (nil))
(insn 15 44 16 2 (parallel [
            (set (reg/f:HI 14 r14 [58])
                (plus:HI (reg/f:HI 14 r14 [58])
                    (const_int 68 [0x44])))
            (clobber (reg:QI 31 r31))
        ]) "fail1.c":29:5 175 {addhi3_clobber}
     (nil))
(note 16 15 17 2 NOTE_INSN_DELETED)
>>> Where is insn 45 ???
(insn 17 16 18 2 (set (reg/f:HI 30 r30 [60])
        (plus:HI (reg/f:HI 30 r30 [60])
            (const_int 2 [0x2]))) "fail1.c":29:9 165 {*addhi3_split}
     (nil))


In the .reload dump however, there is a move insn 45 that restores HI:30 after
the addhi3_clobber:

...
(note 16 15 45 2 NOTE_INSN_DELETED)
(insn 45 16 17 2 (set (reg/f:HI 30 r30 [60])
        (reg/v/f:HI 16 r16 [orig:51 self ] [51])) "fail1.c":28:9 101
{*movhi_split}
     (nil))
(insn 17 45 18 2 (parallel [
            (set (reg/f:HI 30 r30 [60])
                (plus:HI (reg/f:HI 30 r30 [60])
                    (const_int 66 [0x42])))
            (clobber (scratch:QI))
        ]) "fail1.c":28:9 175 {addhi3_clobber}
     (nil))

[Bug rtl-optimization/101188] [AVR] Miscompilation and function pointers

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |uweigand at de dot ibm.com

--- Comment #4 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
Allow me to CC reload maintainer.

[Bug rtl-optimization/101188] [AVR] Miscompilation and function pointers

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

--- Comment #5 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
It happens in postreload.cc::reload_cse_move2add() when


(insn 45 16 17 2 (set (reg/f:HI 30 r30 [60])
        (reg/v/f:HI 16 r16 [orig:51 self ] [51])) "fail1.c":29:9 101
{*movhi_split} (nil))
(insn 17 45 18 2 (parallel [
            (set (reg/f:HI 30 r30 [60])
                (plus:HI (reg/f:HI 30 r30 [60])
                    (const_int 66 [0x42])))
            (clobber (scratch:QI))
        ]) "fail1.c":29:9 175 {addhi3_clobber} (nil))

is transformed to:

(insn 17 16 18 2 (set (reg/f:HI 30 r30 [60])
        (plus:HI (reg/f:HI 30 r30 [60])
            (const_int 2 [0x2]))) "fail1.c":29:9 165 {*addhi3_split} (nil))

The wrong setting of "success" is in postreload.cc:2028 as of the following, so
the condition that leads to there is bogus.

https://gcc.gnu.org/git/?p=gcc.git;a=blame;f=gcc/postreload.cc;h=fb392651e1b6a60e12bf3d36bc302bf9be8bc608;hb=03c7c418baa01f0642817bc9b44192d134102aa9#l2028

[Bug rtl-optimization/101188] [AVR] Miscompilation and function pointers

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

--- Comment #6 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
Created attachment 55152
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55152&action=edit
diff testcase by v4.9.2 vs v5.2.1

Code from v4.9.2 is correct, but from v5.2.1 is bogus:

--- fail1-4.9.2.sx      2023-05-24 17:20:46.508698338 +0200
+++ fail1-5.2.1.sx      2023-05-24 17:19:50.019976879 +0200
@@ -39,11 +39,11 @@
        adiw r24,1       ;  13  addhi3_clobber/1        [length = 1]
        std Z+1,r25      ;  14  *movhi/4        [length = 2]
        st Z,r24
-       adiw r30,2       ;  15  *addhi3/3       [length = 1]
-       movw r14,r16     ;  39  *movhi/1        [length = 1]
-       ldi r24,68       ;  16  addhi3_clobber/3        [length = 3]
-       add r14,r24
+       movw r14,r16     ;  38  *movhi/1        [length = 1]
+       ldi r31,68       ;  15  addhi3_clobber/3        [length = 3]
+       add r14,r31
        adc r15,__zero_reg__
+       adiw r30,2       ;  17  *addhi3/3       [length = 1]
        ld __tmp_reg__,Z+        ;  18  *movhi/3        [length = 3]
        ld r31,Z
        mov r30,__tmp_reg__

[Bug rtl-optimization/101188] [AVR] Miscompilation and function pointers

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

--- Comment #7 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
On branch releases/gcc-5.4.0, the wrong code is since r226119, which is a
backport of r225918.

Doesn't look like this caused the current PR; maybe that change just unmasked a
bug in postreload that's even older.

[Bug rtl-optimization/101188] [postreload] Uses content of a clobbered register

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[AVR] Miscompilation and    |[postreload] Uses content
                   |function pointers           |of a clobbered register
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=56833

--- Comment #8 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
Changing the title to something that resembles what is going wrong.

Also there is PR56833 which was fixed around v4.9, so maybe that fix was
incomplete.  There is also PR56442 which is still open, and where it's unclear
whether that is a duplicate.

[Bug rtl-optimization/101188] [postreload] Uses content of a clobbered register

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

--- Comment #9 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
The bug works as follows:

postreload.cc::reload_cse_move2add() loops over all insns, and at some point it
encounters

(insn 44 14 15 2 (set (reg/f:HI 14 r14 [58])
        (reg/v/f:HI 16 r16 [orig:51 self ] [51])) "fail1.c":28:5 101
{*movhi_split}
     (nil))

During the analysis for that insn, it executes

          rtx_insn *next = next_nonnote_nondebug_insn (insn);
          rtx set = NULL_RTX;
          if (next)
            set = single_set (next);

where next is

(insn 15 44 16 2 (parallel [
            (set (reg/f:HI 14 r14 [58])
                (plus:HI (reg/f:HI 14 r14 [58])
                    (const_int 68 [0x44])))
            (clobber (reg:QI 31 r31))
        ]) "fail1.c":28:5 175 {addhi3_clobber}
     (nil))

Further down, it continues with success = 0:

      if (success)
        delete_insn (insn);
      changed |= success;
      insn = next;
      [...]
      continue;

The scan then continues with NEXT_INSN (insn), which is the insn AFTER insn 15,
so the CLOBBER of QI:31 in insn 15 is bypassed, and note_stores or similar is
never executed on insn 15.  The "set = single_set (next)" also bypasses that
insn 15 is a PARALLEL with a CLOBBER of a general purpose register.

Appears the code is in postreload since 2003, when postreload.c was split out
of reload1.c.

[Bug rtl-optimization/101188] [postreload] Uses content of a clobbered register

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

--- Comment #10 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
Created attachment 55189
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55189&action=edit
Proposed patch for postreload.cc so analysis is not bypassing "next" insn.

(In reply to Georg-Johann Lay from comment #9)
> postreload.cc::reload_cse_move2add() loops over all insns, and at some point
> it encounters
> 
> (insn 44 14 15 2 (set (reg/f:HI 14 r14 [58])
>         (reg/v/f:HI 16 r16 [orig:51 self ] [51])) "fail1.c":28:5 101
> {*movhi_split}
>      (nil))
> 
> During the analysis for that insn, it executes
> 
> 	  rtx_insn *next = next_nonnote_nondebug_insn (insn);
> 	  rtx set = NULL_RTX;
> 	  if (next)
> 	    set = single_set (next);
> 
> where next is
> 
> (insn 15 44 16 2 (parallel [
>             (set (reg/f:HI 14 r14 [58])
>                 (plus:HI (reg/f:HI 14 r14 [58])
>                     (const_int 68 [0x44])))
>             (clobber (reg:QI 31 r31))
>         ]) "fail1.c":28:5 175 {addhi3_clobber}
>      (nil))
> 
> Further down, it continues with success = 0:
> 
>       if (success)
> 	delete_insn (insn);
>       changed |= success;
>       insn = next;
>       [...]
>       continue;
> 
> The scan then continues with NEXT_INSN (insn), which is the insn AFTER insn
> 15, so the CLOBBER of QI:31 in insn 15 is bypassed, and note_stores or
> similar is never executed on insn 15.

A simple solution is to not "insn = next;" and just to pseudo-delete insn so
that NEXT_INSN (insn) works in the for loop:

diff --git a/gcc/postreload.cc b/gcc/postreload.cc
index fb392651e1b..388b8c0a506 100644
--- a/gcc/postreload.cc
+++ b/gcc/postreload.cc
@@ -2031,9 +2031,8 @@ reload_cse_move2add (rtx_insn *first)
                            }
                        }
                      if (success)
-                       delete_insn (insn);
+                       SET_INSN_DELETED (insn);
                      changed |= success;
-                     insn = next;
                      move2add_record_mode (reg);
                      reg_offset[regno]
                        = trunc_int_for_mode (added_offset + base_offset,

[Bug rtl-optimization/101188] [postreload] Uses content of a clobbered register

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

--- Comment #11 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
Created attachment 55217
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55217&action=edit
Proposed patch for postreload.cc to record clobbers of next insn + test case.

This patch solves the problem for avr and tests with no additional regressions
for avr.

--

rtl-optimization/101188: Don't bypass clobbers of some insns that are
optimized or are optimization candidates.

gcc/
        PR rtl-optimization/101188
        * postreload.cc (reload_cse_move2add): Record clobbers of next
        insn using move2add_note_store.

gcc/testsuite/
        PR rtl-optimization/101188
        * gcc.c-torture/execute/pr101188.c: New test.

[Bug rtl-optimization/101188] [postreload] Uses content of a clobbered register

[uweigand at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

Ulrich Weigand <uweigand at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |uweigand at gcc dot gnu.org

--- Comment #12 from Ulrich Weigand <uweigand at gcc dot gnu.org> ---
Sorry for not responding earlier, I've been out on vacation.

I think your root cause analysis is correct.  In this part of code:

              if (success)
                delete_insn (insn);
              changed |= success;
              insn = next;
              move2add_record_mode (reg);
              reg_offset[regno]
                = trunc_int_for_mode (added_offset + base_offset,
                                      mode);
              continue;

the intent seems to be to manually update the move2add data structures to
account for the effects of "next", because the default logic is now skipped for
the "next" insn.  That's why in particular the reg mode and offset are manually
calculated.

This manual logic however is really only correct if "next" is actually just a
simple SET.  Reading the comment before the whole loop:
      /* For simplicity, we only perform this optimization on
         straightforward SETs.  */
makes me suspect the original author assumed that "next" is in fact a
straightforward SET here as well.  This is however not true due to behavior of
the "single_set" extractor.  (I'm wondering if "single_set" used to be defined
differently back in the days?)

Your fix does look correct to me as far as handling parallel CLOBBERs go. 
However, looking at "single_set", it seems there is yet another case: the
extractor also accepts a parallel of two or more SETs, as long as all except
one of those SETs have destinations that are dead.  These cases would still not
be handled correctly with your patch, I think.

I'm wondering whether it is even worthwhile to attempt to cover those cases. 
Maybe a more straightforward fix would be to keep in line with the
above-mentioned comment about "straightforward SETs" and just check for a
single SET directly instead of using "single_set" here.  Do you think this
would miss any important optimizations?

[Bug rtl-optimization/101188] [postreload] Uses content of a clobbered register

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

--- Comment #13 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
(In reply to Ulrich Weigand from comment #12)
> I think your root cause analysis is correct.  In this part of code:
> 
>               if (success)
>                 delete_insn (insn);
>               changed |= success;
>               insn = next;
>               move2add_record_mode (reg);
>               reg_offset[regno]
>                 = trunc_int_for_mode (added_offset + base_offset,
>                                       mode);
>               continue;
> 
> the intent seems to be to manually update the move2add data structures to
> account for the effects of "next", because the default logic is now skipped
> for the "next" insn.  That's why in particular the reg mode and offset are
> manually calculated.
> 
> This manual logic however is really only correct if "next" is actually just
> a simple SET.  Reading the comment before the whole loop:
>       /* For simplicity, we only perform this optimization on
>          straightforward SETs.  */
> makes me suspect the original author assumed that "next" is in fact a
> straightforward SET here as well.

That would render the optimization far less likely, e..g in the case of clobber
of CCmode regs.  I understodd the comment as only referring to "insn", not
necessarily to "next".

> This is however not true due to behavior
> of the "single_set" extractor.  (I'm wondering if "single_set" used to be
> defined differently back in the days?)
> Your fix does look correct to me as far as handling parallel CLOBBERs go. 
> However, looking at "single_set", it seems there is yet another case: the
> extractor also accepts a parallel of two or more SETs, as long as all except
> one of those SETs have destinations that are dead.  These cases would still
> not be handled correctly with your patch, I think.
> 
> I'm wondering whether it is even worthwhile to attempt to cover those cases.
> Maybe a more straightforward fix would be to keep in line with the
> above-mentioned comment about "straightforward SETs" and just check for a
> single SET directly instead of using "single_set" here.  Do you think this
> would miss any important optimizations?

Not sure about how many optimizations this would kill.  Many insns are
parallells that also set CCmode regs which don't interfere with this
optimization, but only considering SETs would skip all such optimizations on
targets that can have CCmode during reload (avr is not one of them).

Also I don't have a test case for your scenario.  I can reproduce the bug back
to v5 on avr and maybe it is even older.  As it appears, this PR lead to no
hickups on any other target, so for now I'd like to keep the fix restricted to
what I can test.

I already started a review this morning:
https://gcc.gnu.org/pipermail/gcc-patches/2023-June/620446.html

[Bug rtl-optimization/101188] [postreload] Uses content of a clobbered register

[uweigand at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

--- Comment #14 from Ulrich Weigand <uweigand at gcc dot gnu.org> ---
(In reply to Georg-Johann Lay from comment #13)
> Also I don't have a test case for your scenario.  I can reproduce the bug
> back to v5 on avr and maybe it is even older.  As it appears, this PR lead
> to no hickups on any other target, so for now I'd like to keep the fix
> restricted to what I can test.

I agree that your patch looks correct and unlikely to cause any new problems,
so I won't object to it being committed.  I just wanted to point out that it
might not be a complete fix.

[Bug rtl-optimization/101188] [postreload] Uses content of a clobbered register

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jeff Law <law@gcc.gnu.org>:

https://gcc.gnu.org/g:ae193f9008e02683e27f3c87f3b06f38e103b1d0

commit r14-1738-gae193f9008e02683e27f3c87f3b06f38e103b1d0
Author: Jeff Law <jlaw@ventanamicro.com>
Date:   Mon Jun 12 12:52:10 2023 -0600

    [committed] [PR rtl-optimization/101188] Fix reload_cse_move2add ignoring
clobbers

    So as Georg-Johann discusses in the BZ, reload_cse_move2add can generate
     incorrect code when optimizing code with clobbers.  Specifically in the
    case where we try to optimize a sequence of 4 operations down to 3
    operations we can reset INSN to the next instruction and continue the loop.

    That skips the code to invalidate objects based on things like REG_INC
    nodes, stack pushes and most importantly clobbers attached to the current
    insn.

    This patch factors all of the invalidation code used by reload_cse_move2add
    into a new function and calls it at the appropriate time.

    Georg-Johann has confirmed this patch fixes his avr bug and I've had it in
    my tester over the weekend.  It's bootstrapped and regression tested on
    aarch64, m68k, sh4, alpha and hppa.  It's also regression tested
successfully
    on a wide variety of other targets.

    gcc/
            PR rtl-optimization/101188
            * postreload.cc (reload_cse_move2add_invalidate): New function,
            extracted from...
            (reload_cse_move2add): Call reload_cse_move2add_invalidate.

    gcc/testsuite
            PR rtl-optimization/101188
            * gcc.c-torture/execute/pr101188.c: New test

[Bug rtl-optimization/101188] [postreload] Uses content of a clobbered register

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

--- Comment #16 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
Created attachment 55317
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55317&action=edit
Backport to v13 of Jeff's patch.

[Bug rtl-optimization/101188] [postreload] Uses content of a clobbered register

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

--- Comment #17 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
Created attachment 55318
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55318&action=edit
Reduced patch for v8 (handles only CLOBBERs, hence should be good enough for
AVR).

[Bug rtl-optimization/101188] [postreload] Uses content of a clobbered register

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #18 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
Fixed in v14.

[Bug rtl-optimization/101188] [11/12/13 Regression] [postreload] Uses content of a clobbered register

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |---
             Status|RESOLVED                    |REOPENED
            Summary|[postreload] Uses content   |[11/12/13 Regression]
                   |of a clobbered register     |[postreload] Uses content
                   |                            |of a clobbered register

--- Comment #19 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
Reopened for back-porting.

[Bug rtl-optimization/101188] [11/12/13 Regression] [postreload] Uses content of a clobbered register

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |law at gcc dot gnu.org
   Target Milestone|---                         |11.5
             Status|REOPENED                    |ASSIGNED

[Bug rtl-optimization/101188] [11/12/13 Regression] [postreload] Uses content of a clobbered register

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101188

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2