[flang-commits] [PATCH] D130166: [flang] Adding a guideline for flang design documentation

Thu Jul 21 01:50:40 PDT 2022

jeanPerier added a comment.

In D130166#3665793 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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").

================
Comment at: flang/docs/DesignGuideline.md:18
+
+When working on a new feature in flang, some design document should
+be produced before submitting patches to the code.
----------------
awarzynski wrote:
> Can you clarify what you mean by "a new feature"? What changes are expected to come with a design doc?
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.

================
Comment at: flang/docs/DesignGuideline.md:25
+3) Implementation details overview
+4) Testing plan
+
----------------
kiranchandramohan wrote:
> Should this have end to end tests? If so should we think of setting up a mechanism to run end-to-end tests?
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.

================
Comment at: flang/docs/DesignGuideline.md:30-33
+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.
----------------
clementval wrote:
> unterumarmung wrote:
> > 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.
> 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?
> 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.

================
Comment at: flang/docs/DesignGuideline.md:30-33
+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.
----------------
jeanPerier wrote:
> clementval wrote:
> > unterumarmung wrote:
> > > 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.
> > 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?
> > 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.
> 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.

================
Comment at: flang/docs/DesignGuideline.md:75
+  (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
----------------
kiranchandramohan wrote:
> Is there are preference for compatibility with gfortran over other fortran compilers?
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.

================
Comment at: flang/docs/DesignGuideline.md:87
+  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.
----------------
kiranchandramohan wrote:
> 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?
That is a good point thanks, I will add it.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130166/new/

https://reviews.llvm.org/D130166