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

Thu Jul 21 03:04:18 PDT 2022

awarzynski added a subscriber: Meinersbur.
awarzynski added inline comments.

================
Comment at: flang/docs/DesignGuideline.md:25
+3) Implementation details overview
+4) Testing plan
+
----------------
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?

================
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:
> 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. 

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130166