www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 8991] New: adding a __ctfe branch with return to a function breaks NRVO

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=8991

           Summary: adding a __ctfe branch with return to a function
                    breaks NRVO
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: nobody puremagic.com
        ReportedBy: dmitry.olsh gmail.com


--- Comment #0 from Dmitry Olshansky <dmitry.olsh gmail.com> 2012-11-10
03:14:36 PST ---
Sample obtained while trying to make move work (at least making a copy) during
CTFE.
In the following snippet if __ctfe section is commented out, then return value
doesn't get copied. If it's present however there is a postblit call.

The expected result is that __ctfe should never affect code generation of
run-time code.

Tested on DMD 2.061 from git master.

import core.stdc.string;

T move(T)(ref T source)
{    
    if (__ctfe)
    {
    *cast(int*)0 = 0; //to demonstrate that no CTFE is attempted
        T result = source;    
        return result;   //must have broke NRVO
    }   

    T result = void;


    static if (is(T == struct))
    {
    static T empty;
        memcpy(&result, &source, T.sizeof);
    memcpy(&source, &empty, T.sizeof);
    }
    else
    {
        result = source;
    }
    return result;
}

unittest
{
    // Issue 5661 test(2)
    static struct S4
    {
        static struct X
        { 
            int n = 0; 
            this(this){n = 0;} 
        }
        X x;
    }
    S4 s41;
    s41.x.n = 1;
    S4 s42 = move(s41);
    assert(s41.x.n == 0); //ok, memcpy-ed T.init over source
    assert(s42.x.n == 1); //fails, postblit got called somewhere 
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 10 2012
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=8991


Maxim Fomin <maxim maxim-fomin.ru> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |maxim maxim-fomin.ru


--- Comment #1 from Maxim Fomin <maxim maxim-fomin.ru> 2012-11-10 11:42:18 PST
---
It seems that dmd is confused by return statement within if(__ctfe) block:
comment it out and you will get desired behavior (tested on 2.060nix).

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 10 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=8991



--- Comment #2 from Dmitry Olshansky <dmitry.olsh gmail.com> 2012-11-11
01:20:32 PST ---
(In reply to comment #1)

 It seems that dmd is confused by return statement within if(__ctfe) block:
 comment it out and you will get desired behavior (tested on 2.060nix).


Yeah, problem is: 
I want __ctfe branch to just do a copy and normal branch to move via memcpy and
other black magic.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 11 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=8991



--- Comment #3 from Don <clugdbug yahoo.com.au> 2012-11-14 01:08:32 PST ---
I have no idea why this evil hacky code exists in Phobos, I cannot see how it
can possibly be correct. If it bypasses postblit, surely that's wrong.
If it is faster to use memcpy(), that's a compiler bug.

But anyway, it fails because it detects that two different variables are being
returned.
The workaround is easy:

+    T result = void;
  if (__ctfe)
    {
    *cast(int*)0 = 0; //to demonstrate that no CTFE is attempted
-        T result = source;    
+        result = source;    
        return result;   //must have broke NRVO
    }   

-    T result = void;

Now, although it would be possible to hack IfStatement::semantic to check for
__ctfe
 or !__ctfe, and restore fd->nrvo_var, this would fail in cases like:

   if (__ctfe) {
Lnasty:
       T result = source;
       return result;
   }
   if (b)  goto Lnasty;
   T result = void;
   return result;

The problem is, that NRVO is run during the semantic pass, rather than
afterwards.
Personally I think that inlining should happen after the semantic3 pass (it
would make CTFE *much* simpler), and I think NRVO could happen then as well.
Otherwise I'm not sure that this is worth doing anything about. It is still
true that if (__ctfe ) never affects backend code generation, it's just odd
that DMD is doing NRVO in the front-end.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 14 2012
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=8991



--- Comment #4 from Dmitry Olshansky <dmitry.olsh gmail.com> 2012-11-14
06:42:57 PST ---
(In reply to comment #3)

I have no idea why this evil hacky code exists in Phobos, I cannot see how it
can possibly be correct. If it bypasses postblit, surely that's wrong.
If it is faster to use memcpy(), that's a compiler bug.


The whole point of move is to avoid extra postblit and turn l-value into an
r-value.
The way to do it seems to be (and very simillar in swap) is to blit T.init into
source and whatever it contained before return as result. 
The source will eventually get destroyed with T.init. 

Thus the postblit is not required and no double destroy of the same object
happens.

While I thought this could work to paint things as r-value:
T move(ref T x ){ return x; }

it will still do a postblit as ref-ed param stays intact elsewhere.


 The workaround is easy:
 
 +    T result = void;
   if (__ctfe)
     {
     *cast(int*)0 = 0; //to demonstrate that no CTFE is attempted
 -        T result = source;    
 +        result = source;    
         return result;   //must have broke NRVO
     }   
 


That was what I did in the first place. Problem is  - it doesn't work for
structs with immutable fields as after:
    T result = void; 
this line:
    result = source;
does't compile. 
I wouldn't have noticed this if Phobos unittests haven't failed
while memcpy hacked through any such inconveniences.

In any case I've found a workaround that seems to pass through:
https://github.com/D-Programming-Language/phobos/pull/936


 The problem is, that NRVO is run during the semantic pass, rather than
 afterwards.
 Personally I think that inlining should happen after the semantic3 pass (it
 would make CTFE *much* simpler), and I think NRVO could happen then as well.
 Otherwise I'm not sure that this is worth doing anything about.


Okay let's either close it or turn into an enhancement request for doing
inlining and NRVO after completion of the semantic pass.


 It is still
 true that if (__ctfe ) never affects backend code generation, it's just odd
 that DMD is doing NRVO in the front-end.


Okay that makes it clearer.

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