Translate `_d_arraycatnTX` to a template by teodutu · Pull Request #14550 · dlang/dmd

teodutu · 2022-10-12T12:59:39Z

This brings the following changes:

Improves the existing template _d_arraycatnTX, to now concatenates both arrays and single elements
Changes the lowerings to _d_arraycatT to use the new template
Moves the lowering logic to _d_arraycatnTX to expressionsem.d
Removes the old non-template _d_arraycatnTX and _d_arraycatT hooks
Moves isRuntimeHook and removeHookTraceImpl to expression.d` so they can be called from both expressionsem.d and dinterpret.d

This continues #10064.

Signed-off-by: Teodor Dutu teodor.dutu@gmail.com

dlang-bot · 2022-10-12T12:59:42Z

Thanks for your pull request, @teodutu!

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#14550"

teodutu · 2022-10-12T13:00:13Z

This is currently blocked by https://issues.dlang.org/show_bug.cgi?id=23408. This causes dmd/test/runnable/test19688.d to fail.

dkorpel · 2022-10-13T17:37:44Z

This causes dmd/test/runnable/test19688.d to fail.

I suggest changing the test case from runnable to compilable, since the original bug report was about a compiler crash.
See my comment in the other PR: #14549 (comment)

RazvanN7 · 2022-10-14T03:57:35Z

@teodutu yes, move the test to compilable and if we decide to move on in the direction of #14549 we can move it back. There is no point to stay blocked by this.

JohanEngelen · 2022-10-17T22:34:45Z

    {
        //printf("[%s] CallExp.toElem('%s') %p, %s\n", ce.loc.toChars(), ce.toChars(), ce, ce.type.toChars());
+        /* Do this check during code gen rather than semantic() because concatenation is
+         * allowed in CTFE, and cannot distinguish that in semantic().


Two major issues with this:

GDC and LDC have to implement this error check themselves (is there a testcase for it?)

-o- will not trigger this error

GDC and LDC have to implement this error check themselves (is there a testcase for it?)

This check was previously in visitCat(), so this PR isn't making things worse. And the reason it's here is explained in the comment. Do you know of a better place where to verify this?

-o- will not trigger this error

Why would it? AFICT -o- simply skips the generation of the .o file. How is this connected to string concatenation with -betterC? This check is triggered by test/fail_compilation/test18312.d.

-o- skips codegen, thus skips e2ir ?

looks like you are right that this PR does not make the situation worse.

-o- skips codegen, thus skips e2ir

And doing so also skips the rest of the backend. Therefore, the CatExp that would otherwise be leaked to the glue layer is ignored. Because code generation is omitted, the leak is silent as it doesn't cause any trouble.

RazvanN7 · 2022-11-01T08:21:35Z

@teodutu any updates on this PR?

teodutu · 2022-11-02T06:58:32Z

@RazvanN7 The buildkite failures come from building DMD. This tells me that the failures are likely coming from string concatenations that then call .ptr like this:

string res = str1 ~ str2;
foo(res.ptr);

For some reason, they didn't trigger errors before, but now they cause issues because res.ptr is not null-terminated. Debugging this is tedious because I can't reproduce the issue locally. Same for the failure on macOS 12

RazvanN7

Looks good to me, although I don't understand why now we have more allocations for the profilegc tests.

RazvanN7 · 2022-11-28T15:01:46Z

@JohanEngelen @ibuclaw @dkorpel could you please take a look at this?

teodutu · 2023-04-14T10:06:24Z

@JohanEngelen @ibuclaw @dkorpel I've fixed the bugs created by moving the lowering to its own field inside CatExp. Can you take another look at this, please? The macOS 13 failures seem unrelated.

RazvanN7 · 2023-04-14T10:16:49Z

@@ -1725,50 +1725,10 @@ elem* toElem(Expression e, IRState *irs)
            return el_long(TYint, 0);
        }



The above betterC check could be probably moved to the frontend. You could check sc.func.needsCodegen, however, that is a topic for a different PR.

teodutu · 2023-04-15T06:51:51Z

@RazvanN7 @JohanEngelen, I have addressed your comments about test23710.d by adding STC.exptemp to the arrayliteral_on_stack that was created. Please have another look.

teodutu · 2023-04-15T06:54:00Z

I have also enclosed _d_arraycatnTX within version (D_ProfileGC) as per #14914.

RazvanN7

LGTM. cc @JohanEngelen any more comments?

RazvanN7 · 2023-04-24T10:13:11Z

@dkorpel @JohanEngelen are there any other concerns?

This brings the following changes: - Improves the existing template `_d_arraycatnTX`, to now concatenates both arrays and single elements - Changes the lowerings to `_d_arraycatT` to use the new template - Moves the lowering logic to `_d_arraycatnTX` to expressionsem.d - Adds a new field to `CatExp` called `lowering` to store the template lowering - Removes the old non-template `_d_arraycatnTX` and `_d_arraycatT` hooks - Moves `test19688.d` from `runnable/` to `compilable/` until https://issues.dlang.org/show_bug.cgi?id=23408 is fixed Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

JohanEngelen · 2023-04-24T14:00:22Z

I lack the knowledge to review in a lot of detail. Overall I am worried about template bloat due to these templatizations, so I'd appreciate if you can add an open issue about trying to implement it in such a way that reduces this bloat (e.g. have the template forward to a function that is the same for POD structs of the same size, etc.)

RazvanN7 · 2023-04-28T07:33:23Z

@JohanEngelen The follow-up project for @teodutu after he has finished templating the existing hooks is to implement optimizations for them.

dkorpel · 2023-04-28T10:21:43Z

This PR broke frontend.h: #15141

FeepingCreature · 2023-06-27T08:47:51Z

This PR caused a regression: #24018

ghost · 2023-08-09T14:49:07Z

And unfortunately #24078

tim-dlang · 2024-02-09T17:15:56Z

There are two other regressions related to this:
https://issues.dlang.org/show_bug.cgi?id=24371
https://issues.dlang.org/show_bug.cgi?id=24378

…RAYCATT, TRACEARRAYCATNTX and TRACEARRAYCATT

* 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 October 12, 2022 12:59

teodutu force-pushed the template-_d_arraycatnTX branch from 6d77157 to b544112 Compare October 12, 2022 15:23

dkorpel mentioned this pull request Oct 13, 2022

Fix Issues 23408 and 23403 - __FUNCTION__ does not resolve properly #14549

Closed

teodutu force-pushed the template-_d_arraycatnTX branch 4 times, most recently from 53909d1 to 39bb37a Compare October 15, 2022 16:39

JohanEngelen reviewed Oct 17, 2022

View reviewed changes

teodutu force-pushed the template-_d_arraycatnTX branch 2 times, most recently from eafbf95 to 98442f1 Compare October 21, 2022 19:38

dlang-bot added Review:Needs Rebase Review:Needs Work labels Nov 4, 2022

teodutu force-pushed the template-_d_arraycatnTX branch from 98442f1 to 4642c29 Compare November 25, 2022 11:21

dlang-bot removed Review:Needs Work Review:Needs Rebase labels Nov 25, 2022

teodutu force-pushed the template-_d_arraycatnTX branch 4 times, most recently from bff1692 to eb525aa Compare November 25, 2022 20:18

RazvanN7 approved these changes Nov 28, 2022

View reviewed changes

teodutu force-pushed the template-_d_arraycatnTX branch 2 times, most recently from 4db8529 to f04d44b Compare November 29, 2022 11:45

ibuclaw reviewed Nov 29, 2022

View reviewed changes

Comment thread compiler/src/dmd/dinterpret.d Outdated

teodutu force-pushed the template-_d_arraycatnTX branch from 2f651be to 29cd668 Compare April 14, 2023 09:46

RazvanN7 reviewed Apr 14, 2023

View reviewed changes

teodutu force-pushed the template-_d_arraycatnTX branch from 29cd668 to 66b4d9c Compare April 14, 2023 10:30

JohanEngelen requested changes Apr 14, 2023

View reviewed changes

Comment thread compiler/test/runnable/test23710.d Outdated

teodutu force-pushed the template-_d_arraycatnTX branch from 66b4d9c to c525392 Compare April 15, 2023 06:47

teodutu requested review from JohanEngelen and RazvanN7 April 15, 2023 06:54

RazvanN7 approved these changes Apr 17, 2023

View reviewed changes

dkorpel requested changes Apr 17, 2023

View reviewed changes

Comment thread druntime/src/core/internal/array/concatenation.d Outdated

teodutu force-pushed the template-_d_arraycatnTX branch from c525392 to 9601ea8 Compare April 19, 2023 09:39

teodutu force-pushed the template-_d_arraycatnTX branch from 9601ea8 to 3c9bea4 Compare April 24, 2023 10:25

dkorpel approved these changes Apr 24, 2023

View reviewed changes

RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Apr 24, 2023

JohanEngelen reviewed Apr 24, 2023

View reviewed changes

Comment thread compiler/test/compilable/test19688.d

RazvanN7 merged commit 1db1ba8 into dlang:master Apr 28, 2023

dkorpel mentioned this pull request Apr 28, 2023

Fix frontend.h #15141

Merged

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

Remove RTLSYM for Translation PR dlang#14550: Removed ARRAYCATNTX, AR…

4de3551

…RAYCATT, TRACEARRAYCATNTX and TRACEARRAYCATT

		@@ -1725,50 +1725,10 @@ elem* toElem(Expression e, IRState *irs)
		return el_long(TYint, 0);
		}

Uh oh!

Conversation

teodutu commented Oct 12, 2022

Uh oh!

dlang-bot commented Oct 12, 2022

Bugzilla references

Testing this PR locally

Uh oh!

teodutu commented Oct 12, 2022

Uh oh!

dkorpel commented Oct 13, 2022

Uh oh!

RazvanN7 commented Oct 14, 2022

Uh oh!

JohanEngelen Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

teodutu Oct 21, 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!

JohanEngelen Oct 22, 2022

Choose a reason for hiding this comment

Uh oh!

JohanEngelen Oct 22, 2022

Choose a reason for hiding this comment

Uh oh!

teodutu Oct 22, 2022

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Nov 1, 2022

Uh oh!

teodutu commented Nov 2, 2022

Uh oh!

RazvanN7 left a comment

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Nov 28, 2022

Uh oh!

Uh oh!

teodutu commented Apr 14, 2023

Uh oh!

Uh oh!

RazvanN7 Apr 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

teodutu commented Apr 15, 2023

Uh oh!

teodutu commented Apr 15, 2023

Uh oh!

RazvanN7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RazvanN7 commented Apr 24, 2023

Uh oh!

Uh oh!

JohanEngelen commented Apr 24, 2023

Uh oh!

RazvanN7 commented Apr 28, 2023

Uh oh!

dkorpel commented Apr 28, 2023

Uh oh!

FeepingCreature commented Jun 27, 2023

Uh oh!

teodutu Oct 21, 2022 •

edited

Loading