Page MenuHomePhabricator

[flang][driver] Rename `flang-new` as `flang`
Needs ReviewPublic

Authored by awarzynski on May 17 2022, 7:13 AM.

Details

Summary

This patch _conditionally_ renames flang-new as flang. This is controlled with the FLANG_USE_LEGACY_NAME Cmake option, which by default is set to ON. This means that by default, the driver executable will still be called flang-new. If you want to use the updated name, flang, set FLANG_USE_LEGACY_NAME to OFF when configuring LLVM Flang. No other set-up is required.

Similarly to Clang, Flang's driver executable will be called flang{-new}-<MAJOR_VERSION> and flang{-new} will be a symlink to flang{-new}-<MAJOR_VERSION>.

Diff Detail

Event Timeline

awarzynski created this revision.May 17 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.May 17 2022, 7:13 AM

Do you know how this will work with the llvm-testsuite runs that currrently use gfortran for codegen? Here is one we have: https://lab.llvm.org/buildbot/#/builders/179/builds/3694

Currently:

2022-05-17 13:06:53 INFO: Execute: /usr/bin/cmake -DCMAKE_C_COMPILER:FILEPATH=/home/tcwg-buildbot/worker/clang-aarch64-full-2stage/stage2.install/bin/clang -DCMAKE_CXX_COMPILER:FILEPATH=/home/tcwg-buildbot/worker/clang-aarch64-full-2stage/stage2.install/bin/clang++ /home/tcwg-buildbot/worker/clang-aarch64-full-2stage/test/test-suite -DCMAKE_C_FLAGS=-mcpu=cortex-a57 -DCMAKE_C_FLAGS_DEBUG= -DCMAKE_C_FLAGS_MINSIZEREL= -DCMAKE_C_FLAGS_RELEASE= -DCMAKE_C_FLAGS_RELWITHDEBINFO= -DCMAKE_CXX_FLAGS=-mcpu=cortex-a57 -DCMAKE_CXX_FLAGS_DEBUG= -DCMAKE_CXX_FLAGS_MINSIZEREL= -DCMAKE_CXX_FLAGS_RELEASE= -DCMAKE_CXX_FLAGS_RELWITHDEBINFO= -DTEST_SUITE_FORTRAN:STRING=ON -DCMAKE_Fortran_COMPILER=/home/tcwg-buildbot/worker/clang-aarch64-full-2stage/stage2.install/bin/flang
<...>
-- The Fortran compiler identification is unknown
-- Check for working Fortran compiler: /home/tcwg-buildbot/worker/clang-aarch64-full-2stage/stage2.install/bin/flang
-- Check for working Fortran compiler: /home/tcwg-buildbot/worker/clang-aarch64-full-2stage/stage2.install/bin/flang  -- works
-- Detecting Fortran compiler ABI info
-- Detecting Fortran compiler ABI info - failed
-- Checking whether /home/tcwg-buildbot/worker/clang-aarch64-full-2stage/stage2.install/bin/flang supports Fortran 90
-- Checking whether /home/tcwg-buildbot/worker/clang-aarch64-full-2stage/stage2.install/bin/flang supports Fortran 90 -- yes

Can we expect the current flang-new, soon to be flang to also cope with these tests (I have no expectations myself here, fine if not) or should we move the bots to use flang-to-gfortran?

Can we expect the current flang-new, soon to be flang to also cope with these tests (I have no expectations myself here, fine if not) or should we move the bots to use flang-to-gfortran?

Great point, thank for highlighting it! Moving the bots to flang-to-gfortran would make most sense. I can split this change into two patches so that there's no disruption from the buildbot perspective:

  • patch 1: rename flang as flang-to-gfortran, but keep flang as an alias for flang-to-gfortran,
  • patch 2: all the remaining changes from this patch.

Once "patch 1" is merged, we can update the buildbot config to use flang-to-gfortran. After that, we could land "patch 2".

Sounds good to me. https://reviews.llvm.org/D125796 for the bot side, let me know if/when the first half of the change goes in.

I'll leave it to others to approve the change overall. I do think being able to do ./bin/flang and get the right flang will save a lot of confusion but I'm not a user of either of them myself.

Very happy to see this finally happening! :)

LGTM. In the call on Monday, it was mentioned that the original users of the flang script have moved on to using a custom driver.

It will be a good idea to,
-> wait for the author of the original script and the throwaway driver, @sscalpone.
-> clarify that execution is still restricted to developers via a flag. We hope to open it to users later in the year.

My personal preference for the renaming is flang-to-external-fc since it is not specific to gfortran.

flang/tools/flang-driver/CMakeLists.txt
55

Nit: backwords -> backwards

This revision is now accepted and ready to land.May 18 2022, 2:08 AM

Sounds good to me. https://reviews.llvm.org/D125796 for the bot side, let me know if/when the first half of the change goes in.

https://reviews.llvm.org/D125832 for "patch 1".

It will be a good idea to,
-> wait for the author of the original script and the throwaway driver, @sscalpone.

👍🏻

-> clarify that execution is still restricted to developers via a flag.

👍🏻 Nothing changes with regard to the compiler flags (there are no functional changes here, just the renaming).

My personal preference for the renaming is flang-to-external-fc since it is not specific to gfortran.

Makes sense, I can rename it.

Rename flang-to-gfortran as flang-to-external-fc

I will also make this change depend on https://reviews.llvm.org/D125832

clarify that execution is still restricted to developers via a flag.

Just confirming what this means: After this patch, would flang sample.f90 generate an executable? If not, how to generate an executable using flang-new? If yes, does this mean that if I just replace classic-flang with new llvm flang (along with library and include paths), I should be able to find and report bugs (expected) or run the application (best case)?

clarify that execution is still restricted to developers via a flag.

Just confirming what this means: After this patch, would flang sample.f90 generate an executable? If not, how to generate an executable using flang-new? If yes, does this mean that if I just replace classic-flang with new llvm flang (along with library and include paths), I should be able to find and report bugs (expected) or run the application (best case)?

Sorry, the point I was trying to say here is that till now the flang script will parse, unparse and call an external compiler to do the codegen/generate executable. With this change the flang driver will not generate an executable unless it is built with an option. This default behaviour might be seen as a regression from the point of view of a flang script user.

With this change the flang driver will not generate an executable unless it is built with an option.

Okay, thank you for the clarification. Is this a cmake option and? Are we documenting this somewhere except this patch summary, like on the flang documentation or something? Apologies if this a repetition to what you meant by clarifying that it is available via a flag.

I think this is an exciting step. I hope it gets approved. Although it's technically true that this could appear to be a regression for current flang script users, the ultimate compilation currently happens by invoking an external compiler so most current flang script users can eliminate the regression by simply calling the external compiler. The exception would be if the current flang script user specifically wants flang's parsing capabilities for checking code correctness (do we know if anyone is using the flang script that way?) or for testing the parser (which I presume can still happen by other means). It might be nice to simply rename the current script so those who want it can simply change the name they use to invoke it.

ktras added a subscriber: ktras.May 18 2022, 3:36 PM

With this change the flang driver will not generate an executable unless it is built with an option.

Okay, thank you for the clarification. Is this a cmake option and? Are we documenting this somewhere except this patch summary, like on the flang documentation or something? Apologies if this a repetition to what you meant by clarifying that it is available via a flag.

I think it is a driver flag -flang-experimental-exec. I was only requesting the add this information to the patch summary. If you feel it should be present somewhere else then please add a separate comment.

Some users might have been blindly running flang before without being aware that the script was calling an external compiler to generated code/executable. This change introduces the lowering flow and the users might see different results,
-> TODO messages for 2003 and above features.
-> Unexpected ICEs or miscomparisons.
-> Not producing an executable.

I think this is an exciting step. I hope it gets approved. Although it's technically true that this could appear to be a regression for current flang script users, the ultimate compilation currently happens by invoking an external compiler so most current flang script users can eliminate the regression by simply calling the external compiler. The exception would be if the current flang script user specifically wants flang's parsing capabilities for checking code correctness (do we know if anyone is using the flang script that way?) or for testing the parser (which I presume can still happen by other means). It might be nice to simply rename the current script so those who want it can simply change the name they use to invoke it.

Yes, the user can use flang-to-external-fc if they want the old flow. But this change might be a surprise for the user hence the request to add the relevant information to the summary.

Update documentation

awarzynski edited the summary of this revision. (Show Details)May 19 2022, 10:48 AM

Summary updated :)

I think it is a driver flag -flang-experimental-exec. I was only requesting the add this information to the patch summary. If you feel it should be present somewhere else then please add a separate comment.

The flag is deliberately not being "advertised" - that was the compromise that we agreed on in https://reviews.llvm.org/D122008. Personally, I would really like to document it and make it more visible, but I don't want anyone to feel that their reservations are being ignored.

Some users might have been blindly running flang before without being aware that the script was calling an external compiler to generated code/executable.

A while back we added documentation to clarify this. The bash script (which is the most confusing part) is documented here (that's being updated here). I'm not sure whether there's anything else that we could improve 🤔.

I think this is an exciting step.

Thank you!

Although it's technically true that this could appear to be a regression for current flang script users, the ultimate compilation currently happens by invoking an external compiler so most current flang script users can eliminate the regression by simply calling the external compiler.

I agree :)

My belief is that we should wait until f18 hits a reasonable quality bar for executables before making flang-new the default. My reasoning seems not to have carried the day.

My belief is that we should wait until f18 hits a reasonable quality bar for executables before making flang-new the default. My reasoning seems not to have carried the day.

It would hep to have a well-defined quality bar, e.g., a standards-based test suite or a collection of application test suites or the progress bar on the relevant GitHub project pages. We could then discuss and vote on which point on the quality bar suffices.

My proposal is:

If the compiler compiles it, it ought to run.
If the compiler can't compile it, it ought to clearly say why.

  1. All tests of legal Fortran that compile & link must also execute correctly (which excludes tests that expect to catch a problem at runtime)
  2. For all tests with unsupported features, the compiler must issues an error message and the message references the source-location of the unsupported feature

My preference is to use the NAG test suite. It is not freely available.

My proposal is:

If the compiler compiles it, it ought to run.
If the compiler can't compile it, it ought to clearly say why.

  1. All tests of legal Fortran that compile & link must also execute correctly (which excludes tests that expect to catch a problem at runtime)
  2. For all tests with unsupported features, the compiler must issues an error message and the message references the source-location of the unsupported feature

My preference is to use the NAG test suite. It is not freely available.

I tested a lot of test cases (mostly Fortran 95 code) and have several detailed questions about the reasonable quality bar.

  1. Should some incorrect execution results be changed into one TODO if there is no plan to support it soon? One example is derived type array in forall (https://github.com/flang-compiler/f18-llvm-project/issues/1598).
  1. What if the case fails in lowering with a "fatal internal error", but the real reason is the incorrect semantic analysis? Should it either be supported, or be turned from the "fatal internal error" into TODO?
  1. What about the edge cases? For those with incorrect execution results, turn them into TODO? One example is D125632.
  1. What about some dangerous usage? Usually, gfortran can report a lot of warnings for the dangerous scenarios such as string length mismatch, external procedure type kind mismatch, the test case in D125891, etc. Maybe this is not in priority.
  1. Are the rules also applied to Fortran 2003, 2008 and 2018 code, or only restricted to Fortran 95 code?

My proposal is:

If the compiler compiles it, it ought to run.
If the compiler can't compile it, it ought to clearly say why.

  1. All tests of legal Fortran that compile & link must also execute correctly (which excludes tests that expect to catch a problem at runtime)
  2. For all tests with unsupported features, the compiler must issues an error message and the message references the source-location of the unsupported feature

My preference is to use the NAG test suite. It is not freely available.

I strongly suspect that you might be able to win over some of the folks by sharing the status/list of failures/list of issues to be fixed for getting the NAG test suite to pass. As I have mentioned before, at least the companies involved have access to the NAG testsuite. The community can probably help you achieve the outlined goals faster. It can also help focus our efforts. Some of our engineers have now worked with f18/flang for some time. While some issues might require a fix by the experts, many can be fixed by others.

A separate issue we have is that it is not clear which branch is it that is being used for testing with NAG. Since many fixes have landed in llvm-project/flang, I assume that fir-dev is now out of date. While most of upstreaming is complete there is still a bit of things left to upstream, should not take much time though.

+1 @kiranchandramohan

I have access to the NAG Fortran compiler and find it very helpful for checking standard conformance. I was unaware of the NAG test suite and don't see any mention of it online. Is it an official product that can be purchased?

Recommend that you contact NAG for more info on the test suite. Here's the only reference that I could find: https://fortran-lang.discourse.group/t/fortran-compiler-testing-framework/1573/10

Thank you all for your comments and apologies for going radio silent - I was away for a few days.

I've identified 2 threads emerging from your comments:

1. flang vs flang-new
@sscalpone, if I understand correctly, you are suggesting that any tool named flang in "LLVM Flang" should satisfy the following condition:

  1. For all tests with unsupported features, the compiler must issues an error message and the message references the source-location of the unsupported feature

Indeed, flang-new does not satisfy this yet, but neither does flang, the bash wrapper script. flang, the bash script, calls flang-new to run parsing and semantic analysis. Basically, it's the same Fortran frontend as far as these initial phases are concerned. So, if "LLVM Flang" is missing diagnostics for e.g. unsupported language features, both flang and flang-new will fail to report these. I don't see how defaulting to flang (the bash script) instead of flang-new (the compiler driver) helps here.

Put differently, if the above condition is to be considered a requirement for any tool called flang, then the bash wrapper script should be renamed as it does not satisfy it.

2. NAG test suite
From what I recall, we did discuss renaming flang-new as flang in the past and it was decided that we would wait for flang-new to be able to generate executables. I couldn't find this written down, so perhaps I'm misremembering? In any case, I don't follow why a test-suite conformance should determine what name is used for a compiler driver in "LLVM Flang". We should definitely work towards 100% pass rate, but to me that's orthogonal to how our tools are called. The level of Language support in "LLVM Flang" should be communicated through the documentation (including release notes) instead of misleading tool names.

As for NAG test-suite specifically:

My preference is to use the NAG test suite. It is not freely available.

If we take this path, we will effectively be introducing yet another dependency on an external project in "LLVM Flang" (libpgmath being the first one). While most (perhaps all?) salaried "LLVM Flang" developers will have access to the NAG test suite, community volunteers will effectively have their ability to contribute severely limited. I'm against this, but also don't have a counter-proposal (I'm not aware of any freely accessible Fortran test suites that we could use here).

@peixin, you raised some really good points, thank you! I agree that even if it is decided that the NAG test suite is used as the benchmark for "LLVM Flang", it's still unclear how to deal with all these tricky corner cases (there will be platform dependencies too). As for the following one:

  1. Are the rules also applied to Fortran 2003, 2008 and 2018 code, or only restricted to Fortran 95 code?

I think that we should limit ourselves to Fortran 95 for now - there's no code-gen support beyond that.

NEXT STEPS
Based on the above, how would you feel about the following:

  1. Lets rename flang as flang-to-external-fc regardless of what's decided for flang-new. That would already be a huge step forward (and would reflect accurately what the bash wrapper script actually does).
  2. As for the NAG test suite, I hope that we can agree not to use it as the condition for renaming flang-new. If it's decided that that's a valid requirement, then I agree with @kiranchandramohan that we should find a way to openly track the progress towards the required pass rate (and how that's defined).

Perhaps we could discuss more in the call today?

I'm fine with removing or renaming the existing flang shell script.

My proposal is not to wait for a 100% pass rate. My proposal is to wait until the messages for unimplemented features are converted to TODOs that reference the program's source line and that legal programs which compile and link will execute without error. In short, what flang compiles should work, and what flang doesn't compile should offer a reasonable message to the user to understand the limitation.

This proposal is not restricted to F95 but to any source that someone might attempt to compile.

On Mon, May 30, 2022 at 2:39 AM Andrzej Warzynski via Phabricator <reviews@reviews.llvm.org> wrote:

  1. Lets rename flang as flang-to-external-fc regardless of what's decided for flang-new. That would already be a huge step forward (and > would reflect accurately what the bash wrapper script actually does).

I support this. I think the proposed new name is better reflects what the current flang script currently does and thereby reduces the likelihood of surprise when a user sees an error message from an external compiler.

  1. As for the NAG test suite, I hope that we can agree not to use it as the condition for renaming flang-new. If it's decided that that's a valid > equirement, then I agree with @kiranchandramohan that we should find a way to openly track the progress towards the required pass rate

(and how that's defined).

I agree.

Perhaps we could discuss more in the call today?

Any news from the call?

Damian

I'm fine with removing or renaming the existing flang shell script.

On Mon, May 30, 2022 at 2:39 AM Andrzej Warzynski via Phabricator <reviews@reviews.llvm.org> wrote:
I support this.

Great, thank you both! "Step 1" merged: https://reviews.llvm.org/D125832. This leaves us with flang as a symlink pointing to flang-to-external-fc, which I will be removing once buildbots are reconfigured: https://reviews.llvm.org/D125796. I'd like this to happen soon.

This proposal is not restricted to F95 but to any source that someone might attempt to compile.

I would really appreciate if we could agree on more specific criteria. IMHO, "any source" leaves too much room for interpretation.

Any news from the call?

Not much - it was a very quiet call (Memorial Day for some folks, ISC 2022 for others, unexpected hiccup with Teams for the rest ). We decided to re-visit in the next call.

Next steps
Just to avoid any confusion:

  • flang, the bash script, will be renamed as flang-to-external-fc (see D125832)
  • flang-new won't be renamed just yet - perhaps we could discuss more in the forthcoming call(s)?

In the call yesterday it was proposed that we add a CMake option that will control the name of the driver. I suggest adding FLANG_USE_LEGACY_NAME:

  • when set to ON, the driver binary will be called flang-new,
  • when set to OFF, the driver binary will be called flang.

With this approach, everyone will be able to select the preferred driver name. Note that:

  • this will affect both the build and installation targets,
  • LLVM's testing infrastructure will automatically choose between flang and flang-new (based on FLANG_USE_LEGACY_NAME),
  • you won't be required to perform any extra steps (in fact, you will be able to continue with your current workflows).

It was requested that the default value is set to ON (i.e. the default name for the driver executable in LLVM Flang will remain flang-new).

Later today I will upload a new version of this patch that implements the above. As for changing the default to flang (i.e. setting the default for FLANG_USE_LEGACY_NAME to OFF), the following remains a blocker:

My proposal is:

If the compiler compiles it, it ought to run.
If the compiler can't compile it, it ought to clearly say why.

  1. All tests of legal Fortran that compile & link must also execute correctly (which excludes tests that expect to catch a problem at runtime)
  2. For all tests with unsupported features, the compiler must issues an error message and the message references the source-location of the unsupported feature

My preference is to use the NAG test suite. It is not freely available.

We did discuss replacing NAG with e.g. SNAP + SPEC, but we run out of time and haven't concluded that part.

As always, please comment if I missed or misread anything!

Next steps
I will update this patch by introducing FLANG_USE_LEGACY_NAME CMake option - it will control the name of the generated driver executable.

awarzynski updated this revision to Diff 435564.Thu, Jun 9, 8:31 AM

Add the FLANG_USE_LEGACY_NAME cmake option that will control the name of the driver executable.

awarzynski edited the summary of this revision. (Show Details)Thu, Jun 9, 8:36 AM

In summary:

If you want to use the updated name, flang, set FLANG_USE_LEGACY_NAME to ON when configuring LLVM Flang.

OFF?

awarzynski edited the summary of this revision. (Show Details)Mon, Jun 27, 9:06 AM

In summary:

If you want to use the updated name, flang, set FLANG_USE_LEGACY_NAME to ON when configuring LLVM Flang.

OFF?

Updated, thanks!

clementval added inline comments.Mon, Jun 27, 9:25 AM
clang/lib/Driver/ToolChain.cpp
189

Why do we need two lines here? Shouldn't we have a single line with the name chosen by the cmake option?

clang/lib/Driver/ToolChains/Flang.cpp
161–165
flang/CMakeLists.txt
411

Maybe add a comment that says what is the legacy name.

flang/test/lit.cfg.py
87

Is this the correct indentation?

awarzynski added inline comments.Mon, Jun 27, 9:50 AM
clang/lib/Driver/ToolChain.cpp
189

That would require making this file "configurable" by CMake. That would be quite an intrusive design change, which IMO we should avoid.

Unless there's some easy way that I'm missing here?

Incorporate @clementval 's suggestions

Overall the patch looks ok from a technical point. Shouldn't we just wait until we can make the permanent renaming so we do not add unnecessary cmake option?

Thank your for reviewing @clementval !

Shouldn't we just wait until we can make the permanent renaming so we do not add unnecessary cmake option?

This was discussed in one of our calls. As we weren't able to agree on any specific time-frames, the CMake route was proposed instead. This was suggested by @sscalpone as an acceptable compromise. From what I recall, there were no objections to this.

flang-new has always been intended as a temporary name for the compiler driver. That's because flang was reserved for the bash script. The bash script has recently been renamed as flang-to-external-fc ( D125832) so we are free to repurpose that name. As I tried to explain earlier, these names (flang-new vs flang) have always been very confusing to people and we are trying to improve and to clarify that, step by step. While this approach is not ideal, it gives us the flexibility to choose our preferred name sooner rather than later. If you feel that we should keep flang-new, you can continue using/building LLVM Flang as you have been so far. If you are ready to update the driver name, the CMake option enables that for you.

We discussed this in our call on Monday and agreed to go ahead provided that this change is technically sound. IIUC, this has now been confirmed:

Overall the patch looks ok from a technical point.

As always, please correct me if I missed or misinterpreted something!

clementval requested changes to this revision.Thu, Jun 30, 2:46 AM
clementval added inline comments.
clang/lib/Driver/ToolChain.cpp
189

This is counter intuitive. We rename flang-new but we add flang-new here. It should be configurable.

This revision now requires changes to proceed.Thu, Jun 30, 2:46 AM
clang/lib/Driver/ToolChain.cpp
189

This is a list of all the valid prefixes of binaries that the driver supports. With this change, an additional one will be supported so I don't think it's an issue to have both flang and flang-new here.

The thing that confuses me is how flang-new works today without this change, given that "flang" is not a suffix of "flang-new"? @awarzynski , do you know why that is? Perhaps the line here is not needed at all? Or is this a bug fix for flang-new that is actually unrelated to this change?

awarzynski added inline comments.Thu, Jun 30, 3:46 AM
clang/lib/Driver/ToolChain.cpp
189

This is counter intuitive.

I can add a comment to clarify this.

It should be configurable.

It is, in Flang's driver.cpp. Originally, the suffix was hard-coded as:

clang::driver::ParsedClangName targetandMode("flang", "--driver-mode=flang");

(i.e. the clangDriver library used flang internally despite the actual name being flang-new). This is now being replaced with (see in "driver.cpp"):

clang::driver::ParsedClangName targetandMode =
     clang::driver::ToolChain::getTargetAndModeFromProgramName(argv[0]);

But the change in "driver.cpp" means that we can no longer make assumptions about the suffix and hence the update here.

Like I mentioned earlier, we should not make this file build-time configurable. One possible option would be to try to update Clang's config.h.cmake, but that's would lead to more Flang-specific changes in Clang's core set-up. Also, I'm not convinced it would work here.

@awarzynski , do you know why that is?

Yeah, check Flang's "driver.cpp". We should've captured this earlier. The original set-up from ToolChain.cpp predates flang-new. And then in "driver.cpp" we just matched what was here. There was a lot of confusion around naming back then and this has slipped in.

clementval added inline comments.Thu, Jun 30, 6:51 AM
clang/lib/Driver/ToolChain.cpp
189

This is counter intuitive.

I can add a comment to clarify this.

There should be at least a comment here.

Like I mentioned earlier, we should not make this file build-time configurable. One possible option would be to try to update Clang's config.h.cmake, but that's would lead to more Flang-specific changes in Clang's core set-up. Also, I'm not convinced it would work here.

If it cannot be configurable I think this is a good reason to wait a bit and make a direct renaming without all the options and duplicate.

We discussed this in our call on Monday and agreed to go ahead provided that this change is technically sound. IIUC, this has now been confirmed:

Overall the patch looks ok from a technical point.

As always, please correct me if I missed or misinterpreted something!

I still have reserved on this patch so please do not take my "Overall the patch looks ok from a technical point." as an approval. There are open discussion so wait for other to confirm or not.

awarzynski updated this revision to Diff 441643.Fri, Jul 1, 2:11 AM

Add a comment