Translate `_d_arrayappend{T,cTX}` to templates by teodutu · Pull Request #13495 · dlang/dmd

teodutu · 2022-01-06T13:20:03Z

This PR lowers expressions such as a ~= b to:

_d_arrayappendcTX(a, 1), a[a.length - 1] = b, a;
// Note that the assignment above is actually a construction.

when b is a single element, or

_d_arrayappendT(a, b);

when b is an array. These lowerings are now moved to the compiler's frontend, as opposed to e2ir.d.

This revives #9982.

dlang-bot · 2022-01-06T13:20:06Z

Thanks for your pull request and interest in making D better, @teodutu! 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#13495"

ibuclaw · 2022-01-06T13:36:04Z

src/dmd/e2ir.d

+        if (auto ve = ce.econd.isVarExp)
+            if (ve.var.ident == Id.ctfe)
+            {
+                elem *e = toElem(ce.e2, irs);
+                if (irs.params.cov && ce.e2.loc.linnum)
+                    e = el_combine(incUsageElem(irs, ce.e2.loc), e);
+                if (tybasic(e.Ety) == TYstruct)
+                    e.ET = Type_toCtype(ce.e2.type);
+                elem_setLoc(e, ce.loc);
+                result = e;
+                return;
+            }


This doesn't look relevant to the change, and the backend should already optimize this case.

If I don't do this here, the true branch of the cond reaches visit(CatExp), which is triggers these asserts [1] [2]. In addition, without those asserts, it doesn't make sense to lower the CatExp twice: in expressionsem.d and here.

[1]

dmd/src/dmd/e2ir.d

Line 2717 in 3f1b2c4

assert(0, "This case should have been rewritten to `_d_arrayappendT` in the semantic phase");

[2]

dmd/src/dmd/e2ir.d

Line 2722 in 3f1b2c4

assert(0, "This case should have been rewritten to `_d_arrayappendcTX` in the semantic phase");

src/dmd/expression.d

ibuclaw · 2022-01-06T13:50:03Z

src/dmd/s2ir.d

+         * This is necessary for expressions such as `a ~= b`, where `b` is not
+         * an array. In expressionsem.d, they are lowered to:
+         * `__ctfe ? a ~= b : _d_arrayappendcTX(a, 1), a[$ - 1] = b, a;`
+         * If CTFE hasn't already interpreted `a ~= b`, then this branch must be
+         * removed, as e2ir.d no longer supports the runtime hook to
+         * `_d_arrayappendcTX`.


Can't you just remove it during semantic?

I'm not sure I understand what you mean. I create the lowering during semantic3 and the lowering is needed for CTFE. I don't see where in semantic i should remove it.

Sounds like something has gone wrong to me. Lowering should only occur after CTFE has had a chance to reduce the code, not before.

The problem is that when the compiler analyzes the body of a function it cannot know if the function is going to be interpreted or not in the future. Since we cannot interpret _d_arrayappend this trick with __ctfe is used.

Performing the lowering in e2ir.d, where it previously was, has the following disadvantages:

Maintainability decreases as LDC and GDC will have to implement the lowering themselves.

Readability also decreases, because the code in e2ir.d is less expressive than that in expressionsem.d.

While it is true that lowering the CatAssign as early as expressionsem.d makes the logic more complex, requiring that e2ir.d and s2ir.d omit the true branch of the __ctfe cond, I believe this approach is preferable because it elegantly solves the problems above, while also clearly separating the expression intended for CTFE from those meant for code generation.

I believe this approach is preferable because it elegantly solves the problems above, while also clearly separating the expression intended for CTFE from those meant for code generation.

No it doesn't, you're adding semantic logic to the code generator. The code generator should at best be just a trivial translation from one representation to another.

What I see is that you've added some new assert(0) branches in the code generator. This means that the front-end should never leak these AST constructs to the back-end, and yet, that is exactly what the lowering does, so the code generator is becomes even more complex than it needs to be by suddenly having a requirement to filter AST nodes that should have not have reached it in the first place.

Hmm, maybe we can make the hook interpret-able. That would get rid of the filtering code in e2ir.d and would still enable us to perform the lowering in the frontend. Is that an acceptable solution @ibuclaw ?

radcapricorn · 2022-01-06T17:16:16Z

This PR lowers expressions such as a ~= b to:
__ctfe ? _d_arrayappendcTX(a, 1), a[a.length - 1] = b, a;
when b is a single element

I don't think that is a correct lowering. a ~= b should be appending a copy of b to a when b is lvalue or emplacing b at the end of a when b is rvalue. As worded, however, this looks like assignment of b to a (default-initialized?) newly added element of a, which may not even be legal (i.e. disabled default constructor). What's the actual semantics here?

teodutu · 2022-01-06T18:32:29Z

This PR lowers expressions such as a ~= b to:
__ctfe ? _d_arrayappendcTX(a, 1), a[a.length - 1] = b, a;
when b is a single element
I don't think that is a correct lowering. a ~= b should be appending a copy of b to a when b is lvalue or emplacing b at the end of a when b is rvalue. As worded, however, this looks like assignment of b to a (default-initialized?) newly added element of a, which may not even be legal (i.e. disabled default constructor). What's the actual semantics here?

First, thanks for pointing out to me that I had forgotten the true branch of the cond. The correct lowering should be:

__ctfe ? a ~= b : _d_arrayappendcTX(a, 1), a[a.length - 1] = b, a;

I've also updated the PR's description.

Now to answer your question, the lowering creates a ConstructExp for the assignment [1], which, differentiates between lvalue and rvalue rhs's in e2ir.d.

[1]

dmd/src/dmd/expressionsem.d

Line 10292 in 92d4630

auto ae = new ConstructExp(exp.loc, elem, value2);

radcapricorn · 2022-01-06T18:46:26Z

Could you then reword the description please? Because _d_arrayappendcTX(a, 1), a[a.length - 1] = b still reads "grow array, assign at end", not "construct at end".

teodutu · 2022-01-06T18:52:07Z

Done @radcapricorn. Is the description clearer now? I'd prefer to keep the code itself as it is, because that's more or less what you get if you print result.toChars(), after running semantic on the lowering.

ibuclaw · 2022-01-06T19:19:25Z

src/dmd/dinterpret.d

+            else if (fd.ident == Id._d_arrayappendT)
+                assert(0, "CTFE cannot interpret _d_arrayappendT!");
+            else if (fd.ident == Id._d_arrayappendcTX)
+                assert(0, "CTFE cannot interpret _d_arrayappendcTX!");


Having another look over, if I understand this function right, you should instead implement these two paths because CTFE can interpret arrayappend operations. Then there's no need for the (__ctfe) ? a : b set-up you've done in semantic, just generate the lowering to template directly.

If the lowering reaches CTFE, this error [1] is triggered because of the mixin in the hook's body [2]. That's why those asserts are here and that's why I use __ctfe ? ....

[1]

dmd/src/dmd/dinterpret.d

Lines 4967 to 4972 in b14861d

if (!fd.fbody)

{

e.error("`%s` cannot be interpreted at compile time, because it has no available source code", fd.toChars());

result = CTFEExp.showcontext;

return;

}

[2] https://github.com/dlang/druntime/blob/cc101972b1f6d3047ca7b54cbf4aee11ac499b75/src/core/internal/array/appending.d#L42

teodutu · 2022-04-21T19:25:57Z

In order to pass the tests, this PR requires an updated implementation of _d_arrayappendT: dlang/druntime#3805.

src/dmd/dinterpret.d

src/dmd/e2ir.d

src/dmd/expression.d

src/dmd/expressionsem.d

src/dmd/s2ir.d

test/compilable/extra-files/vcg-ast.d.cg

RazvanN7 · 2022-04-25T09:06:56Z

@ibuclaw could you please re-review this?

@dkorpel Please take a look at this when you have the time.

dkorpel

I've put a few nits, but my bigger issue is with the size. The added code is substantially more complex than the removed code. Especially seeing how the hook functions get rewritten back to CatAssignExp for CTFE looks clumsy. I feel we can do better than this, but unfortunately I can't give an insightful direction without delving into the project myself, so at this moment I'm abstaining.

I'm curious about the following things:

How many more hooks are there to be translated to templates?
Do those require PRs of similar complexity?
Are there 'clean up' PRs planned after this?

Maybe this was all planned out somewhere, but I can't find the document. (I'm looking at dlang/project-ideas#25)

src/dmd/expression.d

src/dmd/dinterpret.d

src/dmd/expressionsem.d

RazvanN7 · 2022-04-25T14:23:45Z

@dkorpel There really isn't anything too complex happening here. The extra lines that you are seeing are added because:

We need to redo the expression for interpretation in case of ctfe. This is done for all other hooks.
Constructing a callExp in the frontend is more verbose than doing it in the gluelayer because the function/field names are much more descriptive and therefore more verbose.

In addition, some code from previous commits is obsolete and therefore will be deleted so the size will get smaller.

The idea here is that we might add a bit of complexity but, in general, it will worth it.

RazvanN7 · 2022-04-25T14:25:32Z

How many more hooks are there to be translated to templates?

@teodutu can provide the exact count, but I thing there are still somewhere between 10-15 hooks that need to be translated.

Do those require PRs of similar complexity?

No, we expect that this one is the most complex one. However, we have no way of knowing until we look at a given hook.

Are there 'clean up' PRs planned after this?

What do you mean? Anyone is welcome to improve the code is possible.

dkorpel · 2022-04-25T14:48:03Z

There really isn't anything too complex happening here. The extra lines that you are seeing are added because:

We need to redo the expression for interpretation in case of ctfe. This is done for all other hooks.

Can't you interpret the hook? There's an explicit check, but it asserts:

else if (fd.ident == Id._d_arrayappendcTX)
    assert(0, "CTFE cannot interpret _d_arrayappendcTX!");

RazvanN7 · 2022-04-25T14:49:12Z

No, the hook ends up calling C code.

RazvanN7 · 2022-05-06T08:12:26Z

You need a rebase to latest master to get rid of the FreeBSD failure. Also, mind the other nits in my review.

RazvanN7 · 2022-05-06T08:16:27Z

@dkorpel @ibuclaw please re-review.

Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>

dkorpel

Are there no existing tests for global.params.tracegc?

src/dmd/dinterpret.d

teodutu · 2022-05-06T19:49:29Z

@dkorpel There are tests for global.params.tracegc in DRuntime. One such test is in druntime/test/profile/src/profilegc.d.

Lower `a ~= b` to: - `_d_arrayappendT(a, b)` when `b` is an array - `_d_arrayappendcTX(a, 1), a[a.length - 1] = b, a` when `b` is an element Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

dkorpel · 2022-05-06T19:51:44Z

One such test is in druntime/test/profile/src/profilegc.d.

Good, so the codecov warnings are false positives.

addressed

* Lower array appending to _d_arrayappend{T,cTX} Signed-off-by: Dan Printzell <xwildn00bx@gmail.com> * Don't allow _d_arrayappend{T,cTX} to be called in CTFE Signed-off-by: Dan Printzell <xwildn00bx@gmail.com> * Implement check for _d_arrayappend{T,cTX} in nogc.d Signed-off-by: Dan Printzell <xwildn00bx@gmail.com> * e2ir.d: If symbol is already added to symtab, don't call symbol_add Signed-off-by: Dan Printzell <xwildn00bx@gmail.com> * e2ir.d: Only compile `e2` of CondExp if `econd` is `__ctfe` Signed-off-by: Dan Printzell <xwildn00bx@gmail.com> * Remove backend code for _d_arrayappend{T,cTX} Signed-off-by: Dan Printzell <xwildn00bx@gmail.com> * Change lowering of `CatAssignExp` Lower `a ~= b` to: - `_d_arrayappendT(a, b)` when `b` is an array - `_d_arrayappendcTX(a, 1), a[a.length - 1] = b, a` when `b` is an element Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com> Co-authored-by: Dan Printzell <xwildn00bx@gmail.com>

These hooks have been converted to templates and their lowerings changed to use those templates: - dlang/dmd#13116 - dlang/dmd#13495 - dlang/dmd#13398 Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

CyberShadow · 2023-06-04T18:44:38Z

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=23965

…RRAYAPPENDCTX, TRACEARRAYAPPENDT and TRACEARRAYAPPENDCTX

* Remove RTLSYM for Translation PR #15819: Removed NEWARRAYMITX, NEWARRAYMITX, TRACENEWARRAYMTX and TRACENEWARRAYMITX * Remove RTLSYM for Translation PR #15299: Removed NEWARRAYT, NEWARRAYIT, TRACENEWARRAYT and TRACENEWARRAYIT * Remove RTLSYM for Translation PR #14837: Removed NEWCLASS, TRACENEWCLASS * Remove RTLSYM for Translation PR #14664: Removed NEWITEMT, NEWITEMIT, TRACENEWITEMT and TRACENEWITEMIT * Remove RTLSYM for Translation PR #14550: Removed ARRAYCATNTX, ARRAYCATT, TRACEARRAYCATNTX and TRACEARRAYCATT * Remove RTLSYM for Translation PR #14382: Removed ARRAYSETASSIGN * Remove RTLSYM for Translation PR #14310: Removed ARRAYASSIGN * Remove RTLSYM for Translation PR #13495: Removed ARRAYAPPENDT, ARRAYAPPENDCTX, TRACEARRAYAPPENDT and TRACEARRAYAPPENDCTX

teodutu requested a review from ibuclaw as a code owner January 6, 2022 13:20

teodutu mentioned this pull request Jan 6, 2022

Allow _d_arrayappendT to call impure functions dlang/druntime#3662

Closed

ibuclaw previously requested changes Jan 6, 2022

View reviewed changes

ibuclaw reviewed Jan 6, 2022

View reviewed changes

dlang-bot added Review:Needs Rebase Review:Needs Work labels Feb 17, 2022

dlang-bot added the Review:stalled label Apr 10, 2022

teodutu mentioned this pull request Apr 21, 2022

Rework _d_arrayappendT to be a template function dlang/druntime#3805

Merged

teodutu force-pushed the convert-_d_arrayappend-hooks branch from 92d4630 to aa50741 Compare April 21, 2022 19:22

dlang-bot removed Review:Needs Work Review:Needs Rebase Review:stalled labels Apr 21, 2022

teodutu force-pushed the convert-_d_arrayappend-hooks branch from aa50741 to ce4be68 Compare April 22, 2022 06:48

teodutu mentioned this pull request Apr 22, 2022

Add explicit _d_arrayappendTTrace hook dlang/druntime#3807

Merged

teodutu force-pushed the convert-_d_arrayappend-hooks branch 3 times, most recently from 6f0bf28 to 98439b5 Compare April 23, 2022 22:02

RazvanN7 requested changes Apr 25, 2022

View reviewed changes

dkorpel reviewed Apr 25, 2022

View reviewed changes

src/dmd/expression.d Outdated Show resolved Hide resolved

src/dmd/dinterpret.d Show resolved Hide resolved

src/dmd/dinterpret.d Show resolved Hide resolved

src/dmd/expressionsem.d Outdated Show resolved Hide resolved

Vild added 6 commits May 6, 2022 19:08

Lower array appending to _d_arrayappend{T,cTX}

3c38bf2

Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>

Don't allow _d_arrayappend{T,cTX} to be called in CTFE

95d6012

Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>

Implement check for _d_arrayappend{T,cTX} in nogc.d

9efb4cd

Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>

e2ir.d: If symbol is already added to symtab, don't call symbol_add

8659665

Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>

e2ir.d: Only compile e2 of CondExp if econd is __ctfe

95292c1

Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>

Remove backend code for _d_arrayappend{T,cTX}

222acb7

Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>

teodutu force-pushed the convert-_d_arrayappend-hooks branch from 1296991 to 3323ec3 Compare May 6, 2022 16:40

dkorpel reviewed May 6, 2022

View reviewed changes

dkorpel approved these changes May 6, 2022

View reviewed changes

src/dmd/dinterpret.d Show resolved Hide resolved

Change lowering of CatAssignExp

9f7cea7

Lower `a ~= b` to: - `_d_arrayappendT(a, b)` when `b` is an array - `_d_arrayappendcTX(a, 1), a[a.length - 1] = b, a` when `b` is an element Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

teodutu force-pushed the convert-_d_arrayappend-hooks branch from 3323ec3 to 9f7cea7 Compare May 6, 2022 19:51

RazvanN7 merged commit a2ebf9a into dlang:master May 9, 2022

teodutu deleted the convert-_d_arrayappend-hooks branch May 21, 2022 20:50

teodutu mentioned this pull request May 26, 2022

Remove previously templated hooks dlang/druntime#3826

Merged

dkorpel mentioned this pull request Jul 4, 2022

Fix -v output of test3004 #14268

Merged

RazvanN7 mentioned this pull request Jul 5, 2022

Fix the concat assign case of Issue 20321 in a frontend solution #10539

Closed

nybzmr added a commit to nybzmr/dmd that referenced this pull request Mar 19, 2025

Remove RTLSYM for Translation PR dlang#13495: Removed ARRAYAPPENDT, A…

0c61038

…RRAYAPPENDCTX, TRACEARRAYAPPENDT and TRACEARRAYAPPENDCTX

	if (!fd.fbody)
	{
	e.error("`%s` cannot be interpreted at compile time, because it has no available source code", fd.toChars());
	result = CTFEExp.showcontext;
	return;
	}

Uh oh!

Conversation

teodutu commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Jan 6, 2022

Bugzilla references

Testing this PR locally

Uh oh!

ibuclaw Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

teodutu Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ibuclaw Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

teodutu Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

ibuclaw Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Jan 7, 2022

Choose a reason for hiding this comment

Uh oh!

teodutu Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Jan 10, 2022

Choose a reason for hiding this comment

Uh oh!

radcapricorn commented Jan 6, 2022

Uh oh!

teodutu commented Jan 6, 2022

Uh oh!

radcapricorn commented Jan 6, 2022

Uh oh!

teodutu commented Jan 6, 2022

Uh oh!

ibuclaw Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teodutu Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

teodutu commented Apr 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RazvanN7 commented Apr 25, 2022

Uh oh!

dkorpel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RazvanN7 commented Apr 25, 2022

Uh oh!

RazvanN7 commented Apr 25, 2022

Uh oh!

teodutu commented Jan 6, 2022 •

edited

Loading

teodutu Jan 6, 2022 •

edited

Loading

teodutu Jan 7, 2022 •

edited

Loading

ibuclaw Jan 8, 2022 •

edited

Loading

ibuclaw Jan 6, 2022 •

edited

Loading