<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">
Hi all,
<div class=""><br class="">
</div>
<div class="">I’ve created a project board and associated GitHub issues to track this list at
<a href="https://github.com/orgs/flang-compiler/projects/8" class="">https://github.com/orgs/flang-compiler/projects/8</a>. Hopefully this will be useful to see what we’re making progress on.</div>
<div class="">Please let me know if there’s anything that needs adding or fixing on here.</div>
<div class=""><br class="">
</div>
<div class="">Thanks</div>
<div class="">David Truby<br class="">
<div><br class="">
<blockquote type="cite" class="">
<div class="">On 30 Jan 2020, at 16:37, Richard Barton via flang-dev <<a href="mailto:flang-dev@lists.llvm.org" class="">flang-dev@lists.llvm.org</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div class="">Hi all<br class="">
<br class="">
Many thanks for the quick replies so far. Looking at the comments so far I clearly should have been more explicit in some of my items – sorry about that.
<br class="">
<br class="">
Please find below an updated strawman for review. I notice no discussion on the dates I proposed. Are we happy to push for those?<br class="">
<br class="">
First a few comments of my own:<br class="">
<br class="">
<blockquote type="cite" class="">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.<br class="">
</blockquote>
<br class="">
I wonder if we are thinking about this the wrong way around. Our aim to become an LLVM project and we hope that will happen soon. When it does, and we are sitting in the monorepo as flang then all uses of standard C++ types instead of equivalent LLVM types
without good technical grounds would look strange or to paraphrase, seem like "differences for the sake of difference".
<br class="">
<br class="">
We need to be thinking of ourselves as an LLVM project. If we adopt that mindset, then rather than looking for a reason to adopt the LLVM types we should really be checking if there is a good reason _not_ to adopt them. I don’t doubt there will be some occasions
where we have good justification to not adopt LLVM types and stay with the standard type and we certainly will back up our arguments not to chane. But I don’t think the default position should be not switch to LLVM types.<br class="">
<br class="">
<blockquote type="cite" class="">As with improving names, there is no reason to wait for any specific date to make improvements to the code.<br class="">
</blockquote>
<br class="">
I fully agree with that and we should continue improving F18 regardless of upstreaming. My intention with the statement in my mail is to make explicit to the LLVM community that we don’t intend to perform a line-by-line audit and make every change we could
possibly do. We’re being asked to commit to a timeline and I want to make sure that the list of things we need to complete in that timeline is not open ended. I'd like to make explicit what we are not planning to do by some particular date and that we'll do
later over time instead.<br class="">
<br class="">
<blockquote type="cite" class="">No else-after-return ...<br class="">
Here's an example from resolve-names.cpp:<br class="">
</blockquote>
<br class="">
I see where you are coming from here and we can discuss these sorts of details in depth on code review for the specific examples. In cases like these, perhaps there is some refactoring we could do to capture the three way condition in the type class such that
the if/elif/else block is not needed at all, or in such a way that the rewrite to if/if/return seems more natural, or perhaps with enough comments it will look ok anyway. All we need to do at this stage is commit to looking at it seriously and state when we'll
do that by.<br class="">
<br class="">
Ta<br class="">
Rich<br class="">
<br class="">
Updated Strawman<br class="">
=================<br class="">
Proposal:<br class="">
We will make the following style changes before merging to the monorepo<br class="">
<br class="">
F18 changes to make it more LLVM-like in code style<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>1. Rationalise headers to put public headers in /include and not /lib<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>2. Examine F18's clang-format file and ensure we are comfortable with any deviations to the LLVM style<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>3. Rename all .cc files to .cpp<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>4. Capitalize the module directory names in /lib and /include (e.g. /lib/Parser)<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span><br class="">
Increase use of LLVM APIs and utilities in F18<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>a. Switch F18 custom File handling to LLVM's File handling (helps with non-POSIX platform support)<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>b. Change uses of C++ standard stream IO library to LLVM's equivalent library<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>c. Migrate use of std::list to a suitable alternative in LLVM's API<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>d. Use llvm_unreachable with an error message for unreachable cases<br class="">
<br class="">
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.<br class="">
<br class="">
We then propose to make the following changes after merging to the monorepo. <br class="">
<br class="">
F18 changes to make it more LLVM-like in code style<br class="">
We will perform a one-off exercise where we audit the code to find these instances and bring them in line. We'll look at:<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>1. Braces on all single-line if statements<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>2. Uses of else-after-return.<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>3. Add doxygen infrastructure so we can generate docs<br class="">
<br class="">
Increase use of LLVM APIs and utilities in F18<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>a. std::string/std::string_view → StringRef where appropriate<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>b. std::vector → llvm::SmallVector where appropriate<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>c. std::set → llvm::SmallSet/llvm::StringSet/llvm::DenseSet where appropriate<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>d. std::map → llvm::StringMap/llvm::DenseMap where appropriate<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>e. Audit functions in include/flang/common and port to LLVM equivalents (e.g. builtin_popcount)<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span>f. Audit use of any error codes and switch to use llvm::Error instead<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span><br class="">
Assuming we hit the above merge date, we think we can commit to making these changes before the LLVM11 branch is taken in June.<br class="">
<span class="Apple-tab-span" style="white-space:pre"></span><br class="">
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.<br class="">
<br class="">
A few specific areas that have been mentioned before that we will tackle in this way are:<br class="">
- Add doxygen style comments and file comments<br class="">
- Classes, files, names, etc. where a more LLVM-standard naming can be used.<br class="">
- Refactor code to use early exits<br class="">
<br class="">
<blockquote type="cite" class="">-----Original Message-----<br class="">
From: David Greene <<a href="mailto:greened@obbligato.org" class="">greened@obbligato.org</a>><br class="">
Sent: 29 January, 2020 21:16<br class="">
To: Timothy Keith <<a href="mailto:tkeith@nvidia.com" class="">tkeith@nvidia.com</a>>; Richard Barton<br class="">
<<a href="mailto:Richard.Barton@arm.com" class="">Richard.Barton@arm.com</a>>; <a href="mailto:flang-dev@lists.llvm.org" class="">
flang-dev@lists.llvm.org</a><br class="">
Subject: Re: [flang-dev] (Strawman) Costed plan for LLVM-ification of F18<br class="">
<br class="">
Timothy Keith via flang-dev <<a href="mailto:flang-dev@lists.llvm.org" class="">flang-dev@lists.llvm.org</a>> writes:<br class="">
<br class="">
<blockquote type="cite" class="">
<blockquote type="cite" class="">1. Eliminate braces from all single-line if statements<br class="">
</blockquote>
<br class="">
This seems like a really bad idea and would like to see a reason for it.<br class="">
</blockquote>
<br class="">
FWIW this is one of the few things in LLVM's coding standards that I<br class="">
strongly disagree with, but for whatever reason others strongly support<br class="">
it. I just live with it. I would enthusiastically support an RFC to<br class="">
llvm-dev to change it.<br class="">
<br class="">
<blockquote type="cite" class="">
<blockquote type="cite" class="">2. Eliminate all uses of else-after-return<br class="">
</blockquote>
<br class="">
Doing this blindly is also a bad idea.<br class="">
</blockquote>
<br class="">
I don't think anyone is saying to do anything "blindly."<br class="">
<br class="">
<blockquote type="cite" class="">
<blockquote type="cite" class="">b. std::vector → llvm::SmallVector where appropriate<br class="">
c. std::set → llvm::SmallSet/llvm::StringSet/llvm::DenseSet where<br class="">
</blockquote>
</blockquote>
appropriate<br class="">
<blockquote type="cite" class="">
<blockquote type="cite" class="">d. std::map → llvm::StringMap/llvm::DenseMap where appropriate<br class="">
</blockquote>
<br class="">
As long as "where appropriate" means there is tangible benefit to<br class="">
changing from a standard type to a non-standard one. Not just change<br class="">
for the sake of change.<br class="">
</blockquote>
<br class="">
IME, LLVM's data structures are much faster/less resource intensive than<br class="">
the standard library's. That's natural as the standard library is<br class="">
general-purpose code while LLVM's is tuned for specific use-cases. In<br class="">
my work I always use LLVM's data structures unless there's a good reason<br class="">
not to. f18 should follow the LLVM convention here.<br class="">
<br class="">
-David<br class="">
</blockquote>
_______________________________________________<br class="">
flang-dev mailing list<br class="">
<a href="mailto:flang-dev@lists.llvm.org" class="">flang-dev@lists.llvm.org</a><br class="">
https://lists.llvm.org/cgi-bin/mailman/listinfo/flang-dev<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</body>
</html>