This is an archive of the discontinued LLVM Phabricator instance.

[zorg] Add Polly builder for ARM target
ClosedPublic

Authored by pzheng on Jan 9 2017, 3:12 PM.

Diff Detail

Event Timeline

pzheng updated this revision to Diff 83711.Jan 9 2017, 3:12 PM
pzheng retitled this revision from to [zorg] Add Polly builder for ARM target.
pzheng updated this object.
pzheng added a subscriber: llvm-commits.
grosser edited edge metadata.Jan 9 2017, 7:47 PM

Very nice!

Some small questions from my side. What is different in the getPollyARMBuildFactory compared to getPollyBuildFactory? In the optimal case, we just have one build factory that is parametrized to the specific builder, no? Maybe some of the changes could be moved to the getPollyBuildFactory that all Polly builders can benefit. We could e.g. make the 'make-command' just a parameter, which is set to 'ninja' for the ARM builder. Also, while at it, I would not call the build command 'Build Polly', but rather 'Build or Build+LLVM'. The reason is that this will come up in the error messages when a build fails. In many cases such failures are due to errors in LLVM, but if the name implies this is Polly-only people might skip those mails by accident.

Some comments on the implementation, but I share Tobias's concerns. Why not re-use the existing Poly builder?

buildbot/osuosl/master/config/status.py
174

You don't need to add this to new builders.

zorg/buildbot/builders/PollyBuilder.py
222

Paths should be configuration parameters (see the ARM buildbots)

257

Targets and triples should be configuration parameters (see the ARM buildbots)

273

Number of jobs should be a configuration parameter (see the ARM buildbots)

288

Number of jobs should be a configuration parameter (see the ARM buildbots)

295

Number of jobs should be a configuration parameter (see the ARM buildbots)

pzheng updated this revision to Diff 83905.Jan 10 2017, 5:36 PM
pzheng marked 5 inline comments as done.Jan 10 2017, 5:44 PM

Thanks for reviewing the patch!
I have refactored getPollyBuildFactory to take a few additional arguments and got rid of getPollyARMBuildFactory so that the builder for ARM also uses getPollyBuildFactory. Also addressed a few other comments from grosser and rengolin.

pzheng added inline comments.Jan 10 2017, 5:48 PM
buildbot/osuosl/master/config/status.py
174

We want to have someone monitor the builder, thus adding this extra stuff.

grosser accepted this revision.Jan 10 2017, 10:33 PM
grosser edited edge metadata.

Thank you!

This revision is now accepted and ready to land.Jan 10 2017, 10:33 PM
rengolin added inline comments.Jan 11 2017, 1:19 AM
buildbot/osuosl/master/config/status.py
174

This should really pick up from the bot owner's email, not this hard-coded address. But that's not for this patch.

zorg/buildbot/builders/PollyBuilder.py
100

Why do you need the install dir if you're not using it in the buildbot?

pzheng added inline comments.Jan 11 2017, 10:34 AM
zorg/buildbot/builders/PollyBuilder.py
100

The install dir will be used by another builder. More patches coming soon.

This revision was automatically updated to reflect the committed changes.