Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update dmd-cxx glue layer minimally to get dmd building with the 2.071 backend #7595

Merged
merged 6 commits into from Feb 18, 2018
Merged

Update dmd-cxx glue layer minimally to get dmd building with the 2.071 backend #7595

merged 6 commits into from Feb 18, 2018

Conversation

joakim-noah
Copy link
Contributor

Minimal fixes to get it building with the 2.071 backend, plus updated Travis to build the 2.076.1 stdlib.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 4, 2018

Thanks for your pull request, @joakim-noah!

Bugzilla references

Auto-close Bugzilla Severity Description
10225 critical core.simd wrong codegen for XMM.STOUPS with __simd_sto
16488 major [spec][optimization] broadcast scalar to simd vector

@joakim-noah
Copy link
Contributor Author

Alright, that gets dmd building on linux and FreeBSD/x64, now for the stdlib.

@wilzbach, mind updating the druntime and phobos dmd-cxx branches to tag v2.076.1? I'll submit pulls for those branches after that.

{
// noop
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You remove the file from the Makefile below. Are you sure this modification is needed then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a different file name, objc_stubs.c, this one is still in there. Guess it was renamed at some point.

@wilzbach
Copy link
Member

wilzbach commented Jan 4, 2018

@wilzbach, mind updating the druntime and phobos dmd-cxx branches to tag v2.076.1?

dlang/druntime#2029 & dlang/phobos#5993

@joakim-noah
Copy link
Contributor Author

Were pulls necessary? I thought you could just update the branch, nothing really to review there.

@wilzbach
Copy link
Member

wilzbach commented Jan 4, 2018

Were pulls necessary? I thought you could just update the branch, nothing really to review there.

Force-push prevention is enabled for master, stable and dmd-cxx on all three repos.
I could have disabled it temporarily for dmd-cxx, but I think that force-pushing should be avoided whenever possible and doing the merge PRs shows other people what has happened.

@joakim-noah
Copy link
Contributor Author

OK, don't think it's really necessary for dmd-cxx yet, but I guess it should be turned on at some point, might as well do it now.

@wilzbach
Copy link
Member

wilzbach commented Jan 5, 2018

@joakim-noah - FYI: I just merged the merge PRs at druntime and Phobos, so we can move forward now :)
(I just wanted to give everyone at least a day to object)

@wilzbach
Copy link
Member

wilzbach commented Jan 5, 2018

dlang-bot added the Needs Work label 11 hours ago

Don't get confused by this. It simply means that two or more CIs are failing. We should probably stub out AppVeyor too.

@wilzbach wilzbach closed this Jan 5, 2018
@wilzbach wilzbach reopened this Jan 5, 2018
@wilzbach
Copy link
Member

wilzbach commented Jan 5, 2018

Closed & reopned to see if the branches disabled in AppVeyor worked (#7610).

@ibuclaw ibuclaw self-assigned this Jan 5, 2018
src/module.h Outdated
@@ -166,6 +166,10 @@ class Module : public Package

Symbol *sfilename; // symbol for filename

Symbol *massert; // module assert function
Symbol *munittest; // module unittest failure function
Symbol *marray; // module array bounds function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that these weren't re-added. I don't think the changes to the backend to support these are that big.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They won't be, this is a WIP pull.

travis.sh Outdated
@@ -2,7 +2,7 @@

set -exo pipefail

VERSION=v2.072.2
VERSION=v2.076.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't someone want us to switch to dmd-cxx branch for the library?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We updated dmd-cxx at druntime and Phobos to 2.076.1 today :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I used this until the dmd-cxx branch of the stdlib was updated to check what happens when it tries to build, can just use the dmd-cxx branch now.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 5, 2018

Don't get confused by this. It simply means that two or more CIs are failing. We should probably stub out AppVeyor too.

Yeah, that should probably be done (other CIs are stubbed out)

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing -dip1000 handling in the argument parser.

@joakim-noah
Copy link
Contributor Author

Does -dip1000 work in this frontend? I tried enabling it, but then druntime wouldn't build. -dip25 alone is fine, just the global.params.vsafe additions for DIP 1000 are causing an issue.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 6, 2018

It should work with this, if there's a missing patch, just point me to it.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 6, 2018

OK, tried building with vsafe turned on in gdc. I do get odd errors too.

Looks like I missed something in one of Walters mega patches.
d612c94197#diff-9450861ae2d65b41bed0152bb8f8e6e4R3121

Applying that, I get an error in runnable/testscope, which is fixed by another PR from Rainers. :-)

@ibuclaw
Copy link
Member

ibuclaw commented Jan 6, 2018

Patch against GDC, the frontend patch will apply cleanly here also.

https://github.com/D-Programming-GDC/GDC/pull/591/files?w=1

@joakim-noah
Copy link
Contributor Author

Thanks, that fixes the druntime issues with -dip1000. Why don't you submit it as a pull for this branch?

@dlang-bot dlang-bot added the WIP Work In Progress - not ready for review or pulling label Jan 14, 2018
@joakim-noah
Copy link
Contributor Author

I suppose there's no point to this branch if we're going to start bumping the dmd compiler version and tell porters to cross-compile with ldc/gdc initially.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 20, 2018

Well, this frontend is a mirror of gdc. It's as close as possible to 2.076 (notably without static foreach).

It allows switching to ddmd without any codegen or library changes. (D-Programming-GDC/gdc#550)

Whether or not you want dmd backend working with this is your choice. But this branch will be the future baseline for compiling the dmd frontend implementation for the next few years.

@joakim-noah
Copy link
Contributor Author

I understand, but is there any reason for this dmd-cxx branch to exist? We can just switch from using the official release of dmd 2.068.2 to dmd 2.076.1 on the auto-tester and other CI. I pushed for this branch because I thought we needed it for ports to bootstrap from source on a new platform, but if we're just going to tell them to cross-compile dmd/ldc/gdc master from one of the well-supported platforms and use that compiler binary, I see no need for a wholly C++ dmd.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 20, 2018

Beyond switching from github to svn? (I hope gcc switches to git soon) - for me, this branch is intended as an upstream mirror.

@joakim-noah
Copy link
Contributor Author

OK, if there's no real reason to maintain this branch, I'm not going to bother updating the backend. Let's just get this pull in to get dmd minimally building on linux/Freebsd x64 and that should be enough.

@joakim-noah joakim-noah changed the title [WIP] Update backend Update dmd-cxx glue layer minimally to get dmd building with the 2.071 backend Jan 20, 2018
@dkgroot
Copy link
Contributor

dkgroot commented Jan 20, 2018

Having a bootstrap compiler is favorable in my opinion, especially if it is one of the build steps in continues integration (which would make sure future changes would keep everything in sync). I would not even mind if 2-3 intermediate compiler stages were required to get to the current master. I am currently using v2.076.1 v2.067.1 as the bootstrap compiler which is able to Compile the current master directly.

The porting process is quite cumbersome if you have to patch the ldc/gdc compiler and get your PR's accepted and then have to move to another group to get PR's accepted to the dmd Frontend so they can trickle down into your destination compiler at some stage. It creates a chicken egg situation. It is a little difficult to explain how this puts you in a bit of a split dealing with multiple groups who may or may not see the benefit of the addition you want to make. Maybe one of you should try and go through the porting process themselves (using one of the other compilers) for real to see how it does work out.

Dave MacFarlane tried porting ldc+druntime+phobos to dragonfly a couple of months ago, and his PR's were received well, but declined because he had to get them merged upstream first. How to do this is in a sane manner without having a dmd compiler that works on your platform is unclear.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 20, 2018

Dave MacFarlane tried porting ldc+druntime+phobos to dragonfly a couple of months ago, and his PR's were received well, but declined because he had to get them merged upstream first. How to do this is in a sane manner without having a dmd compiler that works on your platform is unclear.

The usual penance of such things usually happens in this order:

  1. Port to platform X using your downstream compiler of choice - gdc or ldc, doesn't matter, both offer compilers that don't depend on a D compiler preexisting.

  2. Submit library changes upstream, things relating to platform never change, so it should be trivial to take your work and apply it to druntime/phobos master.

  3. Downstream compilers may be lenient and accept upstream changes as backports.

There is no requirement to get dmd working with platform X. But if it runs on a cpu supported by dmd, then you can continue to:

  1. Compile dmd master with downstream compiler that has backported patches. Fix it so that it builds and works properly.

@dkgroot
Copy link
Contributor

dkgroot commented Jan 20, 2018

I am not saying it is impossible, it just makes everything a little more difficult, having to figure out multiple compilers at the same time. From a porting perspective, the nice thing about the dmd compiler it is pretty much self contained and does not have (m)any outside dependencies. At the moment i have progressed to having PR's against the current dlang/master dmd/druntime/phobos repos, and outstanding PR's for the ldc-ltsmaster repos for the ldc bootstrap. So yes both can be done at the same time.

Getting started on the porting route is a little daunting, especially without a clear manual on what to do first. Being able to port one single project instead of having to figure out multiple would be preferable in my opinion. Starting with the reference compiler seemed like the obvious choice (to me).

Maybe i should just but and let you guys get on with it. I am hoping i will still be allowed to merge my changes to the dmd-cxx branch ones it is back in working order (ie: this PR is merged).

@joakim-noah
Copy link
Contributor Author

I am currently using v2.076.1 as the bootstrap compiler which is able to Compile the current master directly.

Not sure what you mean by bootstrap compiler, as dmd v2.076.1 requires a D compiler to build.

From a porting perspective, the nice thing about the dmd compiler it is pretty much self contained and does not have (m)any outside dependencies.

Yes, that's nice, but you could still cross-compile using dmd too. Also, ldc and gdc still maintain compiler versions fully written in C++, as Iain just mentioned.

Getting started on the porting route is a little daunting, especially without a clear manual on what to do first. Being able to port one single project instead of having to figure out multiple would be preferable in my opinion. Starting with the reference compiler seemed like the obvious choice (to me).

For non-x86/x64, there's no choice, dmd doesn't support them. I agree that starting with dmd would be easier, but since the plan is to keep bumping the required frontend version, you'll have no choice but to cross-compile eventually. We should write up the porting process on the wiki, I guess that's on me since I've done the most with porting.

I am hoping i will still be allowed to merge my changes to the dmd-cxx branch ones it is back in working order (ie: this PR is merged).

No, this is just going to be an upstream mirror for Iain's frontend, this compiler won't even build on most of the supported platforms.

We'll put your changes into ldc-ltsmaster, you can use that as a wholly C++ bootstrap compiler for now.

@dkgroot
Copy link
Contributor

dkgroot commented Jan 21, 2018

@joakim-noah I accidentally switched around the version number, should have been v2.067.1. Ok thanks for talking me through it.

@joakim-noah
Copy link
Contributor Author

In case it wasn't clear, I'm done working on this pull. It builds on linux/x64 and compiles the stdlib, as seen on Travis, but can't build the tests.

@wilzbach
Copy link
Member

@dkgroot it looks like if you want this to happen you need to pick up the torch yourself ;-)

@ibuclaw
Copy link
Member

ibuclaw commented Feb 18, 2018

Removed genhelpers as per my request.

@ibuclaw
Copy link
Member

ibuclaw commented Feb 18, 2018

Pushed OPvecfill fix for druntime build.

@ibuclaw
Copy link
Member

ibuclaw commented Feb 18, 2018

Applied PIC fix as per @dkgroot, and a further STO fix for simd store ops.

@ibuclaw
Copy link
Member

ibuclaw commented Feb 18, 2018

And... travis builds pass again. :-)

@wilzbach - FYI, merging.

@ibuclaw ibuclaw merged commit daf285a into dlang:dmd-cxx Feb 18, 2018
@ibuclaw ibuclaw removed their assignment Mar 4, 2018
@joakim-noah joakim-noah deleted the cxx-merge branch March 13, 2018 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix C++ Port WIP Work In Progress - not ready for review or pulling
Projects
None yet
5 participants