[flang-dev] (Strawman) Costed plan for LLVM-ification of F18

[Mehdi AMINI via flang-dev]

On Wed, Jan 29, 2020 at 7:49 AM Timothy Keith via flang-dev <
flang-dev at lists.llvm.org> wrote:

> Thanks for the reply.
>
>
>
> Just picking out a few points:
>
>
>
> > Module in the context of F18 means parser, semantics, evaluate etc.
>
>
>
> Those are abstract things. What are the concrete things you want to
> capitalize? Namespaces? Directory names?
>
>
>
> > But what we meant here was to make sure that std::string is not being
> used as a parameter to functions etc, these should be using StringRef (or
> possibly Twine where appropriate).
>
>
>
> What's the benefit of using StringRef over std::string for parameters?
>

StringRef is the moral equivalent of std::string_view: it expresses that
you just intend to read a sequence of characters without anything else.
Using it for function parameters over a « const string & » allow to
decouple the client of the API from the concrete type they hold.
For example at call site you may hold the data in another concrete type
than std::string (like llvm::SmallString for example), or you may want to
pass a substring to the function. Using std::string requires a copy (and
potential heap alloc).

Twine goes a step further and represent the concaténation of multiple
things. It is convenient if your API may or may not use the StringRef: it
will materialize the concatenation only on demand. It may also be useful to
materialize the concatenation directly in the final destination and save a
copy.

Many of these data structure are described here
https://www.llvm.org/docs/ProgrammersManual.html#passing-strings-the-stringref-and-twine-classes


>
> Regarding "always use early exit":
>
> > The LLVM Coding Standards have a strong preference for this, with
> justification given here:
>
> > https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
>
>
>
> That's a pretty thin justification. It gives an example where else should
> not be used and generalizes it to all cases. When you encounter exceptional
> conditions early exit it certainly appropriate. But in non-exceptional
> cases it does not always correctly express intent.
>
>
>
> Here's an example from resolve-names.cpp:
>
>     if (type.IsAssumedType()) {
>
>       return currScope().MakeTypeStarType();
>
>     } else if (type.IsUnlimitedPolymorphic()) {
>
>       return currScope().MakeClassStarType();
>
>     } else {
>
>       return currScope().MakeDerivedType(...);
>
>     }
>
>
>
> You could write this as:
>
>     if (type.IsAssumedType()) {
>
>       return currScope().MakeTypeStarType();
>
>     }
>
>     if (type.IsUnlimitedPolymorphic()) {
>
>       return currScope().MakeClassStarType();
>
>     }
>
>     return currScope().MakeDerivedType(...);
>
>
>
> That makes it look like MakeDerivedType is the "normal" case and the other
> two are exceptions.
>
> But that's wrong -- they are all normal and it's just a return based on a
> three-way condition.
>

As most coding style (like eliding trivial braces), this is always quite
subjective and mostly a matter of habit.
When all the codebase is following the same convention you quickly get use
to it.

Best,

—
Mehdi

>
>
> Tim
>
>
>
> *From: *David Truby <David.Truby at arm.com>
> *Date: *Wednesday, January 29, 2020 at 6:51 AM
> *To: *Timothy Keith <tkeith at nvidia.com>
> *Cc: *Richard Barton <Richard.Barton at arm.com>, "flang-dev at lists.llvm.org"
> <flang-dev at lists.llvm.org>
> *Subject: *Re: [flang-dev] (Strawman) Costed plan for LLVM-ification of
> F18
>
>
>
> *External email: Use caution opening links or attachments*
>
>
>
> Hi Tim,
>
>
>
> I had some input into putting this list together so I’ve added some
> clarifications to some things inline
>
>
>
> Thanks
>
> David Truby
>
>
>
> On 29 Jan 2020, at 14:12, Timothy Keith via flang-dev <
> flang-dev at lists.llvm.org> wrote:
>
>
>
> My comments and questions are below in line.
>
>
>
> Tim
>
>
>
> > Hi list
>
> >
>
> > As discussed on previous threads, we need to propose a plan for making
> F18 more LLVM-like in style and use of LLVM APIs and facilities. David is
> working on getting the permissions to put this into a github project in
> LLVM so we can collaborate . In order to make progress ahead of that
> happening, I'd like to have a go on email.
>
> >
>
> > For each area I would like to capture a list of work items we'll commit
> to doing before merge to monorepo. I want us to be happy we can achieve
> these in a short time period so we can propose a new merging date. I'd also
> like to create a list of items we'll commit to fixing after merging to the
> monorepo and a timeline for getting that done.
>
> >
>
> > Note that I'll only cover API and coding style work items in this
> thread. The other initiatives we can discuss in other threads. I'll just
> say that we'll propose cmake changes and porting test suite to lit in the
> pre-merge stuff and setting up build bots in the post-merge stuff.
>
> >
>
> > When he has access, David can create two projects one for pre-merge and
> one for post-merge and we'll put everything in there.
>
> >
>
> > Please consider the below a strawman. All dates can change, all list
> items can change position, new ones can be added, items can be deleted, etc.
>
> >
>
> > What do people think about these lists? Do we think those two dates are
> do-able?
>
> >
>
> > Ta
>
> > Rich
>
> >
>
> > Proposal:
>
> > We will make the following style changes before merging to the monorepo
>
> >
>
> > F18 changes to make it more LLVM-like in code style
>
> > 1. Rationalise headers to put public headers in /include and not /lib
>
> > 2. Unify F18's clang-format file to match LLVMs
>
>
>
> We should discuss what should be in .clang-format with an eye to make it
> more like LLVM's.
>
>
>
> > 3. Rename all .cc files to .cpp
>
> > 4. Switch module names to starting with capital letters
>
>
>
> What does "module" mean in this context?
>
> Module in the context of F18 means parser, semantics, evaluate etc.
>
>
>
> > Increase use of LLVM APIs and utilities in F18
>
> > a. Switch F18 custom File handling to LLVM's File handling (helps with
> non-POSIX platform support)
>
> > b. Change uses of <iostream> to LLVM's streams
>
>
>
> F18 doesn't  use <iostream>.
>
> I think we are (perhaps ambiguously) using <iostream> here to refer to the
> entire standard stream IO library. E.g. I do see a lot of uses of <ostream>
> in F18. LLVM has its own replacement for these for various reasons and
> highly discourages the use of the standard ones.
>
>
>
> > c. Migrate use of std::list to a suitable alternative in LLVM's API
>
>
>
> I would say: replace std::list with a better data structure where
> appropriate.
>
>
>
> > d. Use llvm_unreachable with an error message for unreachable cases
>
>
>
> What does llvm_unreachable do for us?
>
> llvm_unreachable effectively behaves like an abort in debug builds,
> however in release builds it adds optimiser hints abstracted across the
> various supported compilers that the code in question isn’t reachable (so
> no need to generate a branch there or whatever).
>
>
>
> > e. Use llvm::Error instead of error codes if and when error codes are
> used.
>
>
>
> I don't know of any use of error codes.
>
> What error handling facility does F18 currently use if error codes and
> exceptions aren’t used? I am familiar with a few places that optionals are
> returned, we could consider looking at changing these to using
> llvm::Expected to provide more context about what has gone wrong.
>
>
>
> > We would like to aim for a merge date of Monday 24th Feb to merge to the
> monorepo with all of the above changes committed to F18 master.
>
> >
>
> > We then propose to make the following changes after merging to the
> monorepo.
>
> >
>
> > F18 changes to make it more LLVM-like in code style
>
> > We will commit to a perform a one-off exercise where we automatically
> audit the code to find these instances and bring them in line.
>
> > 1. Eliminate braces from all single-line if statements
>
>
>
> This seems like a really bad idea and would like to see a reason for it.
>
>
>
> > 2. Eliminate all uses of else-after-return
>
>
>
> Doing this blindly is also a bad idea.
>
> The LLVM Coding Standards have a strong preference for this, with
> justification given here:
>
> https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
>
>
>
> > 3. Add doxygen infrastructure so we can generate docs
>
> >
>
> > Increase use of LLVM APIs and utilities in F18
>
> > a. std::string → StringRef where appropriate
>
>
>
> These aren't similar data structures. std::string owns the string data and
> StringRef does not. std::string_view is now a standard class for the latter
> case.
>
> LLVM still currently prefers the use of StringRef over std::string_view. I
> am not sure if there are plans to change this when LLVM switches the
> majority of its code to C++17. But what we meant here was to make sure that
> std::string is not being used as a parameter to functions etc, these should
> be using StringRef (or possibly Twine where appropriate).
>
>
>
> > b. std::vector → llvm::SmallVector where appropriate
>
> > c. std::set → llvm::SmallSet/llvm::StringSet/llvm::DenseSet where
> appropriate
>
> > d. std::map → llvm::StringMap/llvm::DenseMap where appropriate
>
>
>
> As long as "where appropriate" means there is tangible benefit to changing
> from a standard type to a non-standard one. Not just change for the sake of
> change.
>
> This is indeed what we meant by where appropriate. The LLVM guidelines are
> perfectly happy for the use of standard data structures in general, however
> the LLVM specific ones are much better optimised for certain use cases. For
> example, the DenseMap class in particular is much more efficient than
> std::map when the key and value are both small objects.
>
>
>
> > e. Audit functions in include/flang/common and port to LLVM equivalents
> (e.g. builtin_popcount)
>
> >
>
> > Assuming we hit the above merge date, we think we can commit to making
> these changes before the LLVM11 branch is taken in June.
>
> >
>
> > After that date, we will continue to make progress towards LLVM style
> and API usage by fixing things as we find them during development and
> enforce the new style through code review.
>
> >
>
> > A few specific areas that have been mentioned before that we will tackle
> in this way are:
>
> >    - Add doxygen style comments and file comments
>
> >    - Find more expressive names for classes, files, etc.
>
>
>
> Names are already supposed to be expressive. Improvements are always
> welcome.
>
>
>
> >    - Refactor code to use early exits
>
>
>
> Where it improves the code, not blindly. As with improving names, there is
> no reason to wait for any specific date to make improvements to the code.
>
> Again, early exists are something the LLVM Coding Standards have a strong
> preference for, with justification given here:
>
> https://llvm.org/docs/CodingStandards.html#early-exits
>
> ------------------------------
>
> This email message is for the sole use of the intended recipient(s) and
> may contain confidential information.  Any unauthorized review, use,
> disclosure or distribution is prohibited.  If you are not the intended
> recipient, please contact the sender by reply email and destroy all copies
> of the original message.
> ------------------------------
>
> _______________________________________________
> flang-dev mailing list
> flang-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/flang-dev
>
>
> _______________________________________________
> flang-dev mailing list
> flang-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/flang-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/flang-dev/attachments/20200129/527963e7/attachment-0001.html>