Page MenuHomePhabricator

[flang][docs] Add documentation for the new Flang driver
ClosedPublic

Authored by awarzynski on Jun 14 2021, 7:42 AM.

Diff Detail

Event Timeline

awarzynski created this revision.Jun 14 2021, 7:42 AM
awarzynski requested review of this revision.Jun 14 2021, 7:42 AM
Herald added a project: Restricted Project. · View Herald Transcript

Great to see some docs added Andrzej - this should help us maintain the new Driver in future.

I have swept through with some comments - mostly typographical nits. More diagrams would help to explain the structure of Actions, and perhaps the relationship between the compiler driver and other jobs/binaries it could call.

flang/docs/FlangDriver.md
23

Is that the right word choice? I think it's for Fortran compilation jobs isn't it?

I wonder if "toolchain driver" == flang and "compiler driver" -> flang -fc1 might work better?

45

I think a diagram would really help explain this part.

61
110
116
123

sp? Should it be FlangOption ?

129

... and be marked as FlangOnlyOption

132
133

I guess you do this by adding FlangFc1Option do you?

157

Not sure what this para is trying to say.

Are you suggesting that all tests for flang (i.e. the Fortran compiler part of it) ought to use -fc1 command lines and only tests for compiler driver should invoke the compiler driver? So tests in flang/test/Driver should use %flang -### and all the others use %flang_fc1? If so, do you think that will work?

Or are you trying to teach a newcomer how to add a tests for the compiler driver and for frontend driver options handling specifically? If so, then I think it needs a bit more prescriptive detail.

159
awarzynski marked 7 inline comments as done.Jun 16 2021, 1:52 PM

Thank you for taking a look @richard.barton.arm

More diagrams would help to explain the structure of Actions, and perhaps the relationship between the compiler driver and other jobs/binaries it could call.

I agree, but I'm finding it difficult to come up with something useful. IMO, the diagrams that I created in the past wouldn't be that helpful here. Also, should we be documenting clangDriver here? I've added a short paragraph on flang, but that's meant to be just a quick intro.

Another reason for not getting into too much detail on the other compilation jobs (e.g. backend, assembler, linker) is that Flang does not support them yet. Also, there's a great diagram and more documentation in Clang: https://clang.llvm.org/docs/DriverInternals.html#design-overview.

flang/docs/FlangDriver.md
23

I find this a bit tricky - I've never seen Clang being referred to as a "toolchain driver". LLDB is tool that belongs to the LLVM toolchain, but it's not driven by Clang. On the other hand, Clang does drive the compiler (i.e. Clang & LLVM) and the linker, so it's more than just a compiler driver.

45

I find the output generated with -ccc-print-phases more helpful than any of the diagrams that I've generated so far :) Let me try that instead.

133

No, these flags are not used in this process. They are not used much beyond defining the output of {flang|clang} -help. Also, the semantics for option flags are not well defined, so we shouldn't rely on them too much.

157

I just want to highlight that there are differences between %flang and %flang_fc1. In some cases the observable behavior will be identical, so IMO it's worthwhile to have this written down. I will rephrase.

arnamoy10 added inline comments.Jun 18 2021, 5:50 AM
flang/docs/FlangDriver.md
27

[Nit] easy to use --> easy-to-use

57

[Nit] crates -> creates

154

[Nit] Understood by for the frontend -> Understood by the frontend

157

May want to elaborate what you mean by "Plain command line option", or reword

180

[Nit] In some cases --> In some cases,

185

Not sure if we want to document the use of !REQUIRES here

awarzynski marked 4 inline comments as done.

Address PR comments from @arnamoy10

Also:

  • expanded the section on action options and added some code examples
  • moved a couple of paragraphs to improve the flow.
flang/docs/FlangDriver.md
45

LGTM

70

I thought this was wrong when I first saw it, but I can reproduce. What's flang-new doing here then? I thought there was no support for these phases in flang yet?

153
naromero77 added inline comments.Jun 26 2021, 10:50 PM
flang/docs/FlangDriver.md
139

Grammar... `There are ..."

awarzynski marked an inline comment as done.Jun 28 2021, 2:40 AM
awarzynski added inline comments.
flang/docs/FlangDriver.md
70

Sorry, I should've clarified.

-ccc-print-phases prints what phases clangDriver _would_ generate. That's not something flang-new is in control of. However, when you try to run it without -ccc-print-phases, it fails during the 2nd phase above:

error: code-generation is not available yet

Making -ccc-print-phases a bit more clever would be nice, but it would have to happen in Clang. In the meantime, I will add a clarification here.

139

Thank you, fixed!

157

I will reword this section a bit, thanks!

185

That's a good suggestion, though I will avoid ! REQUIRES: new-flang-driver, as we want to get rid of it.

awarzynski marked an inline comment as done.

Fix grammar, add a note on code-generation (not supported)

naromero77 accepted this revision.Jun 28 2021, 9:00 AM

LGTM, but please check with Rich to make sure all his comments are addressed.

Also, the pre-merge checks are failing on windows but it seems unrelated to this patch.

This revision is now accepted and ready to land.Jun 28 2021, 9:00 AM

Happy with the changes made since my comments.
Thanks Andrzej for the great documentation.

flang/docs/FlangDriver.md
23

ok - let's leave it there.

70

That's a nicer error message than I expected too.

This revision was landed with ongoing or failed builds.Jul 1 2021, 1:15 AM
This revision was automatically updated to reflect the committed changes.