Bug 8991 – adding a __ctfe branch with return to a function breaks NRVO

Status
NEW
Severity
normal
Priority
P3
Component
dmd
Product
D
Version
D2
Platform
All
OS
All
Creation time
2012-11-10T03:14:36Z
Last change time
2024-12-13T18:02:43Z
Assigned to
No Owner
Creator
Dmitry Olshansky
Moved to GitHub: dmd#18490 →

Comments

Comment #0 by dmitry.olsh — 2012-11-10T03:14:36Z
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 }
Comment #1 by maxim — 2012-11-10T11:42:18Z
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).
Comment #2 by dmitry.olsh — 2012-11-11T01:20:32Z
(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.
Comment #3 by clugdbug — 2012-11-14T01:08:32Z
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.
Comment #4 by dmitry.olsh — 2012-11-14T06:42:57Z
(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.
Comment #5 by robert.schadek — 2024-12-13T18:02:43Z
THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/dmd/issues/18490 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB