Merge 2.075.1 front-end and stdlibs #2252
Conversation
kinke
force-pushed
the
merge-2.075
branch
2 times, most recently
from
09d55ee to
e425d0c
Compare
|
Built this branch and ran the stdlib tests natively on Android/ARM, had two issues:
I'm currently running the dmd-testsuite, will update with the results. |
|
The following tests now fail in the dmd-testsuite on Android/ARM:
|
DMD 2.074.0 errors out for this minimized testcase derived from failing std.uri:324... string foo()
{
char[] r;
return r;
} |
|
But templatizing makes it work for DMD and LDC... string foo()()
{
char[] r;
return r;
}
int main()
{
auto r = foo();
return cast(int) r.length;
} |
|
@kinke that's by design - mutable result of pure functions can be converted implicitly to immutable. Templating the function allows the compiler to infer purity. |
|
Thanks Petar. So I guess this is an additional check performed by LDC which needs to take this special case of legal implicit conversion into account. I lost the colors in the console on Windows, so I probably screwed up somewhere. |
|
@kinke I'm really confused as to why ldc differs from dmd here, as this feature has been in the language for quite a while and much more unittests in phobos should have been failing by now if ldc never supported this feature. For reference:
|
|
This is the stack trace for the (front-end) error emitted for No obvious LDC specifics in there, so no idea why we diverge here. |
|
Okay, it's just that we have our own LDC-specific |
|
Looking pretty good already, interpreting the Travis LLVM 4.0.1 Linux x64 job:
Note that I simply allowed all vector ops in new |
For DMD's new syntax highlighting. I grepped in all .{h,cpp} files only.
I don't know how much sense that makes, as the front-end expects a result expression of a single bool. LLVM returns a vector of i1 values, the pair-wise results. From my experience with SIMD on x86_64, what's mostly needed is a vector bit mask, as that's what the CPU returns and which is later used to mask accesses/writes. Anyway, due to new `Target.isVectorOpSupported()` simply allowing all ops, (not-)equality comparisons of vectors now land here, and I reduce the pair-wise results via integer bitcast and an additional integer comparison.
Conflicts: runtime/druntime
|
Update, remaining failures based on Semaphore (with enabled assertions; Travis only shows 2 remaining dmd-testsuite failures) and my local tests on Win64:
Oh and there's also druntime's |
|
|
gen/toir.cpp
Outdated
| return true; | ||
| } | ||
| // DMD issue 17457: detect structliteral.ctor(args) | ||
| else if (ce->e1->op == TOKdotvar) { |
There was a problem hiding this comment.
else is meaningless as the previous if ends in return anyway. Ditching it also makes the comment indentation less awkward.
There was a problem hiding this comment.
Glad these commits get actually looked at. ;) - Will address that tiny nit.
Any chance you could help out with the contract thing?
Fixes runnable/sdtor:2842 which tests this for return expressions (in a function using sret).
Conflicts: runtime/druntime runtime/phobos
On the contrary, I think DIP1000 is necessary to make using
How so? That part of the spec talks only about scope classes. With or without DIP1000 you can't return the address of a local variable (at least you shouldn't be allowed in |
|
DIP1000 is not the spec yet. Regarding classes the spec is pretty explicit that "scope is used to guarantee cleanup." |
I'm not sure if The stack-allocation is a transparent optimization which should not affect the behavior of the caller or callee. Whether it happens or not can be thought of as implementation-defined behavior. DIP1000 just now enforces the safety.
De jure it is not, de facto it is. The reason Walter did not put it there yet is that he wanted to right down all the changes to
That's about initialization - |
DIP1000 changes the
DIP1000 might be the future, but it is hidden behind -dip1000 which probably only few people use. It's still too broken, it can even causes code generation errors, e.g. https://issues.dlang.org/show_bug.cgi?id=17449, so I wouldn't call it the de facto spec yet.
There is nothing in the current spec that says so.
Unsafe, but not necessarily a bug. You only add I think changing semantic without a changelog entry is a regression, so I'll file a bug report with the example above. Let's hear what the scope experts say... |
https://issues.dlang.org/show_bug.cgi?id=17869 BTW: the code in #2252 (comment) does fail to compile with -dip1000. |
Conflicts: runtime/druntime
|
After dlang/phobos#5749 for Windows, we'll only have to take care of an apparent LLVM debuginfo inlining bug to be fully green. I compiled the earlier code snippet with current LDC master and this PR using a common LLVM 5.0. LDC master works with The base IR (without inlining) only differs in one function which has (nicely) changed due to befd05e (in-place construction): define weak_odr void @_D7xtest4617__T3mapVAyaa1_61Z3mapFNaNbNiNfAdZS7xtest4617__T3mapVAyaa1_61Z3mapFAdZ6Result(%"xtest46.map!\22a\22.map.Result"* noalias sret align 8 %.sret_arg, { i64, double* } %r_arg) #0 comdat !dbg !46 {
%r = alloca { i64, double* }, align 8 ; [#uses = 2, size/byte = 16]
- %.structliteral = alloca %"xtest46.map!\22a\22.map.Result", align 8 ; [#uses = 3, size/byte = 8]
store { i64, double* } %r_arg, { i64, double* }* %r, !dbg !51 ; [debug line = 3:10]
call void @llvm.dbg.declare(metadata { i64, double* }* %r, metadata !50, metadata !36), !dbg !51 ; [debug line = 3:10] [debug variable = r]
- %1 = getelementptr inbounds %"xtest46.map!\22a\22.map.Result", %"xtest46.map!\22a\22.map.Result"* %.structliteral, i32 0, i32 0 ; [#uses = 1, type = i8**]
+ %1 = getelementptr inbounds %"xtest46.map!\22a\22.map.Result", %"xtest46.map!\22a\22.map.Result"* %.sret_arg, i32 0, i32 0 ; [#uses = 1, type = i8**]
store i8* null, i8** %1, !dbg !52 ; [debug line = 12:9]
%2 = load { i64, double* }, { i64, double* }* %r, !dbg !52 ; [#uses = 1] [debug line = 12:9]
- %3 = call %"xtest46.map!\22a\22.map.Result"* @_D7xtest4617__T3mapVAyaa1_61Z3mapFAdZ6Result6__ctorMFNaNbNcNiNfAdZS7xtest4617__T3mapVAyaa1_61Z3mapFAdZ6Result(%"xtest46.map!\22a\22.map.Result"* nonnull returned %.structliteral, { i64, double* } %2) #0, !dbg !52 ; [#uses = 0] [debug line = 12:9] [display name = xtest46.map!"a".map.Result.this]
- %4 = bitcast %"xtest46.map!\22a\22.map.Result"* %.sret_arg to i8*, !dbg !52 ; [#uses = 1] [debug line = 12:9]
- %5 = bitcast %"xtest46.map!\22a\22.map.Result"* %.structliteral to i8*, !dbg !52 ; [#uses = 1] [debug line = 12:9]
- call void @llvm.memcpy.p0i8.p0i8.i64(i8* %4, i8* %5, i64 8, i32 1, i1 false), !dbg !52 ; [debug line = 12:9]
+ %3 = call %"xtest46.map!\22a\22.map.Result"* @_D7xtest4617__T3mapVAyaa1_61Z3mapFAdZ6Result6__ctorMFNaNbNcNiNfAdZS7xtest4617__T3mapVAyaa1_61Z3mapFAdZ6Result(%"xtest46.map!\22a\22.map.Result"* nonnull returned %.sret_arg, { i64, double* } %2) #0, !dbg !52 ; [#uses = 0] [debug line = 12:9] [display name = xtest46.map!"a".map.Result.this]
ret void, !dbg !52 ; [debug line = 12:9]
} |
|
Does the line info attached to both the |
|
The debuginfos look okayish; all calls have line infos. What looks wrong/suboptimal is the line info for the implicit LL |
|
I'm leaning towards disabling Edit: It's Semaphore only, as the assertion isn't triggered for MSVC targets with LLVM < 5. |
|
@kinke: I'd say just comment out the respective test, and create a ticket for it to be resolved before the final release. |
I'm not that optimistic, but we'll see. ;) - I upgraded SemaphoreCI to LDC-LLVM 5.0.0 as well. |
It was previously untested by AppVeyor (as it requires LLVM 5.0+).
|
Dammit, running out of (RAM?) disk space for Semaphore with LLVM 5, even when building with 2 jobs only... edit: down to 4.0.1 again. |
This inlining test now works as expected on Win32 with LLVM 5. XFAIL was the wrong choice here, as the test fails if it does NOT actually fail.
|
Looks like all CIs pass, with the only failure on Travis with llvm 3.7.1 that times out for some reason. Maybe it's time to merge this into master and let more people bang on it with a beta release? |
|
@kinke Great work! |
|
Ran the master stdlib and dmd tests on Android/ARM, only issue is the |
|
Rather than step through the This doesn't surface in the CI for dmd or ldc because they don't just run the phobos test runner with no arguments so it runs all the tests at once, but pass each test to the runner separately and this problem doesn't show up that way. I also looked at the issue where Running all the tests from one run of the test runner is a good stress test of running a lot of the stdlib code: it looks like we have a couple issues that are not exposed by our CI or upstream, because they don't run the test runner that way. |
Just the quick first version which can be successfully built (on Win64 at least) after ~2.5 hours of work. :)