[Bug rtl-optimization/45235] New: const volatile read moved out of order

[Bug rtl-optimization/45235] New: const volatile read moved out of order

[bigotp at acm dot org]

This example is reduced from hardware-specific code that uses memory-mapped
registers to control and sample a logical signal.  The read of the input signal
is moved to follow the clear of the output signal, in violation of the
requirements for volatile memory access.

Reproduce with:  gcc -S -O2 ira-bug.c

Examine the generated assembly code to verify the read of in has moved to
follow the second write of out within the loop body.

The presence of the const qualifier on the in variable enables the bug; if the
qualifier is removed the original order is retained.


volatile const short int in;
volatile short int out;

void func () {
  short int value;
  do {
    out |= 2;
    value = in;
    out &= ~2;
  } while (value & 1);
}


-- 
           Summary: const volatile read moved out of order
           Product: gcc
           Version: 4.5.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: bigotp at acm dot org
 GCC build triplet: x86_64-unknown-linux-gnu
  GCC host triplet: x86_64-unknown-linux-gnu
GCC target triplet: x86_64-unknown-linux-gnu


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

[Bug rtl-optimization/45235] const volatile read moved out of order

[bigotp at acm dot org]

------- Comment #1 from bigotp at acm dot org  2010-08-09 11:57 -------
Created an attachment (id=21441)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21441&action=view)
fixes for assumption that readonly means constant

The problem is caused by the same sort of test as was fixed in a different
situation in #35729.  In the attached patch, it is the change to
rtanal.c:rtx_varies_p that fixes the problem.  The remaining changes are
plausible, but I don't know whether they're necessary.  There are additional
uses of MEM_READONLY_P that are also questionable, that I didn't get around
to trying.  I suggest a thorough review by somebody who, unlike me, knows GCC
internals.


-- 


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

[Bug rtl-optimization/45235] const volatile read moved out of order

[rguenth at gcc dot gnu dot org]

------- Comment #2 from rguenth at gcc dot gnu dot org  2010-08-09 13:56 -------
Probably easier to not set TREE_READONLY or MEM_READONY_P here.

Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c      (revision 163030)
+++ gcc/emit-rtl.c      (working copy)
@@ -1669,7 +1670,8 @@ set_mem_attributes_minus_bitpos (rtx ref
       base = get_base_address (base);
       if (base && DECL_P (base)
          && TREE_READONLY (base)
-         && (TREE_STATIC (base) || DECL_EXTERNAL (base)))
+         && (TREE_STATIC (base) || DECL_EXTERNAL (base))
+         && !TREE_THIS_VOLATILE (base))
        MEM_READONLY_P (ref) = 1;

       /* If this expression uses it's parent's alias set, mark it such


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
           Keywords|                            |wrong-code
   Last reconfirmed|0000-00-00 00:00:00         |2010-08-09 13:56:06
               date|                            |


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

[Bug rtl-optimization/45235] const volatile read moved out of order

[pinskia at gcc dot gnu dot org]

------- Comment #3 from pinskia at gcc dot gnu dot org  2010-08-11 21:41 -------
Hmm, I don't think this is correct as const volatile is a bit weird.  It means
a read must happen but it does not say order compared to other volatile
variables (or at least I think).


-- 


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

[Bug rtl-optimization/45235] const volatile read moved out of order

[bigotp at acm dot org]

------- Comment #4 from bigotp at acm dot org  2010-08-11 22:54 -------
I don't see that the const qualifier should be relevant: doesn't it simply
indicate that the code is not permitted to write through that lvalue?  (FWIW,
the real code uses a memory mapped address and the const qualifier was placed
on the address declaration by the chip manufacturer.  I assume that a write to
the location may be interpreted as a command to the hardware to self-destruct.)

Given that, the existing behavior appears to be a clear violation of the
requirement that volatile accesses not be re-ordered across sequence points.

In concept, I like rguenth's solution (which I haven't tested), as it fits with
the documented semantics of MEM_READONLY_P indicating "not modified during the
lifetime of the program", something that certainly can't be said of any
readable volatile object.  That the RTL flag is stored in a field named
"unchanging" is a lot more clear than "READONLY".  I think that approach
eliminates the need to muck with any of those other places where the same
problem might be occurring.


-- 


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

[Bug rtl-optimization/45235] const volatile read moved out of order

[rguenth at gcc dot gnu dot org]

------- Comment #5 from rguenth at gcc dot gnu dot org  2010-08-11 23:11 -------
(In reply to comment #4)
> I don't see that the const qualifier should be relevant: doesn't it simply
> indicate that the code is not permitted to write through that lvalue?

That's true which is why I think this bug is valid.

>  (FWIW,
> the real code uses a memory mapped address and the const qualifier was placed
> on the address declaration by the chip manufacturer.  I assume that a write to
> the location may be interpreted as a command to the hardware to self-destruct.)
> 
> Given that, the existing behavior appears to be a clear violation of the
> requirement that volatile accesses not be re-ordered across sequence points.
> 
> In concept, I like rguenth's solution (which I haven't tested), as it fits with
> the documented semantics of MEM_READONLY_P indicating "not modified during the
> lifetime of the program", something that certainly can't be said of any
> readable volatile object.  That the RTL flag is stored in a field named
> "unchanging" is a lot more clear than "READONLY".  I think that approach
> eliminates the need to muck with any of those other places where the same
> problem might be occurring.

Indeed.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |rguenth at gcc dot gnu dot
                   |dot org                     |org
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2010-08-09 13:56:06         |2010-08-11 23:11:05
               date|                            |


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

[Bug rtl-optimization/45235] const volatile read moved out of order

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.6.0

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> 2011-12-15 00:34:17 UTC ---
Fixed.

[Bug rtl-optimization/45235] const volatile read moved out of order

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |4.6.0

--- Comment #8 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-01-11 15:42:12 UTC ---
Fixed on trunk sofar.

[Bug rtl-optimization/45235] const volatile read moved out of order

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-01-11 15:41:23 UTC ---
Author: rguenth
Date: Tue Jan 11 15:41:17 2011
New Revision: 168663

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=168663
Log:
2011-01-11  Richard Guenther  <rguenther@suse.de>

    PR middle-end/45235
    * emit-rtl.c (set_mem_attributes_minus_bitpos): Do not mark
    volatile MEMs as MEM_READONLY_P.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/emit-rtl.c

[Bug rtl-optimization/45235] const volatile read moved out of order

[bigotp at acm dot org]

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

--- Comment #6 from Peter A. Bigot <bigotp at acm dot org> 2010-12-31 17:57:14 UTC ---
I've been running with this since my last comment with no problems.  Could this
be integrated into as many of trunk, gcc-4_4-branch, and gcc-4_5-branch as
possible, please?  Thanks.