[Bug tree-optimization/56360] New: Loop invariant motion can introduce speculative store

[Bug tree-optimization/56360] New: Loop invariant motion can introduce speculative store

[ian at airs dot com]

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

             Bug #: 56360
           Summary: Loop invariant motion can introduce speculative store
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: tree-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: ian@airs.com


When I compile this C++ program with -O3 -std=gnu+11 on x86_64:

#include<mutex>
#include<thread>

extern int a;
std::mutex m;

void nop2(int*);

// this function is ONLY called with i=0.
void foo(int i)
{
  if (i) m.lock();
  nop2(&i); // Only here to prevent optimiser from creating one big if().
  for (int c = 0; c < 10000; ++c) {
    if (i) {
      ++a; // Thus this is never executed.
    }
  }
  nop2(&i); // Only here to prevent optimiser from creating one big if().
  if (i) m.unlock();
}

I see this in _Z3fooi:

        movl    a(%rip), %eax
        movl    12(%rsp), %ecx
        leaq    12(%rsp), %rdi
        leal    10000(%rax), %edx
        testl   %ecx, %ecx
        cmovne  %edx, %eax
        movl    %eax, a(%rip)

This unconditionally stores a value in a.  This is a speculative store, which
is invalid according to the C++ memory model.

This does not happen with -O2.

>From looking at the dumps, the bug appears to be in the loop invariant motion
pass.

[Bug tree-optimization/56360] Loop invariant motion can introduce speculative store

[ian at airs dot com]

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

--- Comment #1 from Ian Lance Taylor <ian at airs dot com> 2013-02-17 04:21:28 UTC ---
The speculative store can be disabled via --param allow-store-data-races=0.

So perhaps the question is: shouldn't that be set by -std=gnu++11?

[Bug tree-optimization/56360] Loop invariant motion can introduce speculative store

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> 2013-02-17 04:55:40 UTC ---
(In reply to comment #1)
> The speculative store can be disabled via --param allow-store-data-races=0.
> 
> So perhaps the question is: shouldn't that be set by -std=gnu++11?

As I understand the C++11 memory model is not fully there in 4.8.

[Bug tree-optimization/56360] Loop invariant motion can introduce speculative store

[pinskia at gcc dot gnu.org]

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> 2013-02-17 04:58:05 UTC ---
http://gcc.gnu.org/wiki/Atomic/GCCMM/gcc4.8 describes what is left.  bitfields
is a big issue.

[Bug tree-optimization/56360] Loop invariant motion can introduce speculative store

[ian at airs dot com]

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

--- Comment #4 from Ian Lance Taylor <ian at airs dot com> 2013-02-17 05:15:01 UTC ---
Bitfields are an issue but I thought that speculative stores were fixed.

[Bug tree-optimization/56360] Loop invariant motion can introduce speculative store

[pinskia at gcc dot gnu.org]

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

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> 2013-02-17 05:46:09 UTC ---
>7. Add flag for multi-threaded vs single threaded.

is still left and that is what needs to turn on --param
allow-store-data-races=0 .

[Bug tree-optimization/56360] Loop invariant motion can introduce speculative store

[ian at airs dot com]

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

--- Comment #6 from Ian Lance Taylor <ian at airs dot com> 2013-02-17 15:33:16 UTC ---
I think this is an important goal for 4.8, as speculative stores baffle lots of
people working on multi-threaded code, including in the Linux kernel.

[Bug tree-optimization/56360] Loop invariant motion by default can introduce speculative store

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-02-18
            Summary|Loop invariant motion can   |Loop invariant motion by
                   |introduce speculative store |default can introduce
                   |                            |speculative store
     Ever Confirmed|0                           |1

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> 2013-02-18 10:49:12 UTC ---
It's a bit late to flip the bit for 4.8 (fallout from little testing coverage
of the disallow-store-data-racse paths).  Also the --param should become a
regular switch before we flip its default anywhere.

I changed the summary according to facts.

[Bug tree-optimization/56360] Loop invariant motion by default can introduce speculative store

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
      Known to work|                            |10.1.0
             Status|NEW                         |RESOLVED

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed in GCC 10 where there now is (default off) -fallow-store-data-races.