[llvm-dev] Flang landing in the monorepo - next Monday!

Wed Jan 15 13:33:54 PST 2020

Hi Richard,

On Wed, Jan 15, 2020 at 1:11 AM Richard Barton <Richard.Barton at arm.com>
wrote:

> Hi Eric, Renato
>
> Thanks again for the engagement and challenge on this, it is really useful
> feedback to know what we need to do to get F18 into the project in a way
> that everyone is happy with.
>
> I have tried to give timelines on the points addressed below where I can
> today. Clearly we need to do some work on points 8-11, but are the above
> plans/answers to points 1-7 sufficient at this stage and would not block a
> merge of F18 to the monorepo? If not, what else would you need to see or
> what would need to be different for you to be happy with F18 merging into
> the monorepo?
>
> >> 4. No use of LLVM APIs and so no connection to the project [Addressed
> by Hal and me - it is the natural next step in development as Flang starts
> to generate MLIR. Nvidia are working on this now.]
> > The choice of MLIR is interesting - and given that there was no
> discussion of it I was wondering if you have a document laying out
> pros/cons and what ultimately made the decision here.
>
> Hopefully Steve’s reply has covered your concerns about the design choice.
> Also to point out that there has been a lot of development done on this
> approach already, which Eric has shared [1] (still a work in progress), and
> this has reinforced our feeling that a) having a high-level IR above LLVM
> IR is a good idea and b) using MLIR for that is a good idea.
>
>
Yes. That was a good post and while I haven't gotten a chance to read
everything yet I appreciate the post.

FWIW my concerns are more than just "uses llvm to generate code" and as
I've outlined a few times there are a lot of specific things that you
should be using LLVM for and C like C++ is another. I did a quick look over
the code and summarized a few of the examples in an earlier post. Please
try to avoid conflating "using llvm" with "using llvm to emit IR of some
sort".

I think that affects a lot of the "we can't give a timeline" in your email.
Can you comment further on the other code issues I've raised?

-eric

> The next step for this work is to break it down into a logical stream of
> patches and go through review to get them committed. Our preference would
> be to do this in phabricator in the llvm project after merging. We think
> the benefit of doing this review in the llvm project is that more reviewers
> can participate, including those from the MLIR project. We also hope that
> by developing FIR in-tree we can more easily manage its MLIR dependency.
>
> I don’t think we can be sure of a timeline for completion of this work. In
> reality adding this part of the frontend will likely be an ongoing effort
> over the next months, perhaps years and it will likely be a work in
> progress by the time of the LLVM11 branch. We can at least commit to
> starting this effort soon, lead by Nvidia. If we don't merge to monorepo
> shortly then the work will start on the F18 github project. Arm engineers
> will be reviewing, but we think the benefit of doing this development in
> the monorepo is that other interested parties can join in from the MLIR and
> LLVM projects. We'd ask that we merge to the monorepo before the review
> starts.
>
> > 5. No use of lit in the regression tests [Arm is working on this]
> This effort has started already in the F18 project [2]. Arm have some work
> downstream to make the next steps – porting the old tests over to use lit.
> Meanwhile, we would expect make check-all to run all the new lit tests as
> well as the old non-lit tests until the latter are all ported over to lit.
> We expect to have this completed before the LLVM11 branch. We propose that
> this not gate merge to the monorepo and we switch development from github
> to phab as soon as the merge happens.
>
> > 6. Need to refactor parts of clang driver that can be shared with flang
> into a separate library [Arm is working on this, but plans to implement a
> simple driver first before refactoring to better understand the
> opportunities to do so. See Peter's RFC [d] ]
> Similar to above, this effort has started. Arm’s next step after
> contributing the flang entry point is to agree on a spec for the
> replacement frontend driver. Given our expectation as to what this will
> contain, we expect to be able to complete before the LLVM11 branch. We
> propose this not block merging to the monorepo and we switch development
> from github to phab as soon as the merge happens. In terms of refactoring
> the code to share with Clang, our proposal is to implement the Flang side
> work then refactor later on once we understand the code better. We would be
> very happy to reconsider this and put up another RFC if we want to refactor
> as we go along instead. This approach is much easier once F18 is accepted
> into the monorepo so we can make the changes together.
>
> > > 7. No integration into the LLVM build system/Cmake
> > I don't really consider this addressed without a plan of action (before
> merge) that contains a list of standard llvm support libraries that would
> be used in addition to things being removed (pretty much all use of C++ c*
> headers and more?) as well as staffing and approximate ETAs.
>
> Eric Schweitz has implemented this as part of his patch that adds FIR [1].
> Our plan is to extract this as a separate patch in phabricator for review
> so it can land more or less right away after F18 is merged into the
> monorepo. To be clear, our intention is that flang would *not* build by
> default after the patch.
>
> >> 8. No use of LLVM utilities or standard data structures
> >> 9. Simple deviations from the LLVM coding style
> >>     a. Separating public headers into include/flang
> >>     b. Syntactical things like braces on single line statements,
> comments on end of namespaces, etc.
> >>     c. .cc file extensions rather than .cpp
> >> 10. Bigger deviations from the LLVM coding style that are harder to fix
> >>     a. Early exits and not using else after return, etc.
> >> 11. Flang not supporting all the same C++ compilers as the rest of the
> project (even taking into account C++17 requirement)
> > As well as this. A plan of action isn't just a "we'll get to this" but
> an actual plan :)
>
> We will discuss these points on the flang-dev list now and report back
> with our plans. We'll work on a list of items to fix before merge and list
> that we propose to fix after merge on a shorter timescale. I welcome you to
> respond on those threads if there are some specific recommendations or key
> issues you care about. In any case, we'll reply back with our proposal.
>
> @Renato - I just wanted to answer a few points from your reply to make
> sure there is no misunderstanding here about F18.
>
> > Now, I am surprised it's not using LLVM API yet (a clear and
> > consistent complaint of the previous one), as well as having its own
> > driver, test infra, etc.
>
> The previous flang (to be absolutely clear - I mean this one:
> https://github.com/flang-compiler/flang) did not use the LLVM API as you
> say, instead writing a .ll file and calling clang on that file. F18 will
> certainly use LLVM APIs to generate IR directly but that code has not made
> it to the master F18 branch today. It is not true to say that F18 has
> inherited this flaw from its predecessor.
>
> > The lack of C++ compatibility may look small now, but once we want to
> > move to a newer standard, Flang will be the bottleneck. In a monorepo,
> > these things are even more important.
> > F18 was supposed to be a whole new front-end, from scratch, but it
> > looks to have carried along many of the previous projects' flaws.
>
> To clarify, F18 code is compatible to C++17 so it is currently ahead of
> the standard used by the rest of the LLVM project. That is an issue by
> itself that needs managing, but a different problem from the one you
> describe here. The problem with F18 being ahead of the rest of the project
> was discussed at length during the original upstreaming discussion
> http://lists.llvm.org/pipermail/llvm-dev/2019-February/130497.html and we
> agreed to revisit the problem when F18 is part of the project and going
> more mainstream and see where the rest of the project was in terms of its
> minimum standard. At that point we can chose what to do.
>
> Thanks again for caring about this and your help.
>
> Ta
> Rich
>
> [1] https://github.com/flang-compiler/f18/pull/920
> [2]
> https://github.com/flang-compiler/f18/commit/a58c6067a1ab5cd02dfb5b6fb9919a20b960d984
> [3] https://reviews.llvm.org/D63607,
> https://github.com/flang-compiler/f18/pull/759,
> https://github.com/flang-compiler/f18/pull/762 and
> https://github.com/flang-compiler/f18/pull/763
>
>
> From: Eric Christopher <echristo at gmail.com>
> Sent: 13 January, 2020 06:51
> To: Richard Barton <Richard.Barton at arm.com>
> Cc: Renato Golin <rengolin at gmail.com>; Hans Wennborg <hans at chromium.org>;
> llvm-dev at lists.llvm.org; Mike Edwards <medwards at llvm.org>
> Subject: Re: [llvm-dev] Flang landing in the monorepo - next Monday!
>
>
>
> On Thu, Jan 9, 2020 at 4:28 AM Richard Barton via llvm-dev <mailto:
> llvm-dev at lists.llvm.org> wrote:
> Hi all
>
> Thanks for all the replies and engagement on this issue.
>
> First point, given the state of discussions today I would like to propose
> that we don't start the merge at 10:00 GMT on Monday 13th as proposed and
> we delay by at least 24 hours until after the scheduled F18 technical call
> on Monday afternoon.
>
> In order to help compile a plan of action, I've tried to compile a list of
> the concerns that folks have raised so far.
>
> I think all these have been addressed (please correct me if you think
> otherwise)
> 1. Audit trail/visibility of code review [Addressed by Peter - code has
> been reviewed [a] to F18 coding guidelines [b].
> 2. Long-term viability of Flang community and overlap with existing LLVM
> community [Hopefully Hal and Johannes replies and Greg's and Pat's and my
> reply demonstrate long-term commitment to Flang after upstreaming]
> 3. Compatibility of license [Addressed by Steve, a recent update has made
> the licenses compatible [c]]
> 4. No use of LLVM APIs and so no connection to the project [Addressed by
> Hal and me - it is the natural next step in development as Flang starts to
> generate MLIR. Nvidia are working on this now.]
>
> The choice of MLIR is interesting - and given that there was no discussion
> of it I was wondering if you have a document laying out pros/cons and what
> ultimately made the decision here.
>
> I think these are acknowledged right now and we are actively working on
> fixes:
> 5. No use of lit in the regression tests [Arm is working on this]
> 6. Need to refactor parts of clang driver that can be shared with flang
> into a separate library [Arm is working on this, but plans to implement a
> simple driver first before refactoring to better understand the
> opportunities to do so. See Peter's RFC [d] ]
> 7. No use of LLVM utilities or standard data structures (Vector, etc.) [I
> think Pat and Eric have patches ready to go for this?]
>
> I don't really consider this addressed without a plan of action (before
> merge) that contains a list of standard llvm support libraries that would
> be used in addition to things being removed (pretty much all use of C++ c*
> headers and more?) as well as staffing and approximate ETAs.
>
>
> I think these are only acknowledged, with the intention to remediate post
> merge, but no concrete plan or owner at this point:
> 8. Simple deviations from the LLVM coding style
>     a. Separating public headers into include/flang
>     b. Syntactical things like braces on single line statements, comments
> on end of namespaces, etc.
>     c. .cc file extensions rather than .cpp
> 9. Bigger deviations from the LLVM coding style that are harder to fix
>     a. Early exits and not using else after return, etc.
> 10. Flang not supporting all the same C++ compilers as the rest of the
> project (even taking into account C++17 requirement)
> As things stand we are not proposing to remediate these (issues 5 and
> greater) in the code before it is merged to LLVM next week. These issues
> will be worked on after commit.
>
> As well as this. A plan of action isn't just a "we'll get to this" but an
> actual plan :)
>
> Does anyone feel passionately that any of the above points need to be
> addressed in the code before we can merge?
> Does anyone feel there is anything missing from that list?
>
> Given Mehdi and Hans' query about why this needs to go into LLVM10. Unless
> anyone has any good reason, perhaps we could postpone until after the
> branch, perhaps by one week to January 20th? That would give us some more
> time to bottom out what changes need to be made before we can accept the
> code and give us time to make any changes that block inclusion (assuming
> they are suitably sized to do in this timescale). As Renato suggests, that
> would also give us the LLVM11 branching point as a natural deadline to have
> addressed the larger concerns in our list.
>
> What do people think to that proposal?
>
> I think waiting until after the llvm 10 branch would probably be best at
> this point. That will give a lot of time for cleanup and to make f18 a much
> more reasonable "preview" including code generation in a forthcoming
> release in about 6 mos.
>
> -eric
>
> Regards
> Rich
>
> [a] https://github.com/flang-compiler/f18/pulls
> [b]
> https://github.com/flang-compiler/f18/blob/master/documentation/C%2B%2Bstyle.md
> [c]
> https://github.com/flang-compiler/f18/commit/e06be2faa64a52471b3cfb2829dc05888236aa68
> [d]  http://lists.llvm.org/pipermail/cfe-dev/2019-June/062669.html
>
> > -----Original Message-----
> > From: llvm-dev <mailto:llvm-dev-bounces at lists.llvm.org> On Behalf Of
> Renato
> > Golin via llvm-dev
> > Sent: 9 January, 2020 10:02
> > To: Hans Wennborg <mailto:hans at chromium.org>
> > Cc: llvm-dev <mailto:llvm-dev at lists.llvm.org>; Mike Edwards <mailto:
> medwards at llvm.org>
> > Subject: Re: [llvm-dev] Flang landing in the monorepo - next Monday!
> >
> > On Thu, 9 Jan 2020 at 08:13, Hans Wennborg via llvm-dev
> > <mailto:llvm-dev at lists.llvm.org> wrote:
> > > > Why is it targeting pre-llvm10 branching? Are we expecting to ship
> > anything in LLVM 10? If so I would have expected it to land much much
> > earlier to be able to stabilize during the development phase long before
> the
> > branching point.
> > >
> > > I was wondering about this too. I'm not necessarily opposed, but
> > > wouldn't it make sense to target landing soon *after* the branch
> > > instead, to give it time to integrate, stabilize, and then ship in the
> > > next release?
> >
> > Usually, yes.
> >
> > I'm guessing this is due to downstream implementations wanting to pick
> > up the current release as base target (which can be done regardless,
> > but...).
> >
> > In theory we can hide flang by having to explicitly name it on the
> > build command, so merging now or later won't make massive practical
> > change.
> >
> > I personally don't like rushing things like that, but I'm not strongly
> > opposed to either, if everyone else is on board with the rush.
> >
> > cheers,
> > --renato
> > _______________________________________________
> > LLVM Developers mailing list
> > mailto:llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> _______________________________________________
> LLVM Developers mailing list
> mailto:llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200115/4f69e767/attachment.html>