> On Jul 25, 2016, at 7:13 PM, Philip Reames <listmail at philipreames.com> wrote: > > > > > > On Jul 25, 2016, at 5:54 PM, Chandler Carruth via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: > >> On Mon, Jul 25, 2016 at 5:47 PM Mehdi Amini via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: >> >> > On Jul 25, 2016, at 4:18 PM, Renato Golin via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: >> > 6. The target's code must have been adapted to the developers policy as well as >> > the coding standards. This can take a while and it should be fine to >> > accept external code into LLVM as experimental, but not officially. >> >> FWIW I’m not fond of not having this as the experimental part in the first place. >> It is harder to have things moving after being upstreamed. >> I’d expect the upstreaming of a backend (into experimental state) to be piece by piece with proper code review according to the current project standard. >> >> I'm not sure it makes sense to insist on backends being incrementally developed in the open. While I rather prefer that (WebAssembly has been fun to watch here), it doesn't seem realistic. >> >> Every backend I can think of other than WebAssembly and the original AArch64 port has come into existence out of tree, and only after proving useful to some folks decided to move into the tree. I'd personally advocate for keeping that door open. > > I strongly agree with Chandler here. > > I *also* feel that gating the move from experimental to non-experimental would be a reasonable place to draw a line in the sand on this topic if desired. I am not arguing for actually drawing that strong a line mind you, just saying that would be a better place if we had to have one.I don’t make sense of this, so I guess we are not talking about the same thing. Moving from experimental to non-experimental is a one-line patch while all the pieces are glued together in-tree, and I don’t see any practical way to have some modular review of a backend *after* this happened. If you accept some spaghetti code as experimental, I don’t believe it is ever gonna be totally untangled by iterating on it in-tree. — Mehdi> > >> >> All that said,I completely agree regarding coding conventions and other basic stuff. I don't think we need to wait for the official point in time to insist on all of the fundamental coding standards and such being followed. >> >> >> > >> > A soft(-er?) requirement: >> > >> > 7. The test coverage is broad and well written (small tests, documented). Where >> > applicable, both the ``check-all`` and ``test-suite`` must pass in at least >> > one configuration (publicly demonstrated, ex. via buildbots). >> >> Same as before: this is a no-brainer should be applicable with any patch, even in experimental state: small and documented lit tests. >> >> Yep, this seems like it should always be true regardless of the experimental nature. >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <http://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/20160725/2d5e42f7/attachment.html>
On 26 July 2016 at 04:46, Mehdi Amini <mehdi.amini at apple.com> wrote:> I don’t make sense of this, so I guess we are not talking about the same > thing.I think I know what it is... :)> Moving from experimental to non-experimental is a one-line patch > while all the pieces are glued together in-tree, and I don’t see any > practical way to have some modular review of a backend *after* this > happened.You're assuming there will be no code review on the back-end and we'll accept spaghetti code because the "policy" doesn't say anything about it. I understand why you see it that way, as it's not explicit. But as with all policies, we'd better encode the bare minimum than the most complete picture, as there will always be edge cases we can't predict. What *normally* happens is that, during the first review (bulk import), people point out several problems with the code. Style issues, bad tests, bad patterns, misconceptions, etc. All of those are generally fixed pre-import, if they're simple, or marked as FIXME if more elaborate. There is no ratio for that, it can be all easy, or many hard, and it will depend on the case. If we make all rules fixed at maximum, we will lose the ability to import code and evolve with time. One of the key points, and the first one indeed, is that the community has to be pro-active and nice. If we request a bunch of changes, they don't change and request to be made official, then they have failed the first and most important rule. OTOH, if we failed to spot the problems in the first bulk, and later accept a bad target as official, than it would be our own fault. We can add a note describing the FIXME approach if people think it will make things more clear, but essentially, the intent is in there already. I'm ok with being more explicit, though, just not more strict. cheers, --renato
> On Jul 26, 2016, at 2:58 AM, Renato Golin <renato.golin at linaro.org> wrote: > > On 26 July 2016 at 04:46, Mehdi Amini <mehdi.amini at apple.com> wrote: >> I don’t make sense of this, so I guess we are not talking about the same >> thing. > > I think I know what it is... :) > > >> Moving from experimental to non-experimental is a one-line patch >> while all the pieces are glued together in-tree, and I don’t see any >> practical way to have some modular review of a backend *after* this >> happened. > > You're assuming there will be no code review on the back-end and we'll > accept spaghetti code because the "policy" doesn't say anything about > it. I understand why you see it that way, as it's not explicit.No, the problem is that your writing is making an exception to the developer policy, and I don’t think it is a good thing.> > But as with all policies, we'd better encode the bare minimum than the > most complete picture, as there will always be edge cases we can't > predict.There is no edge cases in question here.> What *normally* happens is that, during the first review (bulk > import), people point out several problems with the code. Style > issues, bad tests, bad patterns, misconceptions, etc. All of those are > generally fixed pre-import, if they're simple, or marked as FIXME if > more elaborate.What kind of conformance to "developers policy as well as the coding standards” is too elaborate to be fixed pre-import? I don’t think any, so I don’t see why this should be deferred.> There is no ratio for that, it can be all easy, or > many hard, and it will depend on the case. If we make all rules fixed > at maximum, we will lose the ability to import code and evolve with > time. > > One of the key points, and the first one indeed, is that the community > has to be pro-active and nice. If we request a bunch of changes, they > don't change and request to be made officialI have a totally different opinion: they submit a patch, we request changes, and if they don’t the code never gets in tree in the first place.> , then they have failed > the first and most important rule. > > OTOH, if we failed to spot the problems in the first bulk, and later > accept a bad target as official, than it would be our own fault.Asking as much separate patches as possible is part of making sure it does not happen. — Mehdi> > We can add a note describing the FIXME approach if people think it > will make things more clear, but essentially, the intent is in there > already. I'm ok with being more explicit, though, just not more > strict. > > cheers, > --renato