Translate _d_arrayappend{T,cTX} to templates

[Vild]

druntime PR needed for this: dlang/druntime#2632

• This lowers Cat{,Elem}AssignExp} to the templates object._d_arrayappend{T,cTX}.
• CTFE support is restored by rewriting CallExp, that will run the new hooks, back to Cat{,Elem}AssignExp.

[jacob-carlborg]

No tests? You can try to write a test that verifies the AST rewriting works correctly. Have a look here https://github.com/dlang/dmd/tree/master/test/unit

[jpf91]

Instead of rewriting back to CatAssign(Elem)Exp, wouldn't it be cleaner to move out the related code in interpret into evaluateCatAssign funtions, similarily to how it's been done for evaluatePostblit and evaluateDtor?

if (fd.ident == Id.__ArrayPostblit)
     result = evaluatePostblit(istate, result);
else
     result = evaluateDtor(istate, result);

[Vild]

Instead of rewriting back to CatAssign(Elem)Exp, wouldn't it be cleaner to move out the related code in interpret into evaluateCatAssign funtions, similarily to how it's been done for evaluatePostblit and evaluateDtor?

I'm not sure if/how I can do this is a way that does not duplicate code as interpretAssignCommon does a lot of checks for both the left and right expressions for assignments. I have not gone though all the logic to find what paths it can take, but from the "outside" right now, it seems like the best idea is to rewrite the expression back and let the previous code continue to run.

[Vild]

As mentioned in dlang/druntime#2632 (comment)
I'm still not 100% done with this PR. I still need to fix a postblit bug in the druntime code that also depends on lowering code, and I will also add unittest to verify that the lowering code works correctly.

[jpf91]

I'm not sure if/how I can do this is a way that does not duplicate code as interpretAssignCommon does a lot of checks for both the left and right expressions for assignments.

I see. Then let's keep the rewrite for now 👍

[dlang-bot]

Thanks for your pull request and interest in making D better, @Vild! 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 fetch digger
dub run digger -- build "master + dmd#9982"

[Vild]

This should work now, but it requires the fixes in dlang/druntime#2718

This will now lower arr ~= e to __ctfe ? arr~=e : (_d_arrayappendcTX(arr, 1), arr[$-1]=e) and arr ~= arr2 to __ctfe ? arr ~= arr2 : _d_arrayappendT(arr, arr2). This remove the need a CTFE intercept.

I've added two patches to the backend that I think is important to be reviews by someone that know the backend so I don't break anything.
The first one is 2d81b67. The lowering code of _d_arrayappendcTX triggered this assert dmd: dmd/backend/symbol.d:1170: Assertion '(*s).Ssymnum == -1' failed..
It tries to symbol_add an already added symbol, because the toSymbol have a cache with already created symbols. This if just skips the additional symbol_adds. I hope this is the correct way of fixing this.

The second fix is 1f4d9bf. This make it only compile the !__ctfe path. This is required as I added assert(0) to codegen when compiling TOK.concatenate{,Elem}Assign. Without this the assert(0) would always trigger when it reached a lowered CondExp.

Should I move these to a separate PR? It is needed for this PR, but it is also a bit off-topic.

[jpf91]

This looks like debug code which should probably removed before this is merged?

[jpf91]

If this list grows much longer, we should probably use a switch statement instead? But I guess with 4 entries this is still ok.

[jpf91]

Hmm, could we change visit(ConExp) instead to not visit __ctfe only code? I guess that right now we probably do not differentiate, but in the long run it should be possible to ignore everything in __ctfe only blocks for nogc/codegen. As you already have the patch for codegen, why not do the same for nogc checks?

And then add a Test + Changelog entry for this. Some people might be quite happy about this change: https://run.dlang.io/is/G6fv55 ;-)

[Vild]

Sadly I cannot change so it doesn't visit this function because of two reasons.

1. nogc.d is using dmd.apply.walkPostorder which walks it in this order: walk(econd), walk(e1), walk(e2), and lastly visit(CondExp). https://github.com/dlang/dmd/blob/master/src/dmd/apply.d#L144. As this walker is used all over the codebase I don't think it is a good idea to introduce changes to it.

2. It still needs to set err = true to mark the appending as non-@nogc because the lowering code will not lower code that is in a __traits(compiles, ...). Printing error here is not needed because the error will always be gagged for SCOPE.compile, and if errors were printed they would print the exact same message as the one in visit(CallExp).

So this visit function will now just silently error.

[jpf91]

Seems like a good idea to me. Needs to be in its own PR though and I guess there's already a bug report for this somewhere.

Ideally, this should not hard-detect the ctfe variable though. You code will fail on constructs such as these:

void foo(bool dummy)
{
    if (__ctfe && dummy)
    {
        // CTFE-only Code goes here
    }
}

What should be done instead is this: Set __ctfe (somehow) to false, then constant fold / interpret the condition. If the result is a constant false, the if part of the condition is a ctfe only part. If it is constant true, the else branch is a ctfe only branch. If it is not const, both branches may be used at runtime.

Maybe that's out of scope for GSoC though, but it would be very useful for betterC.

[jpf91]

I wonder how all that stuff integrates with SCOPEctfe though, maybe what should really happen is that the if block content should be set to SCOPEctfe instead and the toElem / nogc visitors should skip these scopes.

OTOH there's no scope introduced when using CondExp, so for CondExp your code here seems to be corret.

[jpf91]

ping @JinShil as IIRC your working on SCOPEctfe issues, so you may be interested in this.

[JinShil]

What is this code doing and why?

[jpf91]

It detects an __ctfe ? e1 : e2 expression, then ignores e1 and only sends e2 to the code generation stage. I guess without this, dropping e1 code occurs later in the backend. @Vild can you elaborate why exactly this is necessary?

[Vild]

Without this change the codegen would generate code for both e1 and e2, and while generating code for e1 it would trigger this assert(0) https://github.com/dlang/dmd/pull/9982/files#diff-7a340f667e49df4f91bcab621efa8521R3294

It will also only trigger on code that looks like __ctfe ? e1 : e2, as this is the only form I'm outputting inside expressionsem.d. But I probably need to extend this to atleast support if statements as the inline can rewrite CondExp to IfStatement.

[jpf91]

Without this change the codegen would generate code for both e1 and e2, and while generating code for e1 it would trigger this assert(0).

OK. But somehow the backend currently also drops the e1 branch, the code is not in the generated object file. Does this happen only as dead-code-elimination after the initial codegen stage?

[Vild]

Does this happen only as dead-code-elimination after the initial codegen stage?

I have been unable to find where in the backend were it gets removed, all I know is that it is removed.

[JinShil]

It detects an __ctfe ? e1 : e2 expression, then ignores e1 and only sends e2 to the code generation stage.

So, this is basically a workaround because we don't have version(CTFE)?

Without this change the codegen would generate code for both e1 and e2, and while generating code for e1 it would trigger this assert(0).

Why wasn't e1 lowered in the semantic phase?

[Vild]

So, this is basically a workaround because we don't have version(CTFE)?
ya

Why wasn't e1 lowered in the semantic phase?
Because e1 is the __ctfe code-path, which is the original expression.

[JinShil]

Why would the compiler attempt to lower an expression more than once?

[jpf91]

In this lowering commit:

*output = ce.expressionSemantic(sc);

on the final ConditionExpression does call expressionSemantic on e1 and e2, IIRC. As we repurpose CatAssignExp when lowering CatAssignExp ==> __ctfe ? CatAssignExp : _d_arraycat we have to be careful not to do the lowering on the newly generated CatAssignExp.

However, maybe isCTFEHook or something like that is a better term.

[JinShil]

I don't quite understand the need for these error messages. If the runtime hook causes a GC allocation, then the error message will already be emitted there. However, the error message will be in terms of the lowered code, and not the user's code.

If the purpose of this code is to make the error message more relevant to user code, then that is the case that I mentioned earlier on slack. What we could do instead is pass the file, line number, and even the user's code to the runtime hook and have the runtime hook emit a more meaningful error message, perhaps with a static assert.

Regardless, I'm ok with keeping this as is to maintain the status quo.

[jpf91]

Yes, long-term moving error generation into the hooks would be nicer*. For now I think this is fine to keep compatibility for the error messages.

EDIT: Actually, thinking about this some more, the problem below should be unrelated. I'll keep it here though, as I think it's an interesting, although unrelated, thought.
(*) Right now, main1 and main2 won't compile:

string aCTFEOnlyHelper()(string a)
{
    return "Hello" ~ a;
}

void nogcAppend(ref string r, string val) @nogc
{
}

void main1() @nogc
{
    string result;
    __ctfe ? result ~= "foo" : nogcAppend(result, "foo");
}

void main2() @nogc
{
    enum val = aCTFEOnlyHelper("foo");
}

With the changes suggested here, main1 will actually compile.

For main2, you have the problem that as soon as you instantiate aCTFEOnlyHelper, even if it's in a CTFE-only context, the code for aCTFEOnlyHelper will also be emitted to the object file, so you need the GC there. This is probably harder to fix than the main1 problem, I've filed an issue here: https://issues.dlang.org/show_bug.cgi?id=20101

(Actually, we writing the issue I had epiphany: What we want there is actually already largely available in speculative template instantiation. So it might actually not be too difficult to get the main2 case working).

[Vild]

This code was moved from visit(CatAssignExp) as a call to _d_arrayappend{T,cTX} is now what symbolizes array appending, and the message will be printed when compiling with -vgc.

[Vild]

I'm happy with the code, but it still requires this druntime PR to be merged: dlang/druntime#2718

[RazvanN7]

@teodutu is in the process of reviving this.