http://d.puremagic.com/issues/show_bug.cgi?id=9163



--- Comment #10 from Johannes Pfau <johannespfau@gmail.com> 2012-12-16 08:24:13 PST ---
(In reply to comment #3)
> I'm not sure that this isn't just a GDC bug. Why should status necessarily be thread-local?

Maybe my conclusions were premature, but according to TDPL chapter 13.12.1 page 414: "The compiler optimizes code using non-shared data to the maximum, in full confidence that no other thread can ever access it, and only tiptoes around shared data".

The status field is not TLS data as it's not a static field, but it's instance data and to my understanding if the class instance isn't marked as shared and the field isn't shared the compiler is allowed to assume that only one thread can access that data.

And if the compiler assumes status can only be accessed from one thread and "atomicReadUbyte" is inferred as pure, that would lead to this wrong behavior.

You've got a point that this can't work for __gshared, so I'm not really sure what to do about this.

So is it always invalid to optimize this code in D?
---------------------------------------
pure bool checkFlag(const ref bool a)
{

}

class A
{
    bool flag;

    void test()
    {
        while(true)
        {
            if(checkFlag(flag)) //Can this check be moved out of the loop
                return;
        }
    }
}
--------------------------------------

(In reply to comment #2)
> 
> I don't think the use of volatile statements/variables may help here...

wouldn't status as a volatile field signal to the compiler that it's value might change between calls to "atomicReadUbyte" and therefore prevent this optimization?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9163



--- Comment #11 from Iain Buclaw <ibuclaw@ubuntu.com> 2012-12-16 08:27:35 PST ---
(In reply to comment #10)
> (In reply to comment #3)
> > I'm not sure that this isn't just a GDC bug. Why should status necessarily be thread-local?
> 
> Maybe my conclusions were premature, but according to TDPL chapter 13.12.1 page 414: "The compiler optimizes code using non-shared data to the maximum, in full confidence that no other thread can ever access it, and only tiptoes around shared data".
> 
> The status field is not TLS data as it's not a static field, but it's instance data and to my understanding if the class instance isn't marked as shared and the field isn't shared the compiler is allowed to assume that only one thread can access that data.
> 
> And if the compiler assumes status can only be accessed from one thread and "atomicReadUbyte" is inferred as pure, that would lead to this wrong behavior.
> 
> You've got a point that this can't work for __gshared, so I'm not really sure what to do about this.
> 
> So is it always invalid to optimize this code in D?
> ---------------------------------------
> pure bool checkFlag(const ref bool a)
> {
> 
> }
> 
> class A
> {
>     bool flag;
> 
>     void test()
>     {
>         while(true)
>         {
>             if(checkFlag(flag)) //Can this check be moved out of the loop
>                 return;
>         }
>     }
> }
> --------------------------------------
> 
> (In reply to comment #2)
> > 
> > I don't think the use of volatile statements/variables may help here...
> 
> wouldn't status as a volatile field signal to the compiler that it's value might change between calls to "atomicReadUbyte" and therefore prevent this optimization?

atomicReadUbyte isn't pure, but it is inlined.  Leaving atomicLoad as the culprit.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9163



--- Comment #12 from David Nadlinger <code@klickverbot.at> 2012-12-16 08:29:56 PST ---
Just as a general note, because not everybody might be aware of this: GCC has its own "pure" attribute with semantics different from its namesake in the D langauge.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9163


Iain Buclaw <ibuclaw@ubuntu.com> changed:

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


--- Comment #13 from Iain Buclaw <ibuclaw@ubuntu.com> 2013-01-03 13:45:09 PST ---
Can no longer reproduce this in gdc after some changes. The dmd compiler may not do everything right, but then again sometimes neither does gdc. :o)

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9163


David Simcha <dsimcha@yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dsimcha@yahoo.com


--- Comment #14 from David Simcha <dsimcha@yahoo.com> 2013-01-06 05:56:19 PST ---
Sorry, I've been incredibly busy lately and just saw this post now.  IIUC the **whole point** of atomic operations in core.atomic is that they're supposed to act as fences.  No code motion should take place across ASM statements at the compiler level, and none should take place across lock; instructions at the lower levels.  Obviously either my understanding of the spec is wrong or GDC isn't optimizing them properly if that's the case.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------