About commit TILE-Gx backend to community repository and default disabled

Hi Chris,

could you please comment on committing TILE-Gx backend into community?

========== TILE-Gx Status ===========

Features Supported

Hi Chris,

could you please comment on committing TILE-Gx backend into community?

Hi Jiong,

I don't have any special advice here. It sounds like the general functionality level is high enough. Taking it into mainline sounds great, so long as it is reviewed by someone.

-Chris

thanks.

   I will commit after rebasing & re-testing the code on latest llvm mainline. And I will follow http://llvm.org/docs/HowToAddABuilder.html to setup a TILE-Gx buildbot.

Hi Jiong,

did you receive a full review by an LLVM committer who approved the full patch set (not just individual ones) with some kind of "Looks good to me"? This is the requirement Chris put for inclusion and only after this has happened (and there are no major concerns found during review) you should commit the back end.

I am myself very supportive on getting this in, but I have the feeling this did not yet happen. So I propose to not commit this backend prematurely. (In case I missed something and this review already happened, would you mind pointing me to the email and committer who reviewed the backend)

Thanks,
Tobias

于 2013/3/23 23:14, Tobias Grosser 写道:

> Hi Jiong,

did you receive a full review by an LLVM committer who approved the
full patch set (not just individual ones) with some kind of "Looks
good to me"? This is the requirement Chris put for inclusion and only
after this has happened (and there are no major concerns found during
review) you should commit the back end.

I am myself very supportive on getting this in, but I have the feeling
this did not yet happen. So I propose to not commit this backend
prematurely. (In case I missed something and this review already
happened, would you mind pointing me to the email and committer who
reviewed the backend)

Hi Tobias,

thanks for your reply, it's happy to receive more feedbacks.

below are preivous community feedbacks,

===
my first post to llvm-commit mailing list:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130304/167737.html

my second post to llvm-commit mailing list which split the big patch
into 17 small parts:

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130311/167891.html

Anton Korobeynikov, Jakob Stoklund Olesen, Joerg Sonnenberger given some
feedback on coding style etc, all fixed.

do you think it's OK ? please feel free to point out if there is
anything missing.

Yes, I have seen this and it is a very good start. Some of our core contributors are giving you feedback to this back-end and you are addressing their remarks. This is great.

With Chris's reply you also got the general OK to upstream the patch, _AFTER_ the actual patches have been reviewed.

I am unsure to which extend Anton, Jakob and Joerg have reviewed your back-end, but from the emails I have seen, I am not convinced they are finished. I personally believe for a patch set of this size, you should
wait for an explicit email of one of the core contributors which states that the _full_ patch set has been reviewed and is good to commit or that an individual patch can be committed by itself.

All the best,
Tobias

One thing I was asking on IRC for is whether it makes sense for new
backends to be committable in incremental steps. Especially for a
complete backend like this, e.g. the asm parser would make a reasonable
and useful part to get in and limit the amount of code to be reviewed at
one point.

Joerg

于 2013/3/24 0:07, Joerg Sonnenberger 写道:

With Chris's reply you also got the general OK to upstream the
patch, _AFTER_ the actual patches have been reviewed.

One thing I was asking on IRC for is whether it makes sense for new
backends to be committable in incremental steps. Especially for a
complete backend like this, e.g. the asm parser would make a reasonable
and useful part to get in and limit the amount of code to be reviewed at

Hi Tobias & Joerg,

I understand your concern.

yes, Asm Parser is a module which could be committed individually, but I think the code
for AsmParser is relatively small and highly target dependent, a full regression test which
cover all instructions could largly gurantee the correctness.

I know contributors have their own works. For TILE-Gx backend, it's good if any contributor could have time to finish a full set review, I have been wating for several weeks.

I personly think it's not premature to commit TILE-Gx backend into mainline and default disable. Because:

1. TILE-Gx finished the regression test & test-suite test.
2. TILE-Gx buildbot will be setup, so it will be convinent for both community & Tilera company to monitor the status.

So should I keep waiting the full set review? or it's OK to commit and default disabled?

The AArch64 backend was committed fairly quickly after being proposed for
inclusion, and they did not split up the patch and most of their backend
was committed in one huge commit.

Although IIRC Bill Wendling (CC'd) explicitly asked you to split the
backend into separate patches, it might be best to put it back together
again for the final review. AFAIK Phabricator does not have a good way to
keep all the patches somehow together, and otherwise they just get lost in
people's mail (and it would be annoying to ping 10+ patches). Having a
single patch avoids this problem and makes it a lot easier to keep track of
the backend. Phabricator has good support for commenting on specific parts
of the patch, so as long as the backend components are named in a
recognizable way (I think they are), experienced reviewers should not have
significant difficulty navigating (and they can always apply the patch
locally if necessary, which is easier with one huge patch).

-- Sean Silva

Hi Sean, I appreicate your objective comments here. Anyway, I just finished rebase and retest TILE-Gx backend on the latest llvm mainline, and will re-send the patch on Phabricator for final review. — Regards, Jiong Tilera Corporation

Hi Chris,

could you please comment on committing TILE-Gx backend into community?

Hi Jiong,

I don't have any special advice here. It sounds like the general
functionality level is high enough. Taking it into mainline sounds great,
so long as it is reviewed by someone.

  thanks.

  I will commit after rebasing & re-testing the code on latest llvm
mainline. And I will follow http://llvm.org/docs/**HowToAddABuilder.html<http://llvm.org/docs/HowToAddABuilder.html>to setup a TILE-Gx buildbot.

No, read what he said again: so long as it is reviewed by someone. That
hasn't happened yet, and you need it to happen before you commit.

As to how you can get the entire backend reviewed, I gave advice on that
front in my initial response to your initial email, the very first response
you got:

Unfortunately, this is a *huge* amount of code, and so it is likely to take

quite some time to really get reviews on all of it. Please be patient in
that regard.

The best thing you can do to help accelerate the process is to dive in and

contribute to the shared parts of the open source project to help the
current maintainers, and maybe even free up some of their time to review
your patches. Because LLVM is open source, it is really important to the
project for folks working on a specific target, backend, or application to
also help share the ongoing maintenance and improvement costs of the core
shared infrastructure in the compiler.

-Chandler