[flang] Adding a guideline for flang design documentation

[jeanPerier]

Summary

The goal of this document is to:

• Allow the community to contribute to flang design by defining the expectations.
• Ensure there is a minimum of consistency between future design docs.

[kiranchandramohan]

Thanks @jeanPerier for creating this guideline.

Do you want to clarify whether the scope of this document is limited to Lowering (parse-tree to MLIR, FIR, FIR transformations, FIR to LLVM)? Or is this a general guideline for all of Flang including for eg. Semantics, Runtime, OpenMP, OpenACC etc?
Would it be good to mention how a person can find what is a feature to pick up? For the intrinsics work we have a spreadsheet, would it make sense to have a spreadsheet or projects page in github? This page can have unimplemented features from Fortran 95, 2003 etc.

[kiranchandramohan]

flang/docs/DesignGuideline.md

25

Should this have end to end tests? If so should we think of setting up a mechanism to run end-to-end tests?

75

Is there are preference for compatibility with gfortran over other fortran compilers?

87

And any transformations in between? Also does this FIR Op need to be present while lowering to LLVM or would it have been converted to FIR or other ops?

[awarzynski]

flang/docs/DesignGuideline.md

18

Can you clarify what you mean by "a new feature"? What changes are expected to come with a design doc?

31–33

IMHO, we should make sure that every new design doc is well advertised, so that everyone who is interested can participate in the discussion. In this respect, Discourse is much more inclusive than Phabricator and posting there should be required rather than optional (as suggested in the following paragraph).

[unterumarmung]

Thank you for the guideline, this should be really helpful!

flang/docs/DesignGuideline.md

30–33

So, one should create a design doc first, merge it to the repository after necessary approvals and only then start to work on the feature?
I'm with @awarzynski on this. I think it's more agile to first create a discourse discussion with RFC and/or design doc and then, after a mutual agreement that a feature is needed, start to work on the feature and the final design doc in the same revision.

[tarunprabhu]

I'd like to second @kiranchandramohan on having a document for features if possible. Also, as @awarzynski said, it would be good to get a sense of what features require a design document.

Should this document be kept up to date as the feature is implemented and its design (presumably) evolves? Or is it essentially an RFC? In other words, once the feature is implemented and merged, in this document intended to serve as an overview of the implementation of the feature? Or will it become redundant once the feature is implemented and merged?

[clementval]

flang/docs/DesignGuideline.md

30–33

I think discussion on Discourse are nice but I find it hard to summarize if you come later in the discussion or even after it. IMO The document is a better place to have the summary of what is agreed on. To get it it in tree phab is the only way. What about make a phab review for the document and advertise it in a Discourse post?

[rovka]

flang/docs/DesignGuideline.md

21

Do we have any examples in tree already?

70

73

82

[jeanPerier]

In D130166#3665793, @kiranchandramohan wrote:

Do you want to clarify whether the scope of this document is limited to Lowering (parse-tree to MLIR, FIR, FIR transformations, FIR to LLVM)? Or is this a general guideline for all of Flang including for eg. Semantics, Runtime, OpenMP, OpenACC etc?

I wrote it with Lowering in mind because that is what I am working on, but I think it is generic guideline and up to the code owner of the different part to enforce it. @kiranchandramohan What is your take for OpenMP, is this how you want people to work ?

In D130166#3665793, @kiranchandramohan wrote:

Would it be good to mention how a person can find what is a feature to pick up? For the intrinsics work we have a spreadsheet, would it make sense to have a spreadsheet or projects page in github? This page can have unimplemented features from Fortran 95, 2003 etc.

A Github project page sounds like a great idea to me. I have not created any yet, and I see that there are now two options (beta and classic). @kiranchandramohan Do you have an opinion on which one we should use.

In D130166#3665985, @tarunprabhu wrote:

I'd like to second @kiranchandramohan on having a document for features if possible. Also, as @awarzynski said, it would be good to get a sense of what features require a design document.

Right, I think a github project list would be the best here.

Should this document be kept up to date as the feature is implemented and its design (presumably) evolves? Or is it essentially an RFC? In other words, once the feature is implemented and merged, in this document intended to serve as an overview of the implementation of the feature? Or will it become redundant once the feature is implemented and merged?

It should not be a documentation of the implementation with the exact class names and interfaces... It should justify the solution, and explain some choices/rejected alternatives. So it is not superseded by the implementation. And yes, it should be updated if the solution is changed, if the solution evolves, the rational for it should be kept up to date (hence the small section about "Updating the implementation solution of a feature").

flang/docs/DesignGuideline.md

18

I think this should be something identify as a feature on the list of feature that Kiran mentioned we should have.
I will add a link to the related (probably) github project.

25

Yes, I think it would be great if we could add end-to-end feature tests in the llvm test suite.

Would it make sense if we asked people to create a folder for the feature in https://github.com/llvm/llvm-test-suite/tree/main/Fortran/UnitTests and add their end to end tests there ?

In my opinion though, the testing plan may include running/benchmarking applications that may not be easily added there, and documenting the results might be enough for those.

30–33

So, one should create a design doc first, merge it to the repository after necessary approvals and only then start to work on the feature?
I'm with @awarzynski on this. I think it's more agile to first create a discourse discussion with RFC and/or design doc and then, after a mutual agreement that a feature is needed, start to work on the feature and the final design doc in the same revision.

No, the guideline tells that you can first discuss on discourse if you want to, but I do not think we should mandate the discussions to happen on discourse if the person working on the feature prefers the discussions to happen as a design document review. Although, I agree with @awarzynski point that discourse is more visible, and it makes sense to me to require people to at least advertise those in discourse.

30–33

So, one should create a design doc first, merge it to the repository after necessary approvals and only then start to work on the feature?
I'm with @awarzynski on this. I think it's more agile to first create a discourse discussion with RFC and/or design doc and then, after a mutual agreement that a feature is needed, start to work on the feature and the final design doc in the same revision.

75

I do not want to make it a general rule to avoid inciting people to just do the same as gfortran ABI without thinking it through. But if being compatible with gfortran does not come at extra cost, then I would say it is probably the safe choice.

87

That is a good point thanks, I will add it.

[awarzynski]

flang/docs/DesignGuideline.md

25

Would it make sense if we asked people to create a folder for the feature in https://github.com/llvm/llvm-test-suite/tree/main/Fortran/UnitTests and add their end to end tests there ?

IMO, it would. But I would also ask the wider community first (that's outside the Flang sub-project). @Meinersbur helped a lot adding the existing Fortran tests there - could you chime in? More importantly, how do we make sure that any new tests in llvm-test-suite are actually run by one of the bots?

In my opinion though, the testing plan may include running/benchmarking applications that may not be easily added there, and documenting the results might be enough for those.

So this is not unit testing, is it? It's more like a validation plan. This would make perfect sense for optimisations (to demonstrate the benefits), but what about new features that can be tested with regular LIT tests. It would be good to clarify this point. In particular, testing in LLVM is fairly well documented. What additional testing would you require?

30–33

I suggest that:

1. We require all new proposal/design-docs to be advertised on Discourse.
2. People posting new proposals decide for themselves whether they prefer the discussion to continue on Discourse or Phabricator.

Basically, we let people decide what works for them best.

75

I do not want to make it a general rule to avoid inciting people to just do the same as gfortran ABI without thinking it through.

That's a good point, but I think that we also want to avoid a situation in which some parts of "LLVM Flang" align with GFortran, something else with "Classic Flang" etc. To me, it's either "align with GFortran" or "do something custom". Consulting other compilers is important and should be taken into account, but I would still prioritise GFortran.

This is what Clang folks did in the early days. IIRC, clang used to be a drop-in replacement for gcc for a while. Then, gradually, they started diverging.

[awarzynski]

In D130166#3667742, @jeanPerier wrote:

In D130166#3665793, @kiranchandramohan wrote:

Do you want to clarify whether the scope of this document is limited to Lowering (parse-tree to MLIR, FIR, FIR transformations, FIR to LLVM)? Or is this a general guideline for all of Flang including for eg. Semantics, Runtime, OpenMP, OpenACC etc?

I wrote it with Lowering in mind because that is what I am working on, but I think it is generic guideline and up to the code owner of the different part to enforce it. @kiranchandramohan What is your take for OpenMP, is this how you want people to work ?

Currently there are two code-owners in LLVM Flang: @sscalpone and @schweitz (from CODE_OWNERS.TXT). Should it be updated?

[kiranchandramohan]

In D130166#3667742, @jeanPerier wrote:

In D130166#3665793, @kiranchandramohan wrote:

Do you want to clarify whether the scope of this document is limited to Lowering (parse-tree to MLIR, FIR, FIR transformations, FIR to LLVM)? Or is this a general guideline for all of Flang including for eg. Semantics, Runtime, OpenMP, OpenACC etc?

I wrote it with Lowering in mind because that is what I am working on, but I think it is generic guideline and up to the code owner of the different part to enforce it. @kiranchandramohan What is your take for OpenMP, is this how you want people to work ?

I like the idea of good design docs. We should have something like this for OpenMP as well. The only difficulty is that the OpenMP dialect and OpenMP IRBuilder sit outside the flang tree and can be worked on independently. So maybe any such discussions can be in discourse but before implementation in Flang (parse-tree to OpenMP + FIR dialects) we can make this a requirement for OpenMP as well subject to agreement of this proposal and consensus among OpenMP team members.

In D130166#3665793, @kiranchandramohan wrote:

Would it be good to mention how a person can find what is a feature to pick up? For the intrinsics work we have a spreadsheet, would it make sense to have a spreadsheet or projects page in github? This page can have unimplemented features from Fortran 95, 2003 etc.

A Github project page sounds like a great idea to me. I have not created any yet, and I see that there are now two options (beta and classic). @kiranchandramohan Do you have an opinion on which one we should use.

I have been using Classic mostly. No issues with beta as well but have not spent time understanding it.

flang/docs/DesignGuideline.md

30–33

For me, advertising in discourse should be made compulsory so that people are aware.

75

It might be good to clarify this point further. Given that the descriptor, module format, etc., are not compatible, exactly what compatibility are we referring to here?

GCC has a dominant position in the C/C++ world, but I am not sure about the prominence of Gfortran in the Fortran world. A lot of our community comes from Classic Flang/nvfortran. It will be good to check compatibility there as well.

[jeanPerier]

In D130166#3668764, @kiranchandramohan wrote:

I like the idea of good design docs. We should have something like this for OpenMP as well. The only difficulty is that the OpenMP dialect and OpenMP IRBuilder sit outside the flang tree and can be worked on independently. So maybe any such discussions can be in discourse but before implementation in Flang (parse-tree to OpenMP + FIR dialects) we can make this a requirement for OpenMP as well subject to agreement of this proposal and consensus among OpenMP team members.

Good point, what about adding:

Features impacting projects outside of flang (like work OpenMP or OpenACC that may require touching parts outside of flang tree) should follow the general LLVM process, or the related subproject process. There should still be a related flang design docs if part of the solution impacts flang in significant way (e.g. it involves changes in the lowering from the parse-tree to OpenMP or FIR dialects that are not simply about mapping a parse-tree node to a related dialect operation).

In D130166#3668723, @awarzynski wrote:

Currently there are two code-owners in LLVM Flang: @sscalpone and @schweitz (from CODE_OWNERS.TXT). Should it be updated?

Right, not my call, I think it is probably fine as it is. I turned to Kiran as my OpenMP point of contact, any official code owner is of course welcome to speak up here :) I guess that given the codeowners, and Kiran's point, this document should apply to all feature changes where significant choices have to be made in flang tree. Basically, everything that will have a task with the "design" label under the future flang related subproject should rule what we requires a design doc. I do not want to turn this document into a feature list :)

flang/docs/DesignGuideline.md

21

Not with this exact format, the current documents have various structures. There are documents whose structure is very close like https://github.com/llvm/llvm-project/blob/main/flang/docs/RuntimeDescriptor.md

The purpose of this document is precisely to get to a more unified structure in the future (Note that I do not think it is worth changing the structure of existing docs at least not until we are far enough so that we can spend more time polishing the past).

25

So this is not unit testing, is it? It's more like a validation plan. This would make perfect sense for optimisations (to demonstrate the benefits), but what about new features that can be tested with regular LIT tests. It would be good to clarify this point. In particular, testing in LLVM is fairly well documented. What additional testing would you require?

This is testing in general. What kind of testing needs to be done should be a feature by feature decision, some aspect may be proven via unit tests, some other via validation tests. The purpose of the test plan is precisely to propose which testing should be done. This guideline could list available test options, but I do not think it should dictate more.

[clementval]

flang/docs/DesignGuideline.md

75

+1 on the fact that gfortran has not the same prominence. Especially in scientific computing where users tend to use OpenACC or OpenMP offloading.

[bryanpkc]

flang/docs/DesignGuideline.md

51–53

58–59

73

by semantic checks in the compiler. If not, it is a good idea to start by adding those checks

79

Even for Fortran 77, can we really achieve binary compatibility with other compilers? I feel that binary compatibility shouldn't be a goal at all. As Kiran pointed out above, compatibility can mean different things; some important aspects where compatibility with other established compilers might be useful include: semantics of command-line options, default floating-point math behavior, runtime library functionality, etc.

89–90

[klausler]

flang/docs/DesignGuideline.md

79

Total compatibility is of course not possible, but f18 should avoid gratuitous incompatibilities that prevents linking to F'77-level libraries compiled by gfortran and others. The ABI conventions for procedures that do not require explicit interfaces are well known and not hard to follow.

Code requiring explicit interfaces or modules is of course a different story that is not news to Fortran users; but even there, we've at least used a module file format that any other compiler should be able to parse.

[klausler]

flang/docs/DesignGuideline.md

30–33

There are conference calls, mailing lists, discourse, slack, and phabricator. Only phabricator is mandatory.

[peixin]

How about creating one user manual using tree-structured directories in https://flang.llvm.org/docs/? Who submit the patch with new feature should also fill in the corresponding missed part in the tree. It not only can direct the compiler developers, but also serves users to use LLVM Flang compiler. What we can refer to is clang user manual, gfortran user manual, or NAG user manual.

[kiranchandramohan]

flang/docs/DesignGuideline.md

30–33

The recommended (not mandatory) guideline when making a major change is to announce it via an RFC in discourse.
https://llvm.org/docs/DeveloperPolicy.html#making-a-major-change

If people feel this is a duplication of effort then we should check whether we can make Phabricator send an automatic post to discourse when the project is flang and the subject carries the word "RFC" or "Design Document".

Also, there seems to be a decision made to move from Phabricator to Github PRs by October 2023. I don't know whether this has any impact on this process. https://discourse.llvm.org/t/code-review-process-update/63964

[kiranchandramohan]

In D130166#3668985, @jeanPerier wrote:

In D130166#3668764, @kiranchandramohan wrote:

I like the idea of good design docs. We should have something like this for OpenMP as well. The only difficulty is that the OpenMP dialect and OpenMP IRBuilder sit outside the flang tree and can be worked on independently. So maybe any such discussions can be in discourse but before implementation in Flang (parse-tree to OpenMP + FIR dialects) we can make this a requirement for OpenMP as well subject to agreement of this proposal and consensus among OpenMP team members.

Good point, what about adding:

Features impacting projects outside of flang (like work OpenMP or OpenACC that may require touching parts outside of flang tree) should follow the general LLVM process, or the related subproject process. There should still be a related flang design docs if part of the solution impacts flang in significant way (e.g. it involves changes in the lowering from the parse-tree to OpenMP or FIR dialects that are not simply about mapping a parse-tree node to a related dialect operation).

+1

[kiranchandramohan]

@jeanPerier I see that @clementval has started the process in https://reviews.llvm.org/D131515 and https://discourse.llvm.org/t/rfc-lowering-design-doc-for-polymorphic-entities/64363. Are there still any issues left to be sorted out? Otherwise, we can try to submit this soon (after addressing any remaining comments).

[jeanPerier]

Add clarifications after first review. Fix typos/bad phrasing.

[jeanPerier]

In D130166#3671602, @kiranchandramohan wrote:

In D130166#3668985, @jeanPerier wrote:

In D130166#3668764, @kiranchandramohan wrote:

I like the idea of good design docs. We should have something like this for OpenMP as well. The only difficulty is that the OpenMP dialect and OpenMP IRBuilder sit outside the flang tree and can be worked on independently. So maybe any such discussions can be in discourse but before implementation in Flang (parse-tree to OpenMP + FIR dialects) we can make this a requirement for OpenMP as well subject to agreement of this proposal and consensus among OpenMP team members.

Good point, what about adding:

Features impacting projects outside of flang (like work OpenMP or OpenACC that may require touching parts outside of flang tree) should follow the general LLVM process, or the related subproject process. There should still be a related flang design docs if part of the solution impacts flang in significant way (e.g. it involves changes in the lowering from the parse-tree to OpenMP or FIR dialects that are not simply about mapping a parse-tree node to a related dialect operation).

+1

I added the note (slightly rephrased, and linking to https://llvm.org/docs/DeveloperPolicy.html#making-a-major-change).

In D130166#3722687, @kiranchandramohan wrote:

@jeanPerier I see that @clementval has started the process in https://reviews.llvm.org/D131515 and https://discourse.llvm.org/t/rfc-lowering-design-doc-for-polymorphic-entities/64363. Are there still any issues left to be sorted out? Otherwise, we can try to submit this soon (after addressing any remaining comments).

@kiranchandramohan, I updated the patch, and I believe I should have address all the comments (thanks @rovka and @bryanpkc for all the typo rephrasing suggestions). Please have a look at your convenience.

flang/docs/DesignGuideline.md

18

I added a link to the github project.

25

I added a small note to clarify this point and link to LLVM testing documentation.

30–33

I added a note: "If no RFC was made before sending the design document for review, it is highly encouraged to make a small announcement on https://discourse.llvm.org with a link to the Phabricator design document review."

79

Clarified that point by adding: "By binary compatibility, it is meant that F77 libraries compiled with other Fortran compilers (at least gfortran) should link with flang compiled code and vice-versa."

87

Added: "...,what should happen in the FIR transformation passes (FIR to FIR),..." (I do not want to get too specific by implying there are operations specific to the feature).

flang/docs/DesignGuideline.md

+<!--===- docs/DesignGuideline.md
+
+   Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+   See https://llvm.org/LICENSE.txt for license information.
+   SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+-->
+# Design Guideline
+
+```eval_rst
+.. contents::
+   :local:
+```
+## Documenting the design
+
+### Designing support for a new feature
+
+When working on a new feature in flang, some design document should
+be produced before submitting patches to the code.
+
+The preferred organization of such documents is:
+1) Problem description
+2) Proposed solution
+3) Implementation details overview
+4) Testing plan
+
+If several solutions can be considered, it is best to briefly describe the
+alternate solutions in 2) and why they were not retained.
+  
+The design document should be added to the `docs` folder as a markdown document,
+ideally using the name of the feature as the document name. Its approval on
+Phabricator is the pre-requisite to submitting patches implementing new
+features.
+  
+An RFC on flang https://discourse.llvm.org can first be made as one sees fit,
+but this document should still be produced to summarize, organize, and formalize
+the discussions. If a related discourse RFC was made it is a good idea to give a
+link to it in the document for future reference.
+
+### Updating the implementation solution of a feature                                                                         
+
+When doing a significant change to the implementation solution for a feature,
+the related design document should be updated so that it will justify the new
+solution.
+
+## Design tips
+
+### Design document style
+
+The document does not have to be long. It is highly encouraged to:
+- Stick to Fortran well defined terms when talking about Fortran
+  (definitions of these terms can be found in Section 3. of Fortran 2018
+  standard)
+- Be precise (e.g., pointing to the standard reference or constraint numbers).
+  References should currently be given against the Fortran 2018 standard
+  version.
+- Illustrate with a few small Fortran code snippets if applicable
+- When dealing with lowering, illustrate lowering output with a few FIR,
+  and LLVM IR code snippets
+- Illustrations do not have to be fully functional programs, it is better to
+  keep them small and focused on the feature. More detailed expectations
+  can be added in a second time or in parallel as LIT tests for example.
+
+### Thinking through the design of a Fortran feature
+
+Below is a set of suggested steps that one can take to fully apprehend a
+Fortran feature before writing a design for its implementation in flang.
+
+- Identify the relevant sections and constraints in the standard.
+- Write Fortran program using the feature and, if possible,
+  verify your expectations with existing compilers.
+- Check if the related constraints (Cxxx numbers in the standard) are enforced
+  by semantics. If not it is good idea to start by adding the related checks
+  (this does not require writing a design document).
+- Identify if the feature affects compatibility with programs compiled by other
+  Fortran compilers, or if a given solution for flang could not be changed in
+  the future without breaking compatibility with programs previously compiled
+  with flang. It is not a goal to be 100% binary compatible with other
+  compilers outside of Fortran 77, but sources of incompatibility should be
+  known and justified.
+- Identify related features, or contexts that matter for the feature (e.g,
+  does being in an internal procedure, a module, a block… changes anything
+  to what should happen?).
+- Not everything has to be inlined code, delegating part of the work to the
+  Fortran runtime may be a solution. Identify the relevant Fortran runtime
+  API if any.
+- For inlined code, consider what should happen when generating the FIR,
+  and what should happen when lowering the FIR to LLVM IR.
+- For inlined ops, look at how the existing dialects can be used,
+  if new FIR operations are required, justify their purpose.
+- Look at the related representation in Semantics (e.g., is some information
+  from the parse tree, the Symbol or evaluate::Expr required? Are there tools
+  to query this information easily?).