From b7a3a1942c3a3028e7b9c2554a97dd92919ffdb5 Mon Sep 17 00:00:00 2001 From: Walter Bright Date: Wed, 18 Dec 2024 00:48:52 -0800 Subject: [PATCH] fix Issue 20567 returning the result of a constructor should be NRVO (#20568) --- compiler/src/dmd/declaration.d | 1 + compiler/src/dmd/e2ir.d | 2 +- compiler/src/dmd/expressionsem.d | 32 ++++++++++++++++++-------------- compiler/src/dmd/frontend.h | 2 ++ compiler/src/dmd/funcsem.d | 1 + compiler/src/dmd/initsem.d | 2 +- compiler/src/dmd/s2ir.d | 2 +- compiler/src/dmd/semantic3.d | 2 +- compiler/src/dmd/statementsem.d | 2 +- compiler/test/runnable/nrvo.d | 26 ++++++++++++++++++++++++++ 10 files changed, 53 insertions(+), 19 deletions(-) diff --git a/compiler/src/dmd/declaration.d b/compiler/src/dmd/declaration.d index 995995400a..67cddfa4cb 100644 --- a/compiler/src/dmd/declaration.d +++ b/compiler/src/dmd/declaration.d @@ -95,6 +95,7 @@ extern (C++) abstract class Declaration : Dsymbol enum ignoreRead = 2; // ignore any reads of AliasDeclaration enum nounderscore = 4; // don't prepend _ to mangled name enum hidden = 8; // don't print this in .di files + enum nrvo = 0x10; /// forward to fd.nrvo_var when generating code // overridden symbol with pragma(mangle, "...") const(char)[] mangleOverride; diff --git a/compiler/src/dmd/e2ir.d b/compiler/src/dmd/e2ir.d index 5ea7faa323..e120df28d3 100644 --- a/compiler/src/dmd/e2ir.d +++ b/compiler/src/dmd/e2ir.d @@ -632,7 +632,7 @@ elem* toElem(Expression e, ref IRState irs) if (se.var.toParent2()) fd = se.var.toParent2().isFuncDeclaration(); - const bool nrvo = fd && fd.isNRVO() && fd.nrvo_var == se.var; + const bool nrvo = fd && (fd.isNRVO() && fd.nrvo_var == se.var || se.var.adFlags & Declaration.nrvo && fd.shidden); if (nrvo) s = fd.shidden; diff --git a/compiler/src/dmd/expressionsem.d b/compiler/src/dmd/expressionsem.d index fa1098e732..0e5ccf3e8a 100644 --- a/compiler/src/dmd/expressionsem.d +++ b/compiler/src/dmd/expressionsem.d @@ -816,21 +816,22 @@ extern(D) bool arrayExpressionSemantic( * Params: * sc = the scope where the expression is encountered * e = the expression the needs to be moved or copied (source) - * t = if the struct defines a copy constructor, the type of the destination + * t = if the struct defines a copy constructor, the type of the destination (can be NULL) + * nrvo = true if the generated copy can be treated as NRVO * * Returns: * The expression that copy constructs or moves the value. */ -extern (D) Expression doCopyOrMove(Scope *sc, Expression e, Type t = null) +extern (D) Expression doCopyOrMove(Scope *sc, Expression e, Type t, bool nrvo) { if (auto ce = e.isCondExp()) { - ce.e1 = doCopyOrMove(sc, ce.e1); - ce.e2 = doCopyOrMove(sc, ce.e2); + ce.e1 = doCopyOrMove(sc, ce.e1, null, nrvo); + ce.e2 = doCopyOrMove(sc, ce.e2, null, nrvo); } else { - e = e.isLvalue() ? callCpCtor(sc, e, t) : valueNoDtor(e); + e = e.isLvalue() ? callCpCtor(sc, e, t, nrvo) : valueNoDtor(e); } return e; } @@ -839,12 +840,13 @@ extern (D) Expression doCopyOrMove(Scope *sc, Expression e, Type t = null) * If e is an instance of a struct, and that struct has a copy constructor, * rewrite e as: * (tmp = e),tmp - * Input: + * Params: * sc = just used to specify the scope of created temporary variable * destinationType = the type of the object on which the copy constructor is called; * may be null if the struct defines a postblit + * nrvo = true if the generated copy can be treated as NRVO */ -private Expression callCpCtor(Scope* sc, Expression e, Type destinationType) +private Expression callCpCtor(Scope* sc, Expression e, Type destinationType, bool nrvo) { //printf("callCpCtor(e: %s et: %s destinationType: %s\n", toChars(e), toChars(e.type), toChars(destinationType)); auto ts = e.type.baseElemOf().isTypeStruct(); @@ -861,7 +863,9 @@ private Expression callCpCtor(Scope* sc, Expression e, Type destinationType) * This is not the most efficient, ideally tmp would be constructed * directly onto the stack. */ - auto tmp = copyToTemp(STC.rvalue, "__copytmp", e); + VarDeclaration tmp = copyToTemp(STC.rvalue, "__copytmp", e); + if (nrvo) + tmp.adFlags |= Declaration.nrvo; if (sd.hasCopyCtor && destinationType) { // https://issues.dlang.org/show_bug.cgi?id=22619 @@ -2707,7 +2711,7 @@ private Type arrayExpressionToCommonType(Scope* sc, ref Expressions exps) continue; } - e = doCopyOrMove(sc, e); + e = doCopyOrMove(sc, e, null, false); if (!foundType && t0 && !t0.equals(e.type)) { @@ -3678,7 +3682,7 @@ private bool functionParameters(const ref Loc loc, Scope* sc, */ Type tv = arg.type.baseElemOf(); if (!isRef && tv.ty == Tstruct) - arg = doCopyOrMove(sc, arg, parameter ? parameter.type : null); + arg = doCopyOrMove(sc, arg, parameter ? parameter.type : null, false); } (*arguments)[i] = arg; @@ -12026,7 +12030,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor ce.trusted = true; exp = new CatElemAssignExp(exp.loc, exp.type, exp.e1, ecast); - exp.e2 = doCopyOrMove(sc, exp.e2); + exp.e2 = doCopyOrMove(sc, exp.e2, null, false); } else if (tb1.ty == Tarray && (tb1next.ty == Tchar || tb1next.ty == Twchar) && @@ -12651,7 +12655,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor { if (exp.e1.op == EXP.arrayLiteral) { - exp.e2 = doCopyOrMove(sc, exp.e2); + exp.e2 = doCopyOrMove(sc, exp.e2, null, false); // https://issues.dlang.org/show_bug.cgi?id=14686 // Postblit call appears in AST, and this is // finally translated to an ArrayLiteralExp in below optimize(). @@ -12690,7 +12694,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor { if (exp.e2.op == EXP.arrayLiteral) { - exp.e1 = doCopyOrMove(sc, exp.e1); + exp.e1 = doCopyOrMove(sc, exp.e1, null, false); } else if (exp.e2.op == EXP.string_) { @@ -16552,7 +16556,7 @@ private bool fit(StructDeclaration sd, const ref Loc loc, Scope* sc, Expressions if (e.op == EXP.error) return false; - (*elements)[i] = doCopyOrMove(sc, e); + (*elements)[i] = doCopyOrMove(sc, e, null, false); } return true; } diff --git a/compiler/src/dmd/frontend.h b/compiler/src/dmd/frontend.h index a4837f3e3a..4556252ce0 100644 --- a/compiler/src/dmd/frontend.h +++ b/compiler/src/dmd/frontend.h @@ -6567,6 +6567,8 @@ public: enum : int32_t { hidden = 8 }; + enum : int32_t { nrvo = 16 }; + _d_dynamicArray< const char > mangleOverride; const char* kind() const override; uinteger_t size(const Loc& loc) final override; diff --git a/compiler/src/dmd/funcsem.d b/compiler/src/dmd/funcsem.d index 837e0c6f7c..eba93973a4 100644 --- a/compiler/src/dmd/funcsem.d +++ b/compiler/src/dmd/funcsem.d @@ -2962,6 +2962,7 @@ extern (D) void checkMain(FuncDeclaration fd) */ extern (D) bool checkNRVO(FuncDeclaration fd) { + //printf("checkNRVO*() %s\n", fd.ident.toChars()); if (!fd.isNRVO() || fd.returns is null) return false; diff --git a/compiler/src/dmd/initsem.d b/compiler/src/dmd/initsem.d index 505a3e1363..0c13bc70da 100644 --- a/compiler/src/dmd/initsem.d +++ b/compiler/src/dmd/initsem.d @@ -1630,7 +1630,7 @@ Expressions* resolveStructLiteralNamedArgs(StructDeclaration sd, Type t, Scope* continue; } - elems[fieldi] = doCopyOrMove(sc, ex); + elems[fieldi] = doCopyOrMove(sc, ex, null, false); ++fieldi; } if (errors) diff --git a/compiler/src/dmd/s2ir.d b/compiler/src/dmd/s2ir.d index ac575dd5b1..f8ec0d836e 100644 --- a/compiler/src/dmd/s2ir.d +++ b/compiler/src/dmd/s2ir.d @@ -534,7 +534,7 @@ void Statement_toIR(Statement s, ref IRState irs, StmtState* stmtstate) void visitReturn(ReturnStatement s) { - //printf("s2ir.ReturnStatement: %s\n", s.toChars()); + //printf("s2ir.ReturnStatement: %s\n", toChars(s.exp)); BlockState *blx = irs.blx; BC bc; diff --git a/compiler/src/dmd/semantic3.d b/compiler/src/dmd/semantic3.d index 4f10982a2d..0c9c88b9ff 100644 --- a/compiler/src/dmd/semantic3.d +++ b/compiler/src/dmd/semantic3.d @@ -940,7 +940,7 @@ private extern(C++) final class Semantic3Visitor : Visitor * If NRVO is not possible, all returned lvalues should call their postblits. */ if (!funcdecl.isNRVO()) - exp = doCopyOrMove(sc2, exp, f.next); + exp = doCopyOrMove(sc2, exp, f.next, true); if (tret.hasPointers()) checkReturnEscape(*sc2, exp, false); diff --git a/compiler/src/dmd/statementsem.d b/compiler/src/dmd/statementsem.d index 8138bd2eb9..d259abf47b 100644 --- a/compiler/src/dmd/statementsem.d +++ b/compiler/src/dmd/statementsem.d @@ -2460,7 +2460,7 @@ Statement statementSemanticVisit(Statement s, Scope* sc) /* https://dlang.org/spec/statement.html#return-statement */ - //printf("ReturnStatement.dsymbolSemantic() %p, %s\n", rs, rs.toChars()); + //printf("ReturnStatement.dsymbolSemantic() %s\n", toChars(rs)); FuncDeclaration fd = sc.parent.isFuncDeclaration(); if (fd.fes) diff --git a/compiler/test/runnable/nrvo.d b/compiler/test/runnable/nrvo.d index 5c653fe45a..bceba100e0 100644 --- a/compiler/test/runnable/nrvo.d +++ b/compiler/test/runnable/nrvo.d @@ -22,9 +22,35 @@ void test1() assert(&r == s1ptr); } +/***************************************************/ +// https://github.com/dlang/dmd/issues/20567 + +struct S2 +{ + int x; + this(ref S2 s) { x = s.x; } +} + +S2 returnRval(ref S2 arg1, ref S2 arg2, int i) +{ + return i ? arg1 : arg2; +} + +void test2() +{ + S2 s1, s2; + s1.x = 3; + s2.x = 4; + S2 s = returnRval(s1, s2, 0); + assert(s.x == 4); + s = returnRval(s1, s2, 1); + assert(s.x == 3); +} + /***************************************************/ void main() { test1(); + test2(); }