Page MenuHomePhabricator

[PowerPC] [Zorg] Change ppc64le-rhel bot to unified tree instead
ClosedPublic

Authored by Conanap on Apr 23 2021, 1:38 PM.

Details

Summary

ppc64le-rhel bot is changed to use unifiedtree builder instead as it has
a factory method with cmake and ninja.

I'm not exactly sure about nt-flags in the original clang builder, so
I would like to see how the buildbot handles this for now. The bot
will be kept on staging for now.

Diff Detail

Event Timeline

Conanap created this revision.Apr 23 2021, 1:38 PM
Conanap requested review of this revision.Apr 23 2021, 1:38 PM

The point of the nt_flags= is to not overload the machine. We have noticed in the past that if we let the tests run all at the same time without a thread limit we sometimes get intermittent failing tests.
I think this is an option we should try to keep.

buildbot/osuosl/master/config/builders.py
624–625

There are a number of projects listed here for DLLVM_ENABLE_PROJECTS.
However, we only list llvm in depends_on_projects. I feel like the right way to do this would probably be to add the projects to the depends_on_projects.
Also, inside UnifiedTreeBuilder:

CmakeCommand.applyDefaultOptions(cmake_args, [
    ('-DLLVM_ENABLE_PROJECTS=', ";".join(f.depends_on_projects)),
    ])
626

Does this mean that every time the bot runs we will be doing a clean first? This seems like it will slow down the bot quite a bit.

Conanap updated this revision to Diff 340516.Apr 26 2021, 7:16 AM
Conanap marked 2 inline comments as done.

Changed clean to false, removed a flag in favour of passing into arguments.

jsji added inline comments.Apr 26 2021, 7:23 AM
buildbot/osuosl/master/config/builders.py
625

This may get same bug as ClangBuilder. Can you review https://reviews.llvm.org/D99685 or even integrate it into this, so that we will get compiler-rt changes in buildbot as well.

Thanks, Albion.

Please see my comments inline.

buildbot/osuosl/master/config/builders.py
625–629

No need to specify this. The build factory will take care of the runtimes. It is enough to specify it in the depends_on_projects.

626

No need to specify clean param explicitly, if you do not want to force a clean build on each commit. The default should do a good job for you.

627

Make this a list, please.

636–637

Your worker specifies 4 jobs. But you hard-code here 20 for testing. Could you elaborate why, please?

Conanap added inline comments.Apr 30 2021, 10:04 AM
buildbot/osuosl/master/config/builders.py
636–637

that's actually a really good point... I'll have to look into that. Thanks for the catch!

Conanap updated this revision to Diff 343587.May 6 2021, 11:09 PM
Conanap marked 5 inline comments as done.

Added compiler-rt compatibility, removed thread limit argument. Bot was tested on a local server.

Conanap updated this revision to Diff 343723.May 7 2021, 11:15 AM

Put back -j20 as we're seeing too many threads being used.

Conanap added inline comments.May 7 2021, 1:05 PM
buildbot/osuosl/master/config/builders.py
625

This new patch was able to pick up the compiler-rt changes during testing, so your patch should no longer be needed :)

636–637

I believe we needed -j 20 to prevent too many threads from being utilized - that said, I'm looking at how many jobs we can use for build right now, FYI

Conanap marked an inline comment as not done.May 7 2021, 1:05 PM
Conanap updated this revision to Diff 343931.May 9 2021, 1:30 PM

Updated worker to use 8 jobs

Conanap updated this revision to Diff 344870.May 12 2021, 10:42 AM

Updated buildbot worker to use 192 jobs.

@gkistanova I've tested this on a local test server and it seems to run alright. We've changed the job number to 192 and found it to be stable. However, the lld will remain at 20 as more jobs seem to cause some issues on our machines. Please take a look when you get a chance, thank you!

gkistanova accepted this revision.EditedMay 12 2021, 4:27 PM

LGTM.

Before committing, could you make sure you are based on the current UnifiedTreeBuilder and LLVMBuildFactory, please?
If I remember the context correctly, you may need to specify enable_runtimes="auto", since the default behavior has been changed in https://reviews.llvm.org/rZORGf7b888457641 per request.

However, the lld will remain at 20 as more jobs seem to cause some issues on our machines.

Yes, I noticed that lld memory usage has increased recently.

This revision is now accepted and ready to land.May 12 2021, 4:27 PM
jsji requested changes to this revision.May 12 2021, 7:40 PM

We want to use runtime build for RHEL bots, so we should set `enable_runtimes=['compiler-rt'] to UnifiedTreeBuilder.

This revision now requires changes to proceed.May 12 2021, 7:40 PM
jsji added a comment.May 12 2021, 7:44 PM

If I remember the context correctly, you may need to specify enable_runtimes="auto", since the default behavior has been changed in https://reviews.llvm.org/rZORGf7b888457641 per request.

Or yes, use enable_runtimes="auto" here should work too.

Conanap updated this revision to Diff 345129.May 13 2021, 7:09 AM

Added enable_runtime="auto".

Thanks @gkistanova ! I've added the option and tested it on a local server, seems to be pretty happy.
@jsji I've updated the patch, please take a look when you get a a chance. Thanks!

jsji accepted this revision.May 13 2021, 7:11 AM

Thanks.

This revision is now accepted and ready to land.May 13 2021, 7:11 AM
Conanap updated this revision to Diff 345133.May 13 2021, 7:18 AM

Removed an unintended change

Conanap closed this revision.May 14 2021, 7:31 AM

patch did not auto close; was pushed.

patch did not auto close; was pushed.

For future reference, I think the syntax you need for auto closing (arc creates this automatically) is:

Differential Revision: DXXXX

patch did not auto close; was pushed.

For future reference, I think the syntax you need for auto closing (arc creates this automatically) is:

Differential Revision: DXXXX

Ah, nope, full URL:

Differential Revision: https://reviews.llvm.org/DXXXX

ah maybe I missed the word Revision, thank you!