Replace call to runtime hook _d_arraycast with call to template object.__ArrayCast - Take 2

[JinShil]

An attempt to finish up #8531 and set an example for a GSoC project.

This is a WIP. CTFE is currently running out of memory on my machine (6GB RAM) when importing std.uni. I want to see how it does on the test suite.

[dlang-bot]

Thanks for your pull request and interest in making D better, @JinShil! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
• 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#9516"

[JinShil]

Ok, that gave me a little more information:

../druntime/import/object.d(4793): Error: reinterpreting cast from `void[]` to `Array*` is not supported in CTFE

Anyone have any ideas?

[thewilsonator]

cc @UplinkCoder

[UplinkCoder]

This is deliberate. CTFE cannot assume data-layout at runtime and at ctfe will be the same.

[jacob-carlborg]

Tests?

[JinShil]

Tests?

This is just replacing the runtime hook _d_arraycast with the template object.__ArrayCast. There should be no change in functionality or behavior, though there may be changes in performance. I'm assuming the array cast feature is already sufficiently tested, so no additional tests should be required; it should only do no harm. object.__ArrayCast has unittests to verify its implementation.

If this is merged, a followup PR will be made to druntime to remove the _d_arraycast runtime hook.

This does have the potential side effect of making the array cast feature available to betterC. However, trying to tackle that simultaneously with just getting this working is taking on a little too much at a time. I'm changing my strategy to just do a direct translation of the runtime hook, modify DMD to call it, and then follow up with additional PRs to make the feature work in betterC. At that time, I can add the betterC tests.

[jacob-carlborg]

We had problems in the past with incorrect runtime hooks being called #9481. But I guess once _d_arraycast and all the tests pass it's good.

[JinShil]

OK, I'm pretty happy with this, and I've increased my understanding of how all of this works.

Please understand the scope of the PR. We're trying to replace the runtime hooks with templates. Please see https://forum.dlang.org/post/[email protected] for justification.

I can see now that we could actually improve object.__ArrayCast to handle more casts, even at compile-time, but object.__ArrayCast was created only to replace _d_arraycast. Additional PRs can be created to expand on this work once this infrastructure is in place.

Once this is merged I'll submit a PR to druntime to remove _d_arraycast as it should no longer be needed.

This is ready to go, assuming it passes the test suite.

[JinShil]

Sorry for the interruption. I just realized that since I added the exp.e1.op != TOK.arrayLiteral condition the check for typeof(null)[] is no longer required...I think. Let's see what the test suite says.

[thewilsonator]

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

[ghost]

This commit causes a new ICE : https://issues.dlang.org/show_bug.cgi?id=19954

[ghost]

This new ICE happens because during semantic toSize and fromSize are both equal to 1 so the path taken is to not write the __ArrayCast. But later, ruring IR, tsize value has changed to 16 so an __ArrayCast is expected.

[JinShil]

But later, ruring IR, tsize value has changed to 16 so an __ArrayCast is expected.

Why is the size changed to 16?

[ghost]

I don't know. 16 is the size of an array as a fat pointer but nextof in both case is well used...

Look at the new test case. Something is seriously broken with string literals (array literals containing char literal don't exhibit the ICE).

[WalterBright]

@JinShil please revert this until you find the problem.

[JinShil]

@WalterBright, it has already been released and reverting it would also require reverting the druntime PR that accompanied it. Please give me a couple of days to figure it out.

[ghost]

Are you on the fix @JinShil ? I said earlier that I would have a fix but actually I don't. There are several ways to fix the two new test cases but then either

• template9.d and foreach4.d from the test suite break
• phobos regex engine don't compile (b/c of CTFE)
• etc, depending on the way to fix.

So I give up. You'll probably handle this in a smarter way.

[JinShil]

Are you on the fix @JinShil ?

I'm working on it and have run into the same issues in template9.d and foreach4.d. It appears we may need to special-case array literals and/or string literals (e.g. cast(wstring)"hello"). I think that's probably already done somewhere in the compiler, but I didn't have time to look for it yesterday. I'll work on it tonight. If you or @WalterBright can think of any hints to expedite my fix, I'd be most grateful.

[ghost]

Valid casts are not broken. Maybe to disallow casting between arrays of different count of dimension would work.
People who wants to do funky stuff can still use the "cast pointer and dereference" trick or the union one.

[JinShil]

I'm still waiting on the test suite, but I think I have a fix at #10036