[Bug c/51374] New: Volatile access reordered.

[Bug c/51374] New: Volatile access reordered.

[andyw at pobox dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51374

             Bug #: 51374
           Summary: Volatile access reordered.
    Classification: Unclassified
           Product: gcc
           Version: 4.6.2
            Status: UNCONFIRMED
          Severity: major
          Priority: P3
         Component: c
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: andyw@pobox.com
              Host: i386-redhat-linux
            Target: avr


Created attachment 25964
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25964
.i file demonstrating the bug.

It appears that if the compiler feels able to use the sbis instruction, the
ordering of accesses no longer respects the volatile keyword.

In attached test case, the status register (UCSR0A) needs to be read before the
data register (UDR0) in order to be valid.

The assembler output shows that the sbis instruction is used to test UCSR0A
after UDR0 has been read.

Decoupling assignment and declaration has no effect on this behaviour.

Selected highlights from the avr-gcc mailing list thread leading to this bug
report:

Well, in avr.md we have

;; Lower half of the I/O space - use sbic/sbis directly.
(define_insn "*sbix_branch"
  [(set (pc)
        (if_then_else
         (match_operator 0 "eqne_operator"
                         [(zero_extract:HI
                           (mem:QI (match_operand 1 "low_io_address_operand" 
"n"))
                           (const_int 1)
                           (match_operand 2 "const_int_operand" "n"))
                          (const_int 0)])
         (label_ref (match_operand 3 "" ""))
         (pc)))]
  "(optimize > 0)"
  "* return avr_out_sbxx_branch (insn, operands);"

The problem is that there is

   (mem:QI (match_operand 1 "low_io_address_operand"))

instead of

   (match_operand:QI 1 "low_io_memory_operand"))


Actually, there is no low_io_memory_operand predicate, but if there was one it
would solve this problem.

The reason is that a plain mem is not sensitive to volatile (resp. volatile_ok)
but a memory predicate is.

It was written that way because otherwise, the pattern would *never* match
because the only reason to have it is to produce SBIS/SBIC which only work on
I/O which is volatile.

I am really unsure of combine can be blamed here because the operand that it
moves over the other volatile access (UDR0) is just an integer, namely a
low_io_address_operand one.

I'd suggest to ask in address@hidden if insn combine may do it.

If yes, we are really in trouble because there is no obvious way how avr.md can
fix it without kicking out SBIS/SBIC/SBI/SBC.

[Bug rtl-optimization/51374] [avr] insn combine reorders volatile memory accesses

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51374

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
      Known to work|                            |4.7.0
           Keywords|                            |wrong-code
   Last reconfirmed|                            |2011-12-01
          Component|c                           |rtl-optimization
                 CC|                            |gjl at gcc dot gnu.org
               Host|i386-redhat-linux           |
     Ever Confirmed|0                           |1
            Summary|Volatile access reordered.  |[avr] insn combine reorders
                   |                            |volatile memory accesses
   Target Milestone|---                         |4.6.3
      Known to fail|                            |4.6.2

--- Comment #1 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2011-12-01 09:57:16 UTC ---
Please, supply informations that are needed to reproduce bugs like explained in
http://gcc.gnu.org/bugs.html/#need when you report bugs like compiler switches
applied. Thanks.

To reprodoce, optimization must be turned on:

$ avr-gcc-4.6.2 test.c -Os -dp -S

__vector_18:
    in r24,44-0x20     ;  8    *movqi/4    [length = 1]
    sbis 43-0x20,4     ;  12    *sbix_branch    [length = 2]
    rjmp .L1
    lds r24,slot.1198     ;  14    *movhi/2    [length = 4]
    lds r25,slot.1198+1
    ldi r18,hi8(-2)     ;  15    *cmphi/5    [length = 3]
    cpi r24,lo8(-2)
    cpc r25,r18
    brne .L1     ;  16    branch    [length = 1]
    ldi r24,lo8(-1)     ;  18    *movhi/4    [length = 2]
    ldi r25,hi8(-1)
    sts slot.1198+1,r25     ;  19    *movhi/3    [length = 4]
    sts slot.1198,r24
.L1:
    ret     ;  29    return    [length = 1]


avr-gcc-4.7.0 (trunk 181838) compiles correct with -O1/2/s/3, here with -Os:

__vector_18:
    in r24,0xb     ;  6    movqi_insn/4    [length = 1]
    in r25,0xc     ;  8    movqi_insn/4    [length = 1]
    sbrs r24,4     ;  11    *sbrx_branchqi    [length = 2]
    rjmp .L1
    lds r24,slot.1321     ;  13    *movhi/3    [length = 4]
    lds r25,slot.1321+1
    adiw r24,2     ;  14    *cmphi/7    [length = 1]
    brne .L1     ;  15    branch    [length = 1]
    ldi r24,lo8(-1)     ;  17    *movhi/5    [length = 2]
    ldi r25,lo8(-1)
    sts slot.1321+1,r25     ;  18    *movhi/4    [length = 4]
    sts slot.1321,r24
.L1:
    ret     ;  35    return    [length = 1]

[Bug rtl-optimization/51374] [avr] insn combine reorders volatile memory accesses

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51374

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #25964|0                           |1
        is obsolete|                            |

--- Comment #2 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2011-12-01 09:59:24 UTC ---
Created attachment 25965
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25965
pr51374.c

Use MIME text/plain

[Bug rtl-optimization/51374] [avr] insn combine reorders volatile memory accesses

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51374

--- Comment #3 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2011-12-01 10:08:05 UTC ---
ALso notice, that with the following test case that reads status *after* data

typedef unsigned char uint8_t;

void __vector_18(void)
{
    unsigned char data = (*(volatile uint8_t *)((0x0C) + 0x20));
    unsigned char status = (*(volatile uint8_t *)((0x0B) + 0x20));
    static int slot;

    if (status & 0x10)
    {
        if (slot == (-2))
        {
            slot ++;
        }
    }
}


then *no* SBIS/SBIC instruction is produced:

__vector_18:
    in r24,0xc     ;  6    movqi_insn/4    [length = 1]
    in r24,0xb     ;  8    movqi_insn/4    [length = 1]
    sbrs r24,4     ;  11    *sbrx_branchqi    [length = 2]
    rjmp .L1
...

and combine says:

Trying 9, 10 -> 11:
Successfully matched this instruction:
(set (pc)
    (if_then_else (eq (zero_extract:QI (reg/v:QI 44 [ status ])
                (const_int 1 [0x1])
                (const_int 4 [0x4]))
            (const_int 0 [0]))
        (label_ref:HI 21)
        (pc)))
deferring deletion of insn with uid = 10.
deferring deletion of insn with uid = 9.
modifying insn i3    11 pc={(zero_extract(r44:QI,0x1,0x4)==0)?L21:pc}
      REG_DEAD: r44:QI
      REG_BR_PROB: 0xf3c
deferring rescan insn with uid = 11.

Trying 8 -> 11:
Failed to match this instruction:
(set (pc)
    (if_then_else (eq (zero_extract:QI (mem/v:QI (const_int 43 [0x2b]) [0
MEM[(volatile uint8_t *)43B]+0 S1 A8])
                (const_int 1 [0x1])
                (const_int 4 [0x4]))
            (const_int 0 [0]))
        (label_ref:HI 21)
        (pc)))

[Bug rtl-optimization/51374] [avr] insn combine reorders volatile memory accesses

[andyw at pobox dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51374

--- Comment #4 from andyw at pobox dot com 2011-12-01 13:22:44 UTC ---
Compiler command line & -v output showing configure options (thought I had
pasted this in the initial report - apparently not.) Do not know what happened
to the attachment, the box said text/plain...

$ avr-gcc -mmcu=atmega128 -Wall -O2 -g -c -save-temps -v test.c
Using built-in specs.
COLLECT_GCC=avr-gcc
COLLECT_LTO_WRAPPER=/usr/local/libexec/gcc/avr/4.6.2/lto-wrapper
Target: avr
Configured with: ../configure --target=avr --enable-languages=c,c++
--disable-nls --disable-libssp --with-dwarf2 : (reconfigured) ../configure
--target=avr --enable-languages=c,c++ --disable-nls --disable-libssp
--with-dwarf2 --with-gmp=/usr/local --with-mpc=/usr/local
--with-mpfr=/usr/local
Thread model: single
gcc version 4.6.2 (GCC) 
COLLECT_GCC_OPTIONS='-mmcu=atmega128' '-Wall' '-O2' '-g' '-c' '-save-temps'
'-v'
 /usr/local/libexec/gcc/avr/4.6.2/cc1 -E -quiet -v -imultilib avr51 test.c
-mmcu=atmega128 -Wall -g -fworking-directory -O2 -fpch-preprocess -o test.i
ignoring nonexistent directory
"/usr/local/lib/gcc/avr/4.6.2/../../../../avr/sys-include"
#include "..." search starts here:
#include <...> search starts here:
 /usr/local/lib/gcc/avr/4.6.2/include
 /usr/local/lib/gcc/avr/4.6.2/include-fixed
 /usr/local/lib/gcc/avr/4.6.2/../../../../avr/include
End of search list.
COLLECT_GCC_OPTIONS='-mmcu=atmega128' '-Wall' '-O2' '-g' '-c' '-save-temps'
'-v'
 /usr/local/libexec/gcc/avr/4.6.2/cc1 -fpreprocessed test.i -quiet -dumpbase
test.c -mmcu=atmega128 -auxbase test -g -O2 -Wall -version -o test.s
GNU C (GCC) version 4.6.2 (avr)
        compiled by GNU C version 4.1.2 20070925 (Red Hat 4.1.2-33), GMP
version 4.3.2, MPFR version 2.4.2, MPC version 0.8.1
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
GNU C (GCC) version 4.6.2 (avr)
        compiled by GNU C version 4.1.2 20070925 (Red Hat 4.1.2-33), GMP
version 4.3.2, MPFR version 2.4.2, MPC version 0.8.1
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
Compiler executable checksum: 8445704570d64b6b795b8847e7060bc6
test.c: In function '__vector_18':
test.c:10:16: warning: unused variable 'data' [-Wunused-variable]
COLLECT_GCC_OPTIONS='-mmcu=atmega128' '-Wall' '-O2' '-g' '-c' '-save-temps'
'-v'
 /usr/local/lib/gcc/avr/4.6.2/../../../../avr/bin/as -mmcu=atmega128 -o test.o
test.s
COMPILER_PATH=/usr/local/libexec/gcc/avr/4.6.2/:/usr/local/libexec/gcc/avr/4.6.2/:/usr/local/libexec/gcc/avr/:/usr/local/lib/gcc/avr/4.6.2/:/usr/local/lib/gcc/avr/:/usr/local/lib/gcc/avr/4.6.2/../../../../avr/bin/
LIBRARY_PATH=/usr/local/lib/gcc/avr/4.6.2/avr51/:/usr/local/lib/gcc/avr/4.6.2/../../../../avr/lib/avr51/:/usr/local/lib/gcc/avr/4.6.2/:/usr/local/lib/gcc/avr/4.6.2/../../../../avr/lib/
COLLECT_GCC_OPTIONS='-mmcu=atmega128' '-Wall' '-O2' '-g' '-c' '-save-temps'
'-v'
$

[Bug rtl-optimization/51374] [avr] insn combine reorders volatile memory accesses

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51374

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|4.7.0                       |
      Known to fail|                            |4.7.0

--- Comment #5 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2011-12-08 16:40:28 UTC ---
This bug also affects 4.7.0 but up to now, the problem for 4.7 is hidden behind
PR51425 because avr-gcc 4.7 fails to produce SBIS/SBIC instructions altogether.

Smaller test case:


void __vector_18 (void)
{
    extern char slot;
    unsigned char status = (*(volatile unsigned char*) 0x2B);
    unsigned char data   = (*(volatile unsigned char*) 0x2C);

    if (status & 0x10)
        slot = 0;
}

[Bug rtl-optimization/51374] [avr] insn combine reorders volatile memory accesses

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51374

--- Comment #6 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2011-12-18 19:01:56 UTC ---


In combine.c:try_combine, just after the "Trying..." dump output, there is:

i0 = 0
i1 = 0

i2 =
(set (reg/v:QI 43 [ status ])
     (mem/v:QI (const_int 43 [0x2b])))

i3 =
(set (pc)
     (if_then_else (eq (zero_extract:QI (reg/v:QI 43 [ status ])
                                        (const_int 1)
                                        (const_int 4))
                       (const_int 0))
                   (label_ref:HI 16)
                   (pc)))

where the potential insertion is i2 into i3.

These insns are fed into can_combine_p with

src  = (mem/v:QI (const_int 43))
dest = (reg/v:QI 43)

and then there is this part of an if-clause:

      /* Make sure that the value that is to be substituted for the register
     does not use any registers whose values alter in between.  However,
     If the insns are adjacent, a use can't cross a set even though we
     think it might (this can happen for a sequence of insns each setting
     the same destination; last_set of that register might point to
     a NOTE).  If INSN has a REG_EQUIV note, the register is always
     equivalent to the memory so the substitution is valid even if there
     are intervening stores.  Also, don't move a volatile asm or
     UNSPEC_VOLATILE across any other insns.  */
      || (! all_adjacent
      && (((!MEM_P (src)
        || ! find_reg_note (insn, REG_EQUIV, src))
           && use_crosses_set_p (src, DF_INSN_LUID (insn)))
          || (GET_CODE (src) == ASM_OPERANDS && MEM_VOLATILE_P (src))
          || GET_CODE (src) == UNSPEC_VOLATILE))


In addition to these tests, the following must be disallowed:

If src contains volatile memory, then disallow moving it across:

* volatile memory
* unspec_volatile
* asm volatile

As far as I can see, use_crosses_set_p (src,...) returns 0 (false) which is
incorrect.

So either use_crosses_set_p is incorrect or it relies on incorrect data from
data flow analysis or from wherever.

[Bug rtl-optimization/51374] [avr] insn combine reorders volatile memory accesses

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51374

--- Comment #7 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2012-01-13 15:58:10 UTC ---
Created attachment 26316
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26316
pr51374.diff (tentative patch against 4.7)


    * combine.c (record_dead_and_set_regs_1): Update mem_last_set
    when reading from volatile memory or writing to mem via zero
    extract.

[Bug rtl-optimization/51374] [avr] insn combine reorders volatile memory accesses

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51374

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch

--- Comment #8 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2012-01-13 16:01:30 UTC ---
http://gcc.gnu.org/ml/gcc/2012-01/msg00142.html

[Bug rtl-optimization/51374] [avr] insn combine reorders volatile memory accesses

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51374

--- Comment #9 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2012-02-01 11:35:37 UTC ---
Author: gjl
Date: Wed Feb  1 11:35:34 2012
New Revision: 183796

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183796
Log:
gcc/
    PR rtl-optimization/51374
    * combine.c (can_combine_p): Don't allow volatile_refs_p insns
    to cross other volatile_refs_p insns.

gcc/testsuite/
    PR rtl-optimization/51374
    * testsuite/gcc.target/avr/torture/pr51374-1.c: New.


Added:
    trunk/gcc/testsuite/gcc.target/avr/torture/pr51374-1.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/combine.c
    trunk/gcc/testsuite/ChangeLog

[Bug rtl-optimization/51374] [avr] insn combine reorders volatile memory accesses

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51374

--- Comment #10 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2012-02-01 12:40:26 UTC ---
Author: gjl
Date: Wed Feb  1 12:40:23 2012
New Revision: 183797

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183797
Log:
gcc/
    Backport from mainline r183796
    PR rtl-optimization/51374
    * combine.c (can_combine_p): Don't allow volatile_refs_p insns
    to cross other volatile_refs_p insns.

gcc/testsuite/
    Backport from mainline r183796
    PR rtl-optimization/51374
    * testsuite/gcc.target/avr/torture/pr51374-1.c: New.


Added:
    branches/gcc-4_6-branch/gcc/testsuite/gcc.target/avr/torture/pr51374-1.c
Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/combine.c
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog

[Bug rtl-optimization/51374] [avr] insn combine reorders volatile memory accesses

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51374

--- Comment #11 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2012-02-01 12:46:42 UTC ---
Author: gjl
Date: Wed Feb  1 12:46:39 2012
New Revision: 183798

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183798
Log:
    PR rtl-optimization/51374
    * gcc.target/avr/torture/pr51374-1.c:
    Also fail if SBIS is seen.


Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.target/avr/torture/pr51374-1.c

[Bug rtl-optimization/51374] [avr] insn combine reorders volatile memory accesses

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51374

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

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

--- Comment #12 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2012-02-01 12:56:06 UTC ---
Fixed for the milestone.