This is an archive of the discontinued LLVM Phabricator instance.

[flang][driver] Extend the `flang` bash script to act as a driver
ClosedPublic

Authored by awarzynski on May 26 2021, 9:01 AM.

Details

Summary

Until now, f18 would:

  1. Use Flang to unparse the input files
  2. Call an external Fortran compiler to compile the unparsed source files (generated in step 1)

With this patch, f18 will stop after unparsing the input source files,
i.e. step 1 above. The flang bash script will take care of step 2,
i.e. calling an external Fortran compiler driver to compile them. This
way:

  • the functionality of f18 is reduced - it will only drive Flang (as opposed to delegating code-generation to an external tool on top of this)
  • we will able to switch between f18 and flang-new for unparsing before an external Fortran compiler is called for code-generation

These changes were discussed in [1] as a requirement for replacing f18
with flang-new.

[1] https://lists.llvm.org/pipermail/flang-dev/2021-April/000677.html

Diff Detail

Event Timeline

awarzynski created this revision.May 26 2021, 9:01 AM
awarzynski requested review of this revision.May 26 2021, 9:01 AM
Herald added a project: Restricted Project. · View Herald Transcript

I tested the updated flang script with SPEC 2017 and it worked fine for me. However, I don't really use the functionality re-implemented here, so I might have missed something. I would really appreciate if you tested your workflows and let me know whether everything works for you as before.

I still need to:

  • remove code for calling external compilers in f18.cpp
  • update error messages in flang
  • make flang switch between f18 and flang-new based on FLANG_BUILD_NEW_DRIVER

I appreciate that in this respect my patch is a bit incomplete, but I will be away in the coming days and was hoping to get some early feedback in the meantime.

Thank you!

I'm not clear why you plan to remove code from f18.cpp.

I'm not clear why you plan to remove code from f18.cpp.

Otherwise flang, the bash script, duplicates functionality available in f18.cpp. The goal is to move that functionality to flang.

A concern that I have is that the bash script will only on platforms that have bash. In addition to Windows, some distribution use different default shells and may not have it installed (MacOS uses zsh and ships only an old version of bash).
However, as long as this is transitional, and to be removed again once the flang toolchain is fully functional, I would be ok with it.

Otherwise flang, the bash script, duplicates functionality available in f18.cpp. The goal is to move that functionality to flang.

I think the argument is that f18.cpp will be removed completely, not necessary to remove specific functionality before that.

A concern that I have is that the bash script will only on platforms that have bash.

Thanks for bringing this up, I share your concerns. Our goal is to get code generation in llvm-project/flang as soon as possible. Once that's available, we shouldn't need this script and I will be very much of favor of removing it. If it offers a functionality that we'll need long-term, then that functionality should be identified and re-implemented in something platform agnostic (e.g. C++ or Python). For me, this patch is here to facilitate the transition from f18 to flang-new as the default driver.

Otherwise flang, the bash script, duplicates functionality available in f18.cpp. The goal is to move that functionality to flang.

I think the argument is that f18.cpp will be removed completely, not necessary to remove specific functionality before that.

Yes, that's the end goal. I guess that I'm focusing more on the intermediate steps.

Remove code from f18.cpp

All code that dealt with code-generation in f18.cpp has been removed. The flang script has been extended - it can now compile non-Fortran sources. Also, error messages in the bash script have been updated.

I also had to slightly update the semantics of -funparse-typed-exprs-to-f18-fc and replace it with -fno-unparse-typed-exprs-to-f18-fc. This way I was able to preserve the original behaviour of flang (in general) and f18 (when using -fdebug-unparse).

I still don't understand why you have to mess with f18.cpp. What would break if you left it alone?

I'm not clear why you plan to remove code from f18.cpp.

Even if we decide that it's in our long-term interests to remove this code, it would be better to have a period of transition where we are able to transition to the new functionality while retaining the old as a fallback.

I still don't understand why you have to mess with f18.cpp. What would break if you left it alone?

I'm still new here and don't know the whole history, but I think this is meant to be removed eventually, so removing it in pieces seems unnecessary.

Is there any reason we can't remove it immediately? In a separate patch perhaps...
Is there some reason it needs to remain in the source-tree once there's code-generation working with the new driver?

Switch between flang-new and f18, depending on the value of FLANG_BUILD_NEW_DRIVER

With this latest update, flang will use flang-new when the new driver is enabled. As previously, I tested this with SPEC 2017 (as well as some trivial examples) and it worked fine for me.

Some extra changes in flang-new were required. I extracted them into seperate patches: D103612, D103613.

@klausler , @PeteSteinfeld , thank you for your comments! I'm happy to reduce the changes in f18.cpp to the required minimum. Trimming it "bit-by-bit" seemed like a good idea, but we can move this to a separate patch if that's preferable. I'll send an update shortly!

@Leporacanthicus, yes, our end goal is to replace f18 with flang-new and this means deleting f18.cpp. With this patch, f18 and flang-new become equivalent in terms of functionality and, IIUC, we are ready to delete f18. I'm not aware of any reasons to keep it in-tree long term. Perhaps others could comment?

@PeteSteinfeld, what would be a good transition period? Any suggestions? Is there anything in particular that we could test in the meantime?

@klausler , @PeteSteinfeld , thank you for your comments! I'm happy to reduce the changes in f18.cpp to the required minimum. Trimming it "bit-by-bit" seemed like a good idea, but we can move this to a separate patch if that's preferable. I'll send an update shortly!

@Leporacanthicus, yes, our end goal is to replace f18 with flang-new and this means deleting f18.cpp. With this patch, f18 and flang-new become equivalent in terms of functionality and, IIUC, we are ready to delete f18. I'm not aware of any reasons to keep it in-tree long term. Perhaps others could comment?

@PeteSteinfeld, what would be a good transition period? Any suggestions? Is there anything in particular that we could test in the meantime?

What's the timeline for removing the dependency of flang-new on clang?

@awarzynski, I'm not sure what a good transition period would be. Actually, I don't see the benefit of removing any functionality from f18. This might be a good topic to bring up at the Wednesday f18 meeting.

What's the timeline for removing the dependency of flang-new on clang?

We don't have a timeline for this, but it will be non-trivial amount of effort.
We discussed the dependency on Clang in our community calls recently. I was under the impression that we agreed that this dependency is a fair compromise for what we are getting in return. At least in the interim.

Actually, I don't see the benefit of removing any functionality from f18.

The new driver is developed to replace f18. In terms of functionality, flang-new and f18 are currently equivalent. However, once we start supporting code-generation, flang-new will be able to leverage clangDriver for support for different tool-chains and operating systems. In f18 we would have to implement this from scratch. Also, the new frontend driver is designed to allow for frontend plugins (akin to plugins in Clang). Again, that's something that would take a lot of effort to implement in f18. Also, flang-new is already more user-friendly than f18:

flang-new -fc1 -random-flag -fdebug-unparse file.f
error: unknown argument: '-random-flag'

This behavior is consistent across all options (no matter the semantics) and it's something that we get for "free" from clangDriver. In comparison, f18 happily ignores unknown flags:

f18 -random-flag -fdebug-unparse file.f

On a different note,

This might be a good topic to bring up at the Wednesday f18 meeting.

Sounds like a good idea! Also, as mentioned previously, we don't need to remove any functionality from f18 in this patch. All changes to f18.cpp have been reverted and we can focus on the bash script instead.

awarzynski updated this revision to Diff 349883.Jun 4 2021, 8:33 AM

Rebase on top of D103612

We don't have a timeline for this, but it will be non-trivial amount of effort. We discussed the dependency on Clang in our community calls recently. I was under the impression that we agreed that this dependency is a fair compromise for what we are getting in return. At least in the interim.

That's great for your community, and I appreciate the update on your activity, but I'd like to avoid the clang dependence for what I'm doing, if I can. I'd prefer that f18.cpp stay in your tree but it's not the end of the world for me if you remove it.

I'd like to avoid the clang dependence for what I'm doing, if I can.

Do you require full driver support for what you do? Perhaps we could extract a smaller tool from f18 that would meet your needs and be independent of Clang? For now I'm just thinking out loud, but this might be an option.

We discussed f18 in the call yesterday and agreed to remove it in the near future. I've summarized it here: https://lists.llvm.org/pipermail/flang-dev/2021-June/000742.html. We will have this patch merged first and then wait for at least 2 weeks. Please let me know if you need more time or discover something that this would break for you.

I'd like to avoid the clang dependence for what I'm doing, if I can.

Do you require full driver support for what you do? Perhaps we could extract a smaller tool from f18 that would meet your needs and be independent of Clang? For now I'm just thinking out loud, but this might be an option.

We discussed f18 in the call yesterday and agreed to remove it in the near future. I've summarized it here: https://lists.llvm.org/pipermail/flang-dev/2021-June/000742.html. We will have this patch merged first and then wait for at least 2 weeks. Please let me know if you need more time or discover something that this would break for you.

I don't know what "smaller tools" you could extract from the f18 tool that would be useful, but thanks for the offer. The current f18 is still a good fit for my own needs (as one would expect) and I'd keep it in my tree if you delete it from yours.

The source for the f18 and f18-parse-demo tools might be useful to hypothetical future tool-writers as a minimal example of what one needs to do to drive the parser and later phases in a stand-alone environment for purposes other than Fortran compilation (e.g., pretty-printing, source-to-source transformations, program analysis); is your community still interested in such things?

Overall I support & I'd like to have one driver i.e flang-new considering all the effort that has gone in it and the long term plan associated with the driver(that will cater to the entire flang project as a primary interface). :)

I can observe/note following benefits of having one driver:

  • Keeping things consistent/converge on flang-new will help in synchronizing the effort of the community. This should help anyone who wants to contribute, not to get intimidated by presence of 2 drivers(and implied effort of keeping the feature parity across both).
  • flang-new just now gained the feature parity with existing driver f18 (Thanks to @awarzynski and other folks involved :)), from now onwards either one has to bear the cost of maintenance & NOT breaking each other while progressing.
  • Speaking of the dependency on clang, I think this will remain and if people don't appreciate it - then this will require entire re-write, non-trivial effort, and needs a solid motivation to move forward along this path. NOT to mention, this entire activity may end up eating most (if not all) efforts that were planned to get the driver functional/usable to a point :)

On the path forward, If people have interest in having f18 (which I can see from above conversation), then at least we can mutually agree upon some clear timelines(2-3 weeks ?) as to when f18 is Okay to remove completely.
This should help in synchronizing folks from both sides :)

Overall I support & I'd like to have one driver i.e flang-new considering all the effort that has gone in it and the long term plan associated with the driver(that will cater to the entire flang project as a primary interface). :)

I can observe/note following benefits of having one driver:

  • Keeping things consistent/converge on flang-new will help in synchronizing the effort of the community. This should help anyone who wants to contribute, not to get intimidated by presence of 2 drivers(and implied effort of keeping the feature parity across both).
  • flang-new just now gained the feature parity with existing driver f18 (Thanks to @awarzynski and other folks involved :)), from now onwards either one has to bear the cost of maintenance & NOT breaking each other while progressing.
  • Speaking of the dependency on clang, I think this will remain and if people don't appreciate it - then this will require entire re-write, non-trivial effort, and needs a solid motivation to move forward along this path. NOT to mention, this entire activity may end up eating most (if not all) efforts that were planned to get the driver functional/usable to a point :)

On the path forward, If people have interest in having f18 (which I can see from above conversation), then at least we can mutually agree upon some clear timelines(2-3 weeks ?) as to when f18 is Okay to remove completely.
This should help in synchronizing folks from both sides :)

That's fine with me; as I've said, I don't need to have f18.cpp in your tree to be able to work on my parts of the project. Hypothetical future Fortran tool-writers who just want a parser & maybe semantics can reconstruct it when/if needed, I suppose.

Make sure that -fc1 is the first option that flang-new sees

-fc1 is not a regular option and its semantics require that it's the first option that the driver parses. This revision updates flang accordingly.

Leporacanthicus accepted this revision.Jun 17 2021, 4:26 AM

Some minor nit-picks, but nothing wrong - I tested this with some of my fortran code that I've been using for other testing, and it seems to work.

flang/tools/f18/CMakeLists.txt
70

Nit: wouldn't a single blank line be good enough?

flang/tools/f18/flang.in
230

nit: driver

This revision is now accepted and ready to land.Jun 17 2021, 4:26 AM
awarzynski marked an inline comment as done.

Address nits from @Leporacanthicus

Thank you for reviewing, Mats! This patch depends on D103612, so I need to have that one approved before I can merge this.

awarzynski marked an inline comment as done.Jun 21 2021, 5:05 AM

Rebase on top of main + minor update

I updated f18 so that it understands f18 -fdebug-unparse -o <output_file>. The updated flang script relies on this behaviour. Before, f18 would always dump to stdout (with or without -o <output_file>).

Make sure that flang exits early when -E is used

SPEC 2017 still builds and runs fine.

awarzynski edited reviewers, added: naromero77; removed: gromer.

@sscalpone , thank you for the suggestion on Monday (i.e. to test this change with Fortran Tests in the LLVM Test Suite). While working on it, I discovered that the integration of Flang with CMake is currently not working. I've brought this up on flang-dev for better visibility. As the test-suite requires CMake support, we cannot run it using flang (_before_ or_ after_ this change).

If there are no further comments, I will proceed and merge this change tomorrow. Thank you all for the discussion and for reviewing!

awarzynski updated this revision to Diff 355818.Jul 1 2021, 3:29 AM

Make shellcheck happy

DavidSpickett added a subscriber: DavidSpickett.EditedJul 2 2021, 1:38 AM

@awarzynski We have some fortran failures in the test suite after this commit:
https://lab.llvm.org/buildbot/#/builders/184/builds/109

For example:

******************** TEST 'test-suite :: Fortran/UnitTests/fcvs21_f95/FM363.test' FAILED ********************
/home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/tools/timeit-target --limit-core 0 --limit-cpu 7200 --timeout 7200 --limit-file-size 104857600 --limit-rss-size 838860800 --append-exitstatus --redirect-output /home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/UnitTests/fcvs21_f95/Output/FM363.test.out --redirect-input /dev/null --chdir /home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/UnitTests/fcvs21_f95 --summary /home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/UnitTests/fcvs21_f95/Output/FM363.test.time /home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/UnitTests/fcvs21_f95/FM363
cd /home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/UnitTests/fcvs21_f95 ; /home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/tools/fpcmp-target /home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/UnitTests/fcvs21_f95/Output/FM363.test.out FM363.reference_output

/home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/tools/fpcmp-target: FP Comparison failed, not a numeric difference between 'F' and ' '

Not totally sure this is the cause but it's the only fortran related commit. Some fp related option being dropped or re-ordered?

Oh and that bot is running on SVE hardware but I don't think it's that, it just happens to be the only flang bot that has the test suite enabled.

rovka added a subscriber: rovka.Jul 2 2021, 2:01 AM

@awarzynski We have some fortran failures in the test suite after this commit:
https://lab.llvm.org/buildbot/#/builders/184/builds/109

For example:

******************** TEST 'test-suite :: Fortran/UnitTests/fcvs21_f95/FM363.test' FAILED ********************
/home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/tools/timeit-target --limit-core 0 --limit-cpu 7200 --timeout 7200 --limit-file-size 104857600 --limit-rss-size 838860800 --append-exitstatus --redirect-output /home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/UnitTests/fcvs21_f95/Output/FM363.test.out --redirect-input /dev/null --chdir /home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/UnitTests/fcvs21_f95 --summary /home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/UnitTests/fcvs21_f95/Output/FM363.test.time /home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/UnitTests/fcvs21_f95/FM363
cd /home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/UnitTests/fcvs21_f95 ; /home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/tools/fpcmp-target /home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/UnitTests/fcvs21_f95/Output/FM363.test.out FM363.reference_output

/home/tcwg-buildslave/worker/clang-aarch64-sve-vls/test/sandbox/build/tools/fpcmp-target: FP Comparison failed, not a numeric difference between 'F' and ' '

Not totally sure this is the cause but it's the only fortran related commit. Some fp related option being dropped or re-ordered?

Oh and that bot is running on SVE hardware but I don't think it's that, it just happens to be the only flang bot that has the test suite enabled.

Hi David, clang-aarch64-full-2stage also has the test-suite enabled, is that one passing?
Sometimes that error message comes up in the test-suite when the executable prints an error message instead of the expected output. Do you think you could dig out the actual output of the test?
Thanks!

@awarzynski We have some fortran failures in the test suite after this commit:
https://lab.llvm.org/buildbot/#/builders/184/builds/109

Thanks for reporting this @DavidSpickett !

Not totally sure this is the cause but it's the only fortran related commit. Some fp related option being dropped or re-ordered?

Yes, it looks like it's related, but it's not immediately obvious how exactly! I've tried testing this patch with LLVM's test-suite, but CMake does not support LLVM Flang yet: https://lists.llvm.org/pipermail/flang-dev/2021-June/000745.html. I've asked around and AFAIK, no one yet has been able to use LLVM Flang with the test-suite. So I'm not sure how people managed to set-up that BuildBot worker to work :)

Oh and that bot is running on SVE hardware but I don't think it's that, it just happens to be the only flang bot that has the test suite enabled.

There is another one here: https://lab.llvm.org/buildbot/#/builders/179. Oddly, it doesn't complain. Also, @rovka kindly tested this patch using that worker's config before it was merged.

So, any clue what the difference between the two workers is?

DavidSpickett added a comment.EditedJul 2 2021, 2:52 AM

So, any clue what the difference between the two workers is?

One tries to enable SVE code, that's it.

Edit: Well, the 1 stage sve just sets cpu to a64fx, the 2 stage tries to vectorise the second stage build.

Hi David, clang-aarch64-full-2stage also has the test-suite enabled, is that one passing?

Yknow, it is, but it was before. I don't see why it turned green the only commit in the next build is unrelated.

Leave this with me, I'll see if the next builds also fail.

I've tried testing this patch with LLVM's test-suite, but CMake does not support LLVM Flang yet

Yeah I don't see any reference to flang bar 1 comment line in the test suite. I see references to nagfortran and f2c in Makefile.FORTRAN. Perhaps we installed one of those in the buildbot image accidentally.

flang/tools/f18/f18.cpp