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.

Sign up for GitHub

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

Jump to bottom

add support for llvm-based Fortran compiler #4867

Closed
wants to merge 2 commits into from
Closed

add support for llvm-based Fortran compiler #4867

wants to merge 2 commits into from

Conversation

junghans
Copy link
Contributor

@junghans junghans commented Jun 6, 2017

@gentoo/sci @gentoo/llvm

@junghans junghans added the work in progress The PR is not yet ready to be merged. label Jun 6, 2017
@junghans junghans requested a review from mgorny June 6, 2017 06:34
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull Request assignment

Areas affected: ebuilds
Packages affected: sys-devel/clang, sys-devel/flang

sys-devel/clang: @gentoo/llvm
sys-devel/flang: @gentoo/proxy-maint (new package)

@gentoo-repo-qa-bot gentoo-repo-qa-bot added new package The PR is adding a new package. assigned PR successfully assigned to the package maintainer(s). labels Jun 6, 2017
mgorny
mgorny requested changes Jun 6, 2017
View reviewed changes
Copy link
Member

@mgorny mgorny left a comment

Choose a reason for hiding this comment

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

NEEDINFO. Aside to the minor issues I've noted, I'm strongly against heavy patching of clang in upstream-incompatible way.

sys-devel/clang/metadata.xml Outdated
@@ -29,6 +29,7 @@ Conformance with C/C++/ObjC and their variants</longdescription>
<use>
<flag name="default-compiler-rt">Use compiler-rt instead of libgcc as the default rtlib for clang</flag>
<flag name="default-libcxx">Use libc++ instead of libstdc++ as the default stdlib for clang</flag>
<flag name="flang">Add NVidia patch to support <pkg>sys-devel/flang</pkg></flag>
Copy link
Member

Choose a reason for hiding this comment

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

Seriously? Are we really supposed to advertise the vendor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

sys-devel/clang/Manifest
@@ -1,4 +1,5 @@
DIST cfe-4.0.0.src.tar.xz 10900916 SHA256 cea5f88ebddb30e296ca89130c83b9d46c2d833685e2912303c828054c4dc98a SHA512 a0d9972ec337a5c105fcbe7abc4076ba1e580f28908a3318f43bbfe59143f446ed5b78dad210f624145d7e5a3d56c15bfead78826c068422b60120fa1cfa482a WHIRLPOOL fe04b6955b82915bba09726947fceff92e67ffaac97de4b8c32c18546262f60a4307fdaccd3c9540710392658ed47f3bcfe44791de8d7d30786d56576f339aee
DIST clang-4.0.0-flang.patch.xz 12596 SHA256 af5529c1405eb1679d083f8cb7ad232ff0727ec03fb51775e769c652a721fd82 SHA512 e60718ae6d232e9033c8d346c07688c67b6bb0ef2b1893da69d4359d282a3ab2f1737a3177b85899557b04d3fad6310c319220873362f6d49d1edf9393583523 WHIRLPOOL 19f3dac0ef84b89a2c5eb2f93276eeb14a34d8b175bc8e0b137c531b9f40d21fc2bf1ac700f4f574dd578f4d2b86426ebf53fde957c4348daedde165d8a8160c
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a major change. What's the state of this upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The patch isn't big: junghans/clang@release_40...junghans:flang_release_40
and it mainly add a fortran case everywhere.

Half of it has been merge between 3.9 and 4.0, the rest hopefully will go into the next release.

sys-devel/flang/flang-9999.ebuild Outdated
local mycmakeargs=(
-DTARGET_ARCHITECTURE=$(uname -m) #https://github.com/flang-compiler/flang/pull/83
-DTARGET_OS=$(uname -s) #https://github.com/flang-compiler/flang/pull/86
-DLLVM_CMAKE_PATH="${EPREFIX}"/usr/$(get_libdir)/llvm/4/$(get_libdir)/cmake/llvm
Copy link
Member

Choose a reason for hiding this comment

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

This is the incorrect path which you incorrectly assumed because on your /usr/lib symlink. Plus, there's llvm.eclass to give you paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@junghans
sys-devel/clang: add flang support
f7d1fdd 
Package-Manager: Portage-2.3.5, Repoman-2.3.1
@junghans
sys-devel/flang: initial commit
2bd6cf6 
Package-Manager: Portage-2.3.5, Repoman-2.3.1
@junghans
Copy link
Contributor Author

junghans commented Jun 6, 2017 •
edited

@mgorny: The flang patch mainly extends clang functionality, it is just slightly too big for ${FILESDIR}. However, I think, patching it in would be useful as a Fortran compiler is a big step to support llvm-based compilers as a system compilers.

I leave it up to you if you want to conditionally patch it in or not.

@junghans junghans mentioned this pull request Jun 6, 2017
Closed
@mgorny
Copy link
Member

mgorny commented Jun 6, 2017

If at all, I'd rather apply it unconditionally. However:

  1. I'd rather see that this is likely to land in upstream LLVM soonish.
  2. I want 9999 to be superset of 4.0, not the other way around. So a patch for 9999 would be required as well.

@SoapGentoo
Copy link
Member

I have no say in this, but personally am opposed to this. These patches are way too heavy for downstream stuff. I would wait until the next release, but again, this is @mgorny's call.

@gentoo-repo-qa-bot
Copy link
Collaborator

😞 The QA check for this pull request has found the following issues:

Issues inherited from Gentoo (may be modified by PR):
https://qa-reports.gentoo.org/output/gentoo-ci/3216d5f2d/output.html#sci-biology/mammoth
https://qa-reports.gentoo.org/output/gentoo-ci/3216d5f2d/output.html#sci-chemistry/ambertools
https://qa-reports.gentoo.org/output/gentoo-ci/3216d5f2d/output.html#sci-chemistry/gamess
https://qa-reports.gentoo.org/output/gentoo-ci/3216d5f2d/output.html#sci-chemistry/mead
https://qa-reports.gentoo.org/output/gentoo-ci/3216d5f2d/output.html#sys-libs/suacomp

@junghans
Copy link
Contributor Author

junghans commented Jun 6, 2017

The diff stat is:

 include/clang/Basic/DiagnosticDriverKinds.td |   2 +
 include/clang/Driver/Action.h                |  11 ++++
 include/clang/Driver/Driver.h                |   6 +-
 include/clang/Driver/Options.td              | 118 +++++++++++++++++++++++++++++++++--
 include/clang/Driver/Phases.h                |   1 +
 include/clang/Driver/ToolChain.h             |  15 +++++
 include/clang/Driver/Types.def               |   8 ++-
 include/clang/Driver/Types.h                 |   9 +++
 lib/Driver/Action.cpp                        |   7 +++
 lib/Driver/Driver.cpp                        |   5 ++
 lib/Driver/Phases.cpp                        |   1 +
 lib/Driver/ToolChain.cpp                     |  53 ++++++++++++++++
 lib/Driver/ToolChains.cpp                    | 188 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/Driver/ToolChains.h                      |   3 +
 lib/Driver/Tools.cpp                         | 793 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/Driver/Tools.h                           |  18 ++++++
 lib/Driver/Types.cpp                         |  69 +++++++++++++++++----
 tools/driver/CMakeLists.txt                  |   2 +-
 18 files changed, 1286 insertions(+), 23 deletions(-)

so honestly there are mainly additions, so it seems safe to unconditionally apply it.

@mgorny
Copy link
Member

mgorny commented Jun 13, 2017

All things considered, I'm going to reject it for the moment. We can revisit this when there's a clear agreement between flang and clang upstreams, and either the patches are in clang's git, or at least are on good way making it there.

To complete the rationale: I don't think this kind of upstream patching is a good solution. Even if the patch is not really intrusive (which I don't have the resources to properly verify), it is a burden. For next clang release, I will have to either wait for flang project to release an updated patch or temporarily kill flang support. This is not the kind of patchset chasing I'm interested in.

@mgorny mgorny closed this Jun 13, 2017
@junghans
Copy link
Contributor Author

See llvm-mirror/clang#28 and https://reviews.llvm.org/D34097

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

@mgorny mgorny mgorny requested changes

@nicolasbock nicolasbock nicolasbock approved these changes

Assignees
No one assigned
Labels
assigned PR successfully assigned to the package maintainer(s). new package The PR is adding a new package. work in progress The PR is not yet ready to be merged.
Projects
None yet
Milestone
No milestone
5 participants
@junghans @gentoo-repo-qa-bot @mgorny @SoapGentoo @nicolasbock