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

Thu Aug 25 06:08:12 PDT 2022

jeanPerier added a comment.

In D130166#3671602 <https://reviews.llvm.org/D130166#3671602>, @kiranchandramohan wrote:

> In D130166#3668985 <https://reviews.llvm.org/D130166#3668985>, @jeanPerier wrote:
>
>> In D130166#3668764 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

================
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.
----------------
jeanPerier wrote:
> 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.
I added a link to the github project.

================
Comment at: flang/docs/DesignGuideline.md:25
+3) Implementation details overview
+4) Testing plan
+
----------------
jeanPerier wrote:
> awarzynski wrote:
> > jeanPerier wrote:
> > > 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.
> > > 
> > > 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 [[ https://github.com/llvm/llvm-test-suite |  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?
> > 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.
I added a small note to clarify this point and link to LLVM testing documentation.

================
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.
----------------
kiranchandramohan wrote:
> klausler wrote:
> > kiranchandramohan wrote:
> > > awarzynski wrote:
> > > > jeanPerier wrote:
> > > > > 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.
> > > > > 
> > > > > 
> > > > I suggest that:
> > > > # We require all new proposal/design-docs to be //advertised// on Discourse.
> > > > # 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. 
> > > For me, advertising in discourse should be made compulsory so that people are aware. 
> > There are conference calls, mailing lists, discourse, slack, and phabricator.  Only phabricator is mandatory.
> 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
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."

================
Comment at: flang/docs/DesignGuideline.md:79
+  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.
----------------
klausler wrote:
> bryanpkc wrote:
> > 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.
> 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.
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."

================
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.
----------------
jeanPerier wrote:
> 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.
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).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130166