dmd icon indicating copy to clipboard operation
dmd copied to clipboard

WIP: Move last occurrence of symbols when occurring in rhs of VarExpr or if passed to a function declaration [skip ci]

Open nordlow opened this issue 3 years ago • 1 comments

This enable automatic move of last occurrence of an l-value struct instance in a VarExp when occurring

  1. as a right-hand-side VarExp of an AssignExp
  2. as a value-passed Parameter
  3. as the single VarExpr of a ReturnStatement
  4. ...

It took me and @UplinkCoder about 2-3 hours online to accomplish this. Superb cooperation.

Hopefully this will make https://github.com/dlang/phobos/pull/8537 unnecessary.

C++23 is going in that direction aswell - see https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2266r3.html.

TODOs:

  • [ ] Discuss if it's ok to add VarExp lastVarExp and bool lastVarExpCanBeMoved to VarDeclaration or if should go with a much smaller number of cases that can be handled typically
    1. return VarExp
    2. ... = VarExp; being the last statement in the scope of VarExp
  • [ ] Check for move in return VarExp _after the corresponding VarDeclaration has been resolved. Use asm { int 3; } where VarDeclaration is resolved and continue in gdb.
  • [ ] Handle ReturnStatement case as well. If scope of ReturnStatement matches that of VariableDeclaration of VarExp move is unconditional. Cases to handle are:
    • [ ] return X where X is VarExp
    • [ ] return f(Xs...) for all X in Xs being a VarExpr
    • [ ] return P ? X : Y All these cases are easiest handled with an extra state in visitor keeping track of current ReturnStatement (if any).
  • [ ] Should move structs with disabled copy constructor (moveOnDisabledPostblit in added test file movetest.d should pass)
  • [ ] Test that copy constructors are correctly avoided aswell in assignments
  • [ ] Test that structs with @disable this(this) are allowed in the three cases outline above
  • [ ] Handle if-statements and other branching structures.
  • [ ] Limit moves to when scope of VarDeclaration equals scope of VarExp.
  • [ ] Check if opPostMove needs to be taken into consideration here
  • [ ] Regenerate C++ headers
  • [ ] Update tests now that compiler optimizes avoid away more copy constructor and destructor calls and potentially inserts calls to opPostMove.
  • [ ] Adjust TODO comments added
  • [ ] Discuss if an alternatie solution avoiding extra state in VarDeclaration in favor of extra state in SemanticTimeVisitor is preferred.
  • [ ] Close https://github.com/dlang/phobos/pull/8537 as doesn't need fix
  • [ ] Add warning diagnostics about unnecessary calls to core.lifetime.move
  • [ ] Remove all other calls to move() that are no longer neccessary.

Current simple test

make -f posix.mak; generated/linux/release/64/dmd -vcg-ast -c ~/movetest.d

where movetest.d is

struct S
{
    int x;
    ~this() @safe pure nothrow @nogc
    {
        import std.stdio;
        debug writeln(__FUNCTION__);
    }
}

void moveOn(S s) @safe pure nothrow @nogc
{
    S t = s; // s is moved here and t i
}

void moveOff(S s) @safe pure nothrow @nogc
{
    S t = s; // is not moved here because
    s.x = 42; // it's reference here
}

correctly outputs

import object;
struct S
{
	int x;
	~this()
	{
	}
	alias __xdtor = ~this()
	{
	}
	;
	pure nothrow @nogc ref @trusted S opAssign(S p) return
	{
		(S __swap2 = void;) , __swap2 = this , (this = p , __swap2.~this());
		return this;
	}
}
pure nothrow @nogc @safe void moveOn(S s)
{
	S t = s;
	t.~this();
}
pure nothrow @nogc @safe void moveOff(S s)
{
	S t = s;
	s.x = 42;
	t.~this();
	s.~this();
}
RTInfo!(S)
{
	enum immutable(void)* RTInfo = null;

}
NoPointersBitmapPayload!1LU
{
	enum ulong[1] NoPointersBitmapPayload = [0LU];

}

FYI, @maxhaton @atilaneves @andralex @WalterBright.

nordlow avatar Aug 19 '22 23:08 nordlow

Thanks for your pull request and interest in making D better, @nordlow! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14377"

dlang-bot avatar Aug 19 '22 23:08 dlang-bot

Since there's been no progress on this, and the [skip ci] indicates that there's no need to see the CI output, I'll close this for now

dkorpel avatar May 02 '23 12:05 dkorpel