Target Acceptance Policy

Folks,

In recent discussions about accepting the Lanai back-end as official
(built by default), there was the idea that we should have some
official guidelines, if not rules, that all targets must (should?)
follow when requesting to be added upstream, and later, to be
officially supported.

Here's a list of what more or less converged into the thread with my
personal bias.

Must haves to be accepted upstream as experimental:

1. There must be an active community behind the target. This community will be
  the maintainers of the target by providing buildbots, fixing bugs, answering
  the LLVM community's questions and making sure the new target doesn't break
  any of the other targets, or generic code.

2. The code must be free of contentious issues, for example, large
changes in how
  the IR behaves or should be formed by the front-ends, unless agreed by the
  majority of the community via refactoring of the (LangRef) *before*
the merge of
  the new target changes, following the IR backwards compatibility described in
  the developer's policy document.

3. The code has a compatible license, patent and copyright statements, or can be
  converted by the LLVM's own model.

Where applicable:

4. The target should have either reasonable documentation on how it works (ISA,
  ABI, etc.) or a publicly available simulator/hardware (either free
or cheap enough),
  so that developers can validate assumptions, understand constraints and review
  code that can affect the target. Preferably both.

To be moved as an official target:

5. The target must have been in the tree for at least 6 months, with active
  contributions including: adding more tests conforming to the documents, fixing
  bugs reported by unrelated/generic changes, providing support to other members
  of the community.

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.

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).

My takes on those:

I don't think we should avoid 1, 2 and 3 for any reasons, or we'll be
shooting ourselves in th foot.

4 is only applicable to actual executing targets, and may clash with
existing targets like GPUs, BPF, etc. While I'd *really* like to have
some execution model for GPUs, I'm not prepared to recommend
jettisoning their back-ends without it. :slight_smile:

5 states "6 months" as a ball-park. I'm not too fixed on that, and I
welcome ideas if anyone feels strongly about time taken. I think it
needs to give us enough time to assess the *other* points (es.
community, tests, validation, response).

6 I think it's a strong factor and I feel very strongly about it. We
had projects before merging with other code styles and policies, but
as an "LLVM developer", I feel I should be able to contribute to any
"officially branded LLVM project". So, if those targets want to be
part of *our* community, they have to abide by the same rules.

7 is hard to know, and I'm not sure how much we can really enforce.
Maybe demanding at least one public buildbot? What if the target can't
execute or even run clang? Wouldn't the target's tests on other bots
be enough?

Opinions?

cheers,
--renato

+1, some sort of defined process is definitely needed. In particular I think there should be a guideline for when the backend has been reviewed "enough" before it can be committed.

-Matt

Folks,

In recent discussions about accepting the Lanai back-end as official
(built by default), there was the idea that we should have some
official guidelines, if not rules, that all targets must (should?)
follow when requesting to be added upstream, and later, to be
officially supported.

Here's a list of what more or less converged into the thread with my
personal bias.

Must haves to be accepted upstream as experimental:

1. There must be an active community behind the target. This community will be
the maintainers of the target by providing buildbots, fixing bugs, answering
the LLVM community's questions and making sure the new target doesn't break
any of the other targets, or generic code.

2. The code must be free of contentious issues, for example, large
changes in how
the IR behaves or should be formed by the front-ends, unless agreed by the
majority of the community via refactoring of the (LangRef) *before*
the merge of
the new target changes, following the IR backwards compatibility described in
the developer's policy document.

3. The code has a compatible license, patent and copyright statements, or can be
converted by the LLVM's own model.

Where applicable:

4. The target should have either reasonable documentation on how it works (ISA,
ABI, etc.) or a publicly available simulator/hardware (either free
or cheap enough),
so that developers can validate assumptions, understand constraints and review
code that can affect the target. Preferably both.

To be moved as an official target:

5. The target must have been in the tree for at least 6 months, with active
contributions including: adding more tests conforming to the documents, fixing
bugs reported by unrelated/generic changes, providing support to other members
of the community.

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.

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.

  1. 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.

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:

  1. 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.

There is a nuance: the fact that it is not developed in tree does not prevent from splitting it in “some” pieces that can be independently reviewed, this is what we (should) do for any large chunk of code that we integrate.
Now it is possible that considering the current way of implementing backend in LLVM, there is not much pieces that can be split out of a backend (I still think that’s unfortunate though).

  1. 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.

There is a nuance: the fact that it is not developed in tree does not prevent from splitting it in “some” pieces that can be independently reviewed, this is what we (should) do for any large chunk of code that we integrate.
Now it is possible that considering the current way of implementing backend in LLVM, there is not much pieces that can be split out of a backend (I still think that’s unfortunate though).

Sure, we should definitely split it in ways that make sense.

I was just pointing out that this won’t be the same thing as what “incremental development” would have naturally led to if the backend had been in tree on day-one, and while we actually insist on nearly replaying incremental development for some things, it doesn’t seem likely to make sense for a backend.

  1. 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.

  1. 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.

There is a nuance: the fact that it is not developed in tree does not prevent from splitting it in “some” pieces that can be independently reviewed, this is what we (should) do for any large chunk of code that we integrate.
Now it is possible that considering the current way of implementing backend in LLVM, there is not much pieces that can be split out of a backend (I still think that’s unfortunate though).

Sure, we should definitely split it in ways that make sense.

I was just pointing out that this won’t be the same thing as what “incremental development” would have naturally led to if the backend had been in tree on day-one, and while we actually insist on nearly replaying incremental development for some things, it doesn’t seem likely to make sense for a backend.

I think we’re on the same page.

The stake to me is not “incremental development” in this case but “modularity” and “good testing”, the two usually needs/feeds each other, and ultimately help to have an efficient review.

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.

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.

Indeed. Minimum requirements, not expected target.

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.

Indeed. Expected target, not minimum requirements. :slight_smile:

Bu having them separated, we're telling people that the latter is what
they should strive for, but we're happy to help them along the way.

> 7. The test coverage is broad and well written (small tests,
> documented).

Yep, this seems like it should always be true regardless of the experimental
nature.

I think this is the same as above. New targets are generally
introduced by people that don't know LLVM too well, or haven't written
a full back-end before.

Expecting *full* coverage and perfect tests from day one is as
unreasonable as expecting them to fit all the required code style and
other bits.

But in order for it to be relevant upstream (ie. no maintenance
burden), the tests need to be in good quality. After all, a lot of
unrelated buildbots will be running *their* tests.

It's the same thing: start small, grow up, become official. All in
all, we're here to help them grow. That's the message I'm trying to
pass along.

cheers,
--renato

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

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

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

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 official

I 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.

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.

I think requiring such a high bar from start is not a good community
sportsmanship.

I want to help other people to understand and follow our guidelines,
and the best place to do this, is to have an experimental stage, where
the back-end is in tree but not officially built. This gives them time
to address all comments with our help. We want targets as much as they
want to be in LLVM. It's a cooperation and acting inflexibly won't
help anyone.

Also, if we put all the blockages in the first stop, what's the point
of being experimental in the first place?

There is no edge cases in question here.

Chandler has found a number of edge cases on my original (more strict)
writing. This is a revised text, and by all means, will have cases
that we didn't foresee.

I 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.

I hear, loud and clear. I also understand the reasons. I just don't
share your opinion that this is valid in *every* case.

We usually reserved strong policies for critical problems (legal,
license, patents, copyright), and less strict ones for more common
sense kind of things. May not be perfect, but it's a balance.

cheers,
--renato

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.

I think requiring such a high bar from start is not a good community
sportsmanship.

Yet this is exactly what we do for any new contributor that wants to add a new pass in the compiler for instance, even if it is not enabled in the default pipeline (which is equivalent to the experimental stage somehow).

I want to help other people to understand and follow our guidelines,
and the best place to do this, is to have an experimental stage, where
the back-end is in tree but not officially built.

I disagree, the best place for that is with a patch reviewed.

It is very hard to evaluate how much coverage components gets with lit-based test without doing it while integrating patches.
I just don’t see any viable alternative, re-evaluating the full target and its tests every other month is just *not* practical.

I you don’t have a good way to integrate the patches in good conditions (i.e. with conferment coding style and good tests), you’ll never get there later (or at very high cost).

This gives them time
to address all comments with our help. We want targets as much as they
want to be in LLVM. It's a cooperation and acting inflexibly won't
help anyone.

I believe that it helps everyone on the opposite.

Also, if we put all the blockages in the first stop, what's the point
of being experimental in the first place?

All the rest of what you described in (show that the maintainer is active, etc.), and also, waiting for the implementation to be “complete” (as in “all patches integrated”).

I'm sorry, but I'll never subscribe to inflexibility, no matter the argument.

I think we have reached the point where the conversation is less
useful because the assumptions are radically opposing.

I want to hear other people's opinions, too, so we can reach a
consensus, not just the opinions of two loud guys talking. :slight_smile:

cheers,
--renato

From: "Renato Golin via llvm-dev" <llvm-dev@lists.llvm.org>
To: "Mehdi Amini" <mehdi.amini@apple.com>
Cc: "LLVM Dev" <llvm-dev@lists.llvm.org>
Sent: Tuesday, July 26, 2016 12:40:58 PM
Subject: Re: [llvm-dev] Target Acceptance Policy

>> This gives them time
>> to address all comments with our help. We want targets as much as
>> they
>> want to be in LLVM. It's a cooperation and acting inflexibly won't
>> help anyone.
>
> I believe that it helps everyone on the opposite.

I'm sorry, but I'll never subscribe to inflexibility, no matter the
argument.

I think there are different kinds of inflexibility. We will use our collective professional judgment. There are some large-scale design changes that we might decide can happen over time. Whatever we decide to accept, however, needs to be in good shape for what it is and follow our coding conventions.

-Hal

Absolutely. There is a large range of solutions, and we have been most
successful on the ones we picked over the years. I think we should
continue with the trend.

What (I think) I have proposed is nothing different than what we've
been doing (ie. I'm not trying to change the status quo).

So, if that's not what's coming out, my wording is wrong, please
advise. If it is, than I don't think we should argue *now*.

I just want to encode what is, and then, once we have something in,
working and actively helping people add their targets, we can discuss
(in separate) the merits of our current policies.

Maybe I should have said that from the beginning. Oh well, hind sight and all.

--renato

IIUC you are writing specifically that an experimental backend can be integrated without conforming to http://llvm.org/docs/DeveloperPolicy.html#quality ; I don’t think this is a current “policy” but your writing is making it a policy AFAICT.

I think there are different kinds of inflexibility. We will use our collective professional judgment. There are some large-scale design changes that we might decide can happen over time. Whatever we decide to accept, however, needs to be in good shape for what it is and follow our coding conventions.

Absolutely. There is a large range of solutions, and we have been most
successful on the ones we picked over the years. I think we should
continue with the trend.

What (I think) I have proposed is nothing different than what we've
been doing (ie. I'm not trying to change the status quo).

All this is really good. Do you also have thoughts on guidelines for accessibility to bots and debugging support? When a patch causes failures on bots testing a backend, what support do you expect to help root cause the issue? This can be a big burden on developers not familiar with an architecture.

I’m most certainly not. Just because I didn’t write something, that I means I have written the opposite.

I could perhaps add “fully adapted” on point 6, to help make it clearer. Would that help?

But as with what was said in the code of conduct, exhausting all possibilities is not only exhausting, but pointless.

We know how to do code review, we know how to follow the policies, and we can collectively take decisions without needing to resort to rules and regulations.

This piece is just the last resort, minimum requirements. Not a complete description of everything we do.

I really thought this would be a no brainer, that every was on board with what everyone already does. Seems I was, again, wrong.

Cheers,
Renato