Skip to content

Translate _d_newarray{U,iT,T} to a single template#15299

Merged
RazvanN7 merged 2 commits intodlang:masterfrom
teodutu:template-_d_newarrayT
Oct 24, 2023
Merged

Translate _d_newarray{U,iT,T} to a single template#15299
RazvanN7 merged 2 commits intodlang:masterfrom
teodutu:template-_d_newarrayT

Conversation

@teodutu
Copy link
Member

@teodutu teodutu commented Jun 8, 2023

This achieves the following:

  • Convert _d_newarray{U,iT,T} to a single template _d_newarrayT that handles arrays of elements that either have an init symbol or are zero-initialised.
  • Move compiler lowering to the semantic phase
  • Store lowered expression in NewExp.lowering
  • Copy or move array utils functions as templates from rt.lifetime.d to core.internal.array.utils.d. Some cannot be removed from rt.lifetime.d because they're still used by non-template hooks and it's impossible to get a type from a TypeInfo argument at compile time as required by templates.

This is still a draft because some tests are failing locally and I don't see how they're related to my changes. I just wanted to see if they also fail when running the tests remotely.

@dlang-bot
Copy link
Contributor

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

@teodutu teodutu force-pushed the template-_d_newarrayT branch 2 times, most recently from 2b695a7 to 6cf7bf2 Compare June 8, 2023 09:25
@teodutu teodutu force-pushed the template-_d_newarrayT branch 2 times, most recently from cf561a3 to bde3a7f Compare June 20, 2023 09:52
@teodutu teodutu force-pushed the template-_d_newarrayT branch from bde3a7f to 9881fb9 Compare July 14, 2023 08:54
@ibuclaw
Copy link
Member

ibuclaw commented Jul 14, 2023

Is OVERRIDE_MEMALLOC being considered here? I notice that newitem and newclass already seem to be dead. Potentially at the cost of compile-time.

@teodutu teodutu force-pushed the template-_d_newarrayT branch from 9881fb9 to 04e8b5a Compare July 15, 2023 04:02
@teodutu teodutu force-pushed the template-_d_newarrayT branch from 04e8b5a to 5eaf2ea Compare July 19, 2023 07:07
@teodutu teodutu force-pushed the template-_d_newarrayT branch 3 times, most recently from abfc554 to b3ddd1f Compare July 19, 2023 09:06
@teodutu
Copy link
Member Author

teodutu commented Jul 19, 2023

OVERRIDE_MEMALLOC

@ibuclaw, I will take care of _d_newclass and _d_newitemT in the future but don't see any _d_newarray* hook defined under OVERRIDE_MALLOC here [1]. Is there any other place?

[1]

static if (OVERRIDE_MEMALLOC)
{
// Override the host druntime allocation functions in order to use the bump-
// pointer allocation scheme (`allocmemoryNoFree()` above) if the GC is disabled.
// That scheme is faster and comes with less memory overhead than using a
// disabled GC alone.
extern (C) void* _d_allocmemory(size_t m_size) nothrow
{
return allocmemory(m_size);
}
private void* allocClass(const ClassInfo ci) nothrow pure
{
alias BlkAttr = GC.BlkAttr;
assert(!(ci.m_flags & TypeInfo_Class.ClassFlags.isCOMclass));
BlkAttr attr = BlkAttr.NONE;
if (ci.m_flags & TypeInfo_Class.ClassFlags.hasDtor
&& !(ci.m_flags & TypeInfo_Class.ClassFlags.isCPPclass))
attr |= BlkAttr.FINALIZE;
if (ci.m_flags & TypeInfo_Class.ClassFlags.noPointers)
attr |= BlkAttr.NO_SCAN;
return GC.malloc(ci.initializer.length, attr, ci);
}
extern (C) void* _d_newitemU(const TypeInfo ti) nothrow;
extern (C) Object _d_newclass(const ClassInfo ci) nothrow
{
const initializer = ci.initializer;
auto p = mem.isGCEnabled ? allocClass(ci) : allocmemoryNoFree(initializer.length);
memcpy(p, initializer.ptr, initializer.length);
return cast(Object) p;
}
version (LDC)
{
extern (C) Object _d_allocclass(const ClassInfo ci) nothrow
{
if (mem.isGCEnabled)
return cast(Object) allocClass(ci);
return cast(Object) allocmemoryNoFree(ci.initializer.length);
}
}
extern (C) void* _d_newitemT(TypeInfo ti) nothrow
{
auto p = mem.isGCEnabled ? _d_newitemU(ti) : allocmemoryNoFree(ti.tsize);
memset(p, 0, ti.tsize);
return p;
}
extern (C) void* _d_newitemiT(TypeInfo ti) nothrow
{
auto p = mem.isGCEnabled ? _d_newitemU(ti) : allocmemoryNoFree(ti.tsize);
const initializer = ti.initializer;
memcpy(p, initializer.ptr, initializer.length);
return p;
}
// TypeInfo.initializer for compilers older than 2.070
static if(!__traits(hasMember, TypeInfo, "initializer"))
private const(void[]) initializer(T : TypeInfo)(const T t)
nothrow pure @safe @nogc
{
return t.init;
}
}

@teodutu
Copy link
Member Author

teodutu commented Jul 19, 2023

So far the DMD and phobos tests are passing. core/internal/array/duplication.d calls _d_newarrayU. When I tried to split _d_newarrayT into _d_newarrayU that only allocates memory and _d_newarrayT that initialises it, I broke some stuff in the GC. That's why there's commented code added in the PR. I'll try to fix these errors before converting to a full PR.

The builkite failure seems unrelated. I tested it locally and it gives the same error when using either dmd-2.104.1 or my branch. Other completely different PRs are also failing this test, so I think it has nothing to do with these changes.

@teodutu teodutu force-pushed the template-_d_newarrayT branch from b3ddd1f to f45431d Compare July 19, 2023 10:07
@teodutu teodutu force-pushed the template-_d_newarrayT branch 3 times, most recently from 8455979 to 3162060 Compare August 2, 2023 09:20
@teodutu teodutu force-pushed the template-_d_newarrayT branch 7 times, most recently from faaa8f7 to f222386 Compare September 24, 2023 21:59
@teodutu teodutu marked this pull request as ready for review September 25, 2023 06:59
@teodutu teodutu requested a review from ibuclaw as a code owner September 25, 2023 06:59
@teodutu
Copy link
Member Author

teodutu commented Sep 25, 2023

dup() still uses the old hook [1]. I'll leave those for a later PR as this one is already large enough as it is. In addition, getting this merged will allow me to start working on _d_arraym{iT,T}X as well [2].

[1]

private extern (C) void[] _d_newarrayU(const scope TypeInfo ti, size_t length) pure nothrow;

[2] https://github.com/dlang/dmd/blob/396fc8d477ca66e20f6b4fec4d532f2aee053ba3/druntime/src/rt/lifetime.d#L1135C19-L1158

@teodutu teodutu force-pushed the template-_d_newarrayT branch from f222386 to df896e1 Compare October 17, 2023 18:34
This copies `__setArrayAllocLength()`, `__arrayAlloc()` and moves
`__arrayStart()` and `__arrayClearPad()` to
`core.internal.array.utils.d`. This is needed because `_d_newarrayT()`
calls these functions from `rt.lifetime.d`, but the file cannot be
imported from `core.internal.array.creation.d`.

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
This achieves the following:
- Convert `_d_newarray{U,iT,T}` to a single template `_d_newarrayT` that
handles arrays of elements that either have an init symbol or are
zero-initialised.
- Move compiler lowering to the semantic phase
- Store lowered expression in `NewExp.lowering`

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
@teodutu teodutu force-pushed the template-_d_newarrayT branch from df896e1 to 00c1822 Compare October 21, 2023 09:51
@RazvanN7 RazvanN7 merged commit fcff1b5 into dlang:master Oct 24, 2023
@RazvanN7
Copy link
Contributor

RazvanN7 commented Oct 24, 2023

Up until this point we have not put into changelogs the templatization of druntime hooks. However, if people find it useful we could add it. @WalterBright @ibuclaw what do you think?

I, personally, don't see any harm in putting such info in the changelog, except that it might be more verbose with no benefit for the majority of the readers (having a custom runtime should be a niche use case).

@teodutu
Copy link
Member Author

teodutu commented Oct 24, 2023

@ryuukk, @RazvanN7, I created PR #15729 with a changelog entry about this. We can discuss this further there.

@schveiguy
Copy link
Member

@RazvanN7 Having a changelog helps knowing what you have to do for each version of the compiler you want to support with your custom runtime.

nybzmr added a commit to nybzmr/dmd that referenced this pull request Mar 19, 2025
…RRAYIT, TRACENEWARRAYT and TRACENEWARRAYIT
thewilsonator pushed a commit that referenced this pull request Mar 19, 2025
* 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 added a commit to teodutu/dmd that referenced this pull request Jul 11, 2025
PR dlang#15299 added the template `_d_newarrayU`, but not its corresponding
trace-GC hook. This commit adds the latter as a wrapper on top of
`_d_newarrayUPureNothrow` so `_d_newarrayUTrace` can be called from such
contexts

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
teodutu added a commit to teodutu/dmd that referenced this pull request Jul 11, 2025
PR dlang#15299 added the template `_d_newarrayU`, but not its corresponding
trace-GC hook. This commit adds the latter as a wrapper on top of
`_d_newarrayUPureNothrow` so `_d_newarrayUTrace` can be called from such
contexts

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
teodutu added a commit to teodutu/dmd that referenced this pull request Jul 11, 2025
PR dlang#15299 added the template `_d_newarrayU`, but not its corresponding
trace-GC hook. This commit adds the latter as a wrapper on top of
`_d_newarrayUPureNothrow` so `_d_newarrayUTrace` can be called from such
contexts

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
teodutu added a commit to teodutu/dmd that referenced this pull request Jul 11, 2025
PR dlang#15299 added the template `_d_newarrayU`, but not its corresponding
trace-GC hook `_d_newarrayUTrace`. This commit adds the latter as a wrapper
on top of `_d_newarrayUPureNothrow` so `_d_newarrayUTrace` can be called
from such contexts.

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
teodutu added a commit to teodutu/dmd that referenced this pull request Jul 11, 2025
PR dlang#15299 added the template `_d_newarrayU`, but not its corresponding
trace-GC hook `_d_newarrayUTrace`. This commit adds the latter as a wrapper
on top of `_d_newarrayUPureNothrow` so `_d_newarrayUTrace` can be called
from such contexts.

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
thewilsonator pushed a commit that referenced this pull request Jul 12, 2025
)

PR #15299 added the template `_d_newarrayU`, but not its corresponding
trace-GC hook `_d_newarrayUTrace`. This commit adds the latter as a wrapper
on top of `_d_newarrayUPureNothrow` so `_d_newarrayUTrace` can be called
from such contexts.

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
@schveiguy
Copy link
Member

This PR introduced a regression #22517

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

5 participants