Target Acceptance Policy

Hi Gerolf,

This is more the third stage, “how to be good target maintainers”. I think we could add a few guidelines somewhere, as I agree this is really important, but maybe not in this section.

I imagined this as a minimum requirements section, not as a guidelines. Though, we could think as public CI maintenance as a minimum requirement for target maintainers,too.

I’m open to suggestions…

Cheers,
Renato

From: "Renato Golin" <renato.golin@linaro.org>
To: "Hal Finkel" <hfinkel@anl.gov>
Cc: "LLVM Dev" <llvm-dev@lists.llvm.org>, "Mehdi Amini" <mehdi.amini@apple.com>
Sent: Tuesday, July 26, 2016 2:16:51 PM
Subject: Re: [llvm-dev] Target Acceptance Policy

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

Sure; have we accepted backends in the recent past which contained code that did not meet our coding conventions?

-Hal

I’m no authority, but when people have a back end to contribute, they have already done some due diligence, and the conventions are usually mostly there.

But because the target was implemented off tree, there was less peer review, and why we may accept some minor violations if they get fixed later. Usually things that clang format can’t fix.

For more accurate information, you could look into the recent merges: bpf, system z, apple’s ARM64, lanai. They all had different histories. Also arm’s AArch64, which was mostly brewed inside tree, but a good part was copied from the arm back end (I even found the same bugs on both) :wink:

I personally see all those efforts as successful, each with their own quirks, maybe none of them perfect, but definitely better with them than without.

Maybe I’m being too optimistic?

Cheers,
Renato

From: "Renato Golin" <renato.golin@linaro.org>
To: "Hal Finkel" <hfinkel@anl.gov>
Cc: "Mehdi Amini" <mehdi.amini@apple.com>, "LLVM Dev" <llvm-dev@lists.llvm.org>
Sent: Tuesday, July 26, 2016 7:11:29 PM
Subject: Re: [llvm-dev] Target Acceptance Policy

> Sure; have we accepted backends in the recent past which contained
> code that did not meet our coding conventions?

I'm no authority, but when people have a back end to contribute, they
have already done some due diligence, and the conventions are
usually mostly there.

But because the target was implemented off tree, there was less peer
review, and why we may accept some minor violations if they get
fixed later. Usually things that clang format can't fix.

Like what? Minor things are generally things that are easy to ask to be changed in a code review.

For more accurate information, you could look into the recent merges:
bpf, system z, apple's ARM64, lanai. They all had different
histories. Also arm's AArch64, which was mostly brewed inside tree,
but a good part was copied from the arm back end (I even found the
same bugs on both) :wink:

I personally see all those efforts as successful, each with their own
quirks, maybe none of them perfect, but definitely better with them
than without.

I agree.

-Hal

Hi Gerolf,

This is more the third stage, “how to be good target maintainers”. I think we could add a few guidelines somewhere, as I agree this is really important, but maybe not in this section.

I imagined this as a minimum requirements section, not as a guidelines. Though, we could think as public CI maintenance as a minimum requirement for target maintainers,too.

I’m open to suggestions…

I could see the expectation for future maintenance as part to the requirements. I understand this would expand your intent to cover (aspects) of the entire backend lifecycle. But I feel that this is important to be explicit about and have it documented.

I’m failing to reconcile what you’re claiming above with the following that is in your proposal:

"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”

I interpret this as "code that does not respect policy and/or coding standards should be fine to be committed as experimental”, how should I read that?

Minor problems like not using range based loops, too much use of std string, variable naming. Each own could be a simpler task, but rebasing (and validating) a large patch set for every review can be daunting.

I particularly don’t feel strongly about caps on first letter of variable names, for instance, so I would be fine if a big renaming happening after landing, if the developer is already making other big and more important changes to the code base.

Cheers,
Renato

There is point 5, but it’s not explicit about after going official. Should be fine to add something with that regard, yes.

Thanks!
Renato

I expected the “this can take a while” to be the hint. You may have read it as “we will accept anything” but I certainly didn’t write it.

My answer to Hal should provide you with the context you need. I’ll try to reword that point a bit better tomorrow.

Re-cap, after reviews. Main changes:

* Making it clear that the "active community" behaviour is expected to
continue throughout the target's lifetime.
* Making it clear that only a reduced set of violation will be allowed
in the experimental phase, providing the maintainers are taking the
cost to move it to full compliance.
* Trust, but verify: If the target's community doesn't follow these
assumptions, the target will be removed.

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. This
behaviour is expected to continue throughout the lifetime of the
target's 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 to LLVM's own.

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 completely adapted to the
developers policy as well as the coding standards. Any exceptions that
were made to move into experimental mode must have been fixed to
become official.

7. The test coverage needs to be broad and well written (small tests,
well documented). The build target ``check-all`` must pass with the
new target built, and where applicable, the ``test-suite`` must also
pass without errors, in at least one configuration (publicly
demonstrated, ex. via buildbots).

8. Public buildbots need to be created and actively maintained, unless
the target requires no additional buildbots (ex. ``check-all`` covers
all tests). The more relevant and public CI infrastructure a target
has, the more the LLVM community will embrace it.

To continue as a supported and official target:

9. The target maintainer must continue following these rules
throughout the lifetime of the target. Violations of the policy will
be treated on a case by case basis, but several and continuous
violations could be met with complete removal of the target from the
code base.

I hope that's better. To me, it feels a bit pushy, but people seemed
to prefer a bit more rigorous text.

We can always change it in the future, of course, and reiterating,
this is not intended to change the behaviour of the community (which
is already good), but to encode it and show others what do we mean by
"target responsibility".

cheers,
--renato

...and this requirement pushes conversion of Lanai back-end to an official
target status to Sep 28th as earliest. Just saying.

[Personally, I believe that given the level of support, Lanai is already
ready for an official status. But if we codify "must have" rules, they
should be obeyed by everyone, no? Otherwise, we are setting a very
dangerous precedent.]

Yours,
Andrey

I’m not hung up on the fixed number of months. I don’t even think it’s the best idea, but I was expecting people to give their own ideas… :slight_smile:

But saying “as soon as they are ready” may be hard to assess. And writing it specific for the Lanai back end would not be appropriate.

I’m open to ideas… Please share! :slight_smile:

–renato

IMHO, this is a very important point missed from the discussion so far.

What if a new back-end ticks all the right boxes but simply not being
reviewed by established members of the community? I expect this to be quite
common, especially with LLVM broaden in popularity and reaching beyond its
traditional developer / user base. We all have busy lives and it's
understandable that existing core developers / maintainers have no time nor
inclination to thoroughly review every new back-end. When enough is enough
and it's OK to press "commit" button?

Yours,
Andrey

Well, It's easy to criticize, but much harder to create new ideas... So I
specialize in former. :slight_smile:

I have only two suggestions:
1) Leave the wording as is, and make Lanai an official back-end no earlier
than Sep 28th.
2) Downgrade the wording from "must have" to "suggested" and add something
like "can be transformed to official status earlier if there is enough
support from LLVM maintainers". We can even formalize what "enough support"
means -- say, at least three maintainers employed by other companies LGTMed
this.

Yours,
Andrey

This case is clear. The code only goes in after proper review.

Interests vary from case to case, and normally, pinging on the mailing
list and on IRC is the only way to go. Waiting forever doesn't work,
and asking a friend maintainer to "push it for you" or just pushing
yourself is very much *never* appropriate.

This is one of the few clear cases I can think of that would warrant a
permanent commit ban for all involved.

Upstream takes time, we all have to live with it. But the alternative
is worse. :slight_smile:

cheers,
--renato

1) Leave the wording as is, and make Lanai an official back-end no earlier
than Sep 28th.

I don't want to *have* to do that just because we introduced a policy
after the Lanai back-end started the process...

And making Lanai official just before the policy goes public would be cheeky. :slight_smile:

2) Downgrade the wording from "must have" to "suggested" and add something
like "can be transformed to official status earlier if there is enough
support from LLVM maintainers". We can even formalize what "enough support"
means -- say, at least three maintainers employed by other companies LGTMed
this.

A bit too specific, but I think we can make this work... I'll try again.

cheers,
--renato

Renato,

I’m not speaking on ways to circumvent the review system, but rather on when enough is enough.

Do you expect whole back-ends to receive the same level of code review as individual incremental patches to existing ones? If yes, by who? In case of existing back-ends, there are maintainers who understand the code and able to do a proper code review; who can do the same for new back-ends?

Can we put some formal expectations into your guidelines?

Yours,
Andrey

Trying to re-write based on Andrey's response:

5. The target must have been stable in the tree and have addressed
every other minimum requirement for at least 2 months. This cool down
period is to make sure that the back-end and the target community can
endure continuous upstream development for the foreseeable future.

I think that's better than 6 months because it makes it explicit what
is the need for this (cool down period, demonstration of stability),
which I personally think it's a very important one and we shouldn't
get away with it.

It also changes from "landing in tree" to "having addressed all
points", so if the back-end lands in tree already fully conforming,
there's no point in waiting another 4 months just because.

Does that look better?

Also, "having addressed the minimum requirements" may end up being
subjective, but I think we should keep it that way on purpose. I don't
want to have to come up with a "committee of target approval" or
anything like that. We're doing well as we do.

cheers,
--renato

While I think the 6-month mark is artificial (what's wrong about a vague
"several months"? these are policies/guidelines, not legal contracts), FWIW
we (Lanai maintainers) don't particularly mind to wait until Sep 28 if
that's deemed important by the community. There's no rush, and we don't
request any special-casing here.

Eli

While I think the 6-month mark is artificial (what's wrong about a vague
"several months"? these are policies/guidelines, not legal contracts),

I'm ok with that, too.

Though, what do you think about the "2 months after all done"? Maybe
"at least 2"?

Giving a number makes people think less about the uncertainties, and
be more accepting, I think.

FWIW
we (Lanai maintainers) don't particularly mind to wait until Sep 28 if
that's deemed important by the community. There's no rush, and we don't
request any special-casing here.

I know. And I think you guys are "at least 2 months with all bullets
checked", which for me is the important bit.

People that don't work on LLVM (or other large upstream projects)
don't quite get the volume of changes and interactions that are
needed.

We just need to make sure communities that want to be part of LLVM
show they understand.

cheers,
--renato