This is an archive of the discontinued LLVM Phabricator instance.

[flang][driver] Add support for generating executables
ClosedPublic

Authored by awarzynski on Mar 18 2022, 7:13 AM.

Details

Summary

This patch adds 2 missing items required for flang-new to be able to
generate executables:

  1. Extra linker flags to include Fortran runtime libraries.
  1. The Fortran_main runtime library, which implements the main entry point into Fortran's PROGRAM.

With this change, you can generate an executable that will print `hello,
world!` as follows:

$ cat hello.f90
program hello
  write (*,*), "hello, world!"
end program hello
$ flang-new hello.f90
./a.out
hello, world!

Note: Fortran_main was originally written by Peter Klausler, Jean Perier
and Steve Scalpone in the fir-dev` branch in [1].

[1] https://github.com/flang-compiler/f18-llvm-project

Co-authored-by: Eric Schweitz <eschweitz@nvidia.com>
Co-authored-by: Peter Klausler <pklausler@nvidia.com>
Co-authored-by: Jean Perier <jperier@nvidia.com>
Co-authored-by: Steve Scalpone <sscalpone@nvidia.com

Diff Detail

Event Timeline

awarzynski created this revision.Mar 18 2022, 7:13 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added a subscriber: mgorny. · View Herald Transcript
awarzynski requested review of this revision.Mar 18 2022, 7:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2022, 7:13 AM

Fix formatting

awarzynski edited the summary of this revision. (Show Details)Mar 18 2022, 9:24 AM
rovka accepted this revision.Mar 22 2022, 5:21 AM
rovka added a subscriber: rovka.

LGTM :)

This revision is now accepted and ready to land.Mar 22 2022, 5:21 AM

Use llvm_add_library instead of add_flang_library

Fortran_main should be a static library, but add_flang_library does not parse
STATIC in add_flang_library(Fortran_main STATIC Fortran_main.c). I could
extend add_flang_library instead, but llvm_add_library works fine as well
and this change is less intrusive.

Also rebased on top of main.

We discussed this patch in our community call today and some people expressed their reservations about merging this just now. As I mentioned, I would like to have this merged to unblock the CMake PR. As promised, here are the options that we've considered:

  1. merge this patch "today",
  2. merge this patch "today", but add a flag so that by default flang-new file.f90 wouldn't generate an executable (we would remove this flag at a later time),
  3. wait until the upstreaming of fir-dev is complete (based on today's update, I think that that would be in June/July the earliest).

Could you tell me what your preference is?

Please keep in mind that even once CMake support is available, it's going to land in the "latest" version of CMake. Folks will have to download the latest binaries from https://github.com/Kitware/CMake/releases in order to benefit from this. With time, CMake shipped with various OSes will catch-up and become "modern" enough. But there's going to be a delay and hence it makes sense to get the CMake support sooner rather than later.

Please comment if I missed or misinterpreted anything. Also, I will add more reviewers - basically everyone who discussed this in the call today. Please add anyone that I've missed!

Thanks for taking a look!

I'm personally in favor of number 3.

As I said this morning, I prefer to wait with this patch until the upstreaming is finished. The reason is not only to do with upstreaming, but also with a concurrent effort to button up any assertion and runtime errors so the initial user experience is better. While some posit the the news of your patch won't be heard around the world, I think it is a major accomplishment & the user community will be thrilled to try flang. My preference is for that first wave of users to come a way with the impression that flang is a high-quality work in progress.

@awarzynski The CMAKE PR is closed. Do you plan to revitalize it? Will you check that this patch, along with the CMAKE PR, is indeed enough to satisfy the requirement for the cmake reviewers as well as provides enough functionality to support cmake in a real project?

There's 128037 lines of code and documentation outside of lowering and drivers in flang/, and "git blame" tells me that I wrote 63% of them, including most of the Fortran semantics and nearly all of the Fortran parsing and runtime. Based on my knowledge of the code base, the Fortran language, and where we stand in the process of shaking things out with applications and test suites, it is clear to me that things are not yet ready to be opened up to a larger user base. We certainly don't need wider exposure yet to flush out more bugs. Premature exposure now would disrupt the critical path to overall success. So I prefer option (3).

https://reviews.llvm.org/D54604 used a really ugly flag:
-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang

Option 1 for me, let's not delay.

With upstreaming of fir-dev making great progress, we can start to think about having a working flang in LLVM 15. For that we would need to have upstreaming finished, the driver finished and CMake support finished before end of June. The initial user experience work should be focused on LLVM 15. The sooner we have the code on main branch, the sooner we can begin the work of shaking out the problems. To delay means that flang in LLVM 15 will not be as good as it could be.

This patch is orthogonal to the issues discussed on this thread and on the call on 4th April. Anyone using flang today is going to see those issues just the same with or without this patch as flang will error out before it gets to linking. Committing this patch won't change what flang can't do yet but it will probably flush out some more issues - perhaps serious ones - for us to fix. I don't think we should be scared to receive bug reports. At this stage in development, bug reports are surely good for us and the sooner we get them, the sooner we can potentially fix the issues.

I think the group of interested users with high quality-level expectations for flang are going to be evaluating it on a release rather than at some random point on main. I think anyone following along on main is going to have an accurate understanding of flang's maturity there and unlikely to be surprised by the quality of it. We should consider asking Alex to skip this patch in LLVMWeekly, or ask him to message it as we want to.

rovka added a comment.EditedApr 7 2022, 1:52 AM

Just for the record, I'd like to speak in favor of option 2 - merge the patch now, but put the functionality behind a flag. This sounds like a good compromise to me. I think if we choose an inconspicuous name like -fexperimental-test-driver-integration-with-cmake (or something even more obfuscated) we'll be off most users' radar, while at the same time making progress with cmake. We should probably first confirm that it would be acceptable for the cmake developers to go ahead with something like that.

I also very much like the second option. I think it prevents a naive user from stumbling into treacherous territory. Also, as someone who is funded to write tests for flang, making it easier to build executables would expand the options for testing. Currently, we're focused on semantics tests primarily because of the complications associated with generating executables.

In my example, they even promised that the flag will removed.

MaskRay added inline comments.Apr 12 2022, 1:11 PM
clang/lib/Driver/ToolChains/Gnu.cpp
392

I think the multilib style CLANG_LIBDIR_SUFFIX is being phased out in clang driver, so no need for the comment.

397

Thank you all for your feedback!

CMake integration
I have a couple more data points. I've experimented with CMake using Tin's CMake PR. I confirm that together with this patch, I was able to build a small CMake project:

-- The Fortran compiler identification is LLVMFlang 15.0.0
-- Detecting Fortran compiler ABI info
-- Detecting Fortran compiler ABI info - done
-- Check for working Fortran compiler: /home/build/release/bin/flang-new - skipped
-- Configuring done
-- Generating done
-- Build files have been written to: /home/work/cmake_test

As you can see, the compiler discovery worked fine (I was able to build and executable using the CMake generated Make files). I also tried the same experiment with an extra flag (Option 2 above) and that didn't work. I investigated a bit and discovered that internally CMake calls try_compile (CMake docs). I don't see any way to pass custom compiler options there (i.e. try_compile expects flang-new file.f90 to just work). This makes me rather hesitant to pursue Option 2 at all.

@sscalpone I hope that this answers your question. I'm happy to re-open that PR myself, but it would probably make more sense for Tin to do it (since he wrote it originally). Either way, we will be making sure that it's attributed to Tin.

I think that it's also worth taking into account CMake's release cycle. Based on the following, it looks like a new version is released every March, July and November:

It would be extremely helpful to align adding CMake support for LLVM Flang with LLVM releases.

LLVM 15 Release
While people seem to lean towards Option 3, I would like this change to be merged on time for LLVM 15. LLVM 16 will be out in 2023 - lets not wait that long. Also, with no public progress tracker for upstreaming, I would really like to make sure that there is a cut off date for this to land in LLVM.

Other points

As I said this morning, I prefer to wait with this patch until the upstreaming is finished. The reason is not only to do with upstreaming, but also with a concurrent effort to button up any assertion and runtime errors so the initial user experience is better.

Can you give us an indication when to expect this to be completed? Also, like @richard.barton.arm pointed out, people already can see the issues that you mentioned even without this patch (i.e. miscompilation errors are orthogonal to this).

Note that this is only the first step. We still need to rename flang-new as flang. That's likely going to require a separate discussion. In fact, from my experience most people new to LLVM Flang use flang rather than flang-new and most (all?) find it very confusing. This change is unlikely to change their perception of LLVM Flang because it only affects flang-new.

Lastly, I don't quite understand why can't we use project's documentation to communicate the level of support instead of artificially delaying patches like this one. This is what we currently claim (extracted from index.md, contributed in https://reviews.llvm.org/D120067 by myself):

While it is capable of generating executables for a number of examples, some functionality is still missing.

I wrote that, perhaps naively, believing that this patch (or something similar) would soon be merged. In any case, if Flang developers are concerned how LLVM Flang is perceived by compiler users, then could we please fix this through better documentation and LLVM release notes?

New proposal
@rovka and @rouson , I know that you voted for Option 2, but after my CMake experiment I feel that we should really choose between Option 1 and 3. As there isn't enough support for Option 1, we should probably park this for now. Please let me know if Option 2 would unblock any of you (or anyone else!) and we can re-visit. Otherwise, lets wait a bit. Our goal is to have this merged in time for LLVM 15.

Thank you all for reading,
Andrzej

Updates based on feedback from @MaskRay (thank you!)

Speaking as a relative outside (but who's been waiting for flang in LLVM since well before it joined the monorepo), I'd much prefer code-generation-with-rough-edges in LLVM 15 (to start testing and raising eventual bugs), rather than a more polished flang (realistically still with bugs that need to be shaken out) in LLVM 16.

I can understand why Option 2 would be more attractive for those not wanting to attract too much attention yet (and shouldn't be a blocker for my purposes), hence I looked a bit into:

I investigated a bit and discovered that internally CMake calls try_compile (CMake docs). I don't see any way to pass custom compiler options there (i.e. try_compile expects flang-new file.f90 to just work). This makes me rather hesitant to pursue Option 2 at all.

I'd be more than a little surprised that there's no way to do this. Have you tried CMAKE_Fortran_FLAGS? AFAICT, the environment variables should be respected. After a bit of googling, a very similar issue came up already at least 15 years ago, and here's a recent SO answer about it. Worst case, one can always ask on the CMake discourse, people are very helpful and responsive in my experience.

Given that Option 2 might be the path of highest consensus, I think it would be worth a bit more investigation if it's really impossible.

ktras added a subscriber: ktras.Apr 13 2022, 3:35 PM
rouson added a comment.EditedApr 13 2022, 4:24 PM

@awarzynski based on @h-vetinari's optimism about adding flags, I still like option 2. If it's not possible to add flags, then I would prefer option 1. If we go with option 3, then I am at least encouraged by reviewing the upstreaming project status links that @kiranchandramohan posted in the Slack flang workspace #general channel. I don't know the time required to complete the remaining tasks, but the following issues show 216 of 235 tasks completed with 2 in progress:

Statement Lowering Upstreaming:
12 To Do/1 In Progress/30 Done

FIR Codegen Upstreaming:
2 To Do/0 In Progress/81 Done

Intrinsic Lowering Upstreaming::
0 To Do/0 In Progress/94 Done

FIR passes upstreaming:
3 To Do/1 In Progress/11 Done

rouson added a comment.EditedApr 13 2022, 4:39 PM

@awarzynski I'll echo your comment that "... most people new to LLVM Flang use flang rather than flang-new and most (all?) find it very confusing." I've known of flang in various forms (including a flang that predated classic flang) and yet I'm still finding new information that clears up confusion on my part. I only just realized today that the phrase "LLVM flang" is used to differentiate from "classic flang." And although I've known about armflang for several years and AOCC (which seemingly should be AOCCF or some such) for a month or so, I only just realized today that they are based on classic flang, which finally makes sense because I couldn't figure out how they could be based on what is currently on the main branch of LLVM flang. For users and testers, it will be really nice if the community coalesces around the main branch of LLVM flang as the default meaning when someone says "flang" and if that default flang produces executables even if it's buggy.

@h-vetinari & @rouson, thanks for the encouragement!

I'd be more than a little surprised that there's no way to do this. Have you tried CMAKE_Fortran_FLAGS? AFAICT, the environment variables should be respected.

I completely forgot about CMAKE_Fortran_FLAGS! Indeed, it worked :) I really appreciate you doing all that digging and the suggestion itself! Yes, sounds like we can return to Option 2, which is fantastic news and great team effort!

I will post an updated version of this patch shortly. My suggestion for the new flag: -flang-experimental-exec. Hopefully not too long or to vague and informative enough. WDYT?

I don't know the time required to complete the remaining tasks, but the following issues show 216 of 235 tasks completed with 2 in progress:

Thanks for that executive summary @rouson, it does look encouraging :) Unfortunately, upstreaming is effectively on hold ATM. People are not too keen to commit to any dates (understandable), but we (Arm) will continue working towards the LLVM 15 target (fingers crossed!).

Summary
AFAICT, Option 2 addresses all the issues raised here and (more importantly) unblocks a few new streams of development. IMO, it's a very good compromise and we should go ahead with it. I will leave this here for a few more days in case people have more comments.

Thank you,
-Andrzej

Add a flang, -flang-experimental-exec, to guard the new functionality.

Update the tests after the previous update

clementval added a comment.EditedApr 18 2022, 5:24 AM

Do you plan to discuss this again during the next call? Note that today is a holiday in various country in Europe (maybe elsewhere too) so the one on 4/27 is probably better.

Do you plan to discuss this again during the next call? Note that today is a holiday in various country in Europe (maybe elsewhere too) so the one on 4/27 is probably better.

To be perfectly honest, I wasn't planning to.

The main reason to bring this up in one of our community calls was to increase the visibility of this patch. I feel that that goal has been achieved. Keeping the discussion here means that even people who are unable to join our calls can participate and share their view. As such, discussing on Phabricator is more inclusive.

While I appreciate that some people expressed preference for Option 3, it seemed to me that Option 2 is a compromise that will work for everyone. It hides the new functionality behind a flag and makes it very counter-intuitive to use. The flag itself makes it clear that this functionality is experimental. At the same time, Option 2 is sufficient to unblock progress in other areas.

If folks disagree and feel that Option 2 is still problematic, could you elaborate "why" and suggest an alternative? There's no need to wait for the next call to discuss this.

Thanks!

Btw, if there are no new comments, I would like to merge this in the coming days.

Btw, if there are no new comments, I would like to merge this in the coming days.

New comments or not, this step remains premature from my perspective.

New comments or not, this step remains premature from my perspective.

Let me make -flang-experimental-exec a "hidden" option then. This way this flag (and the functionality that it enables) will be less discoverable (we still will be to use it). We will be removing it at some point anyway.

This revision was landed with ongoing or failed builds.Apr 25 2022, 5:02 AM
This revision was automatically updated to reflect the committed changes.