Page MenuHomePhabricator

Make LLVM_TARGETS_TO_BUILD=all build all targets
Needs ReviewPublic

Authored by rsmith on Jun 30 2017, 2:23 PM.

Details

Summary

For LLVM development, we want a mode that builds everything; this is what the buildbots that say "all targets" should be testing. (All code in tree should build and pass its tests, even if it's an experimental target.) However, LLVM_TARGETS_TO_BUILD=all currently (surprisingly) only builds non-experimental targets. (Additional targets can be specified via LLVM_EXPERIMENTAL_TARGETS_TO_BUILD, which confusingly is just a list of more targets to build in addition to those in LLVM_TARGETS_TO_BUILD.)

This patch addresses this as follows:

  • LLVM_TARGETS_TO_BUILD=all builds all targets
  • A new value, LLVM_TARGETS_TO_BUILD=stable, builds only non-experimental targets
  • The default for LLVM_TARGETS_TO_BUILD is changed from all to stable.

I've left LLVM_EXPERIMENTAL_TARGETS_TO_BUILD alone for now. I think it'd make more sense to be able to specify something like LLVM_TARGETS_TO_BUILD=stable;WebAssembly to add experimental targets to the stable targets list, but I don't see a reason to couple such a change to this change.

Diff Detail

Repository
rL LLVM

Event Timeline

rsmith created this revision.Jun 30 2017, 2:23 PM
MatzeB added a subscriber: MatzeB.EditedJun 30 2017, 2:33 PM

Changing the meaning of "all" to really mean all targets makes sense. But there is of course a fundamental discussion here:

I think we explicitely do NOT build the experimental targets in any of the normal buildbots. I think it is fair to leave the burden of updating the experimental targets to whoever maintains them. I do make sure that I do not break compilation of experimental targets when I make changes to the API. However I do not want to start debugging and updating even more tests. This is always a huge pita on architectures you don't know, have no simulators/documentation about, may just have bugs in the target specific code that are triggered by your changes etc.
To have an example: Let's say a valid CodeGen change produces slightly different code that hits a "Not Implemented Yet" assert in the experimental target, I don't wanna implement that just to keep the tests running. Even XFAILing the affected tests, filing PRs for them etc. is a big hassle.

We should leave that reponsibility with the maintainers of the experimental targets. Which means we should only have "silent" bots for the experimental targets, i.e. only the target maintainers get failure mails and they are responsible for fixing their target.

silvas added a subscriber: silvas.

IIRC the intention was that in-tree work should not be burdened by experimental targets, up to and including breaking them. E.g. an in-tree API migration wouldn't have to update experimental targets. So in that sense LLVM_TARGETS_TO_BUILD=all is working as intended I think.

Maybe Renato remembers more details?

mehdi_amini edited edge metadata.Jun 30 2017, 4:40 PM

IIRC the intention was that in-tree work should not be burdened by experimental targets, up to and including breaking them. E.g. an in-tree API migration wouldn't have to update experimental targets. So in that sense LLVM_TARGETS_TO_BUILD=all is working as intended I think.

Yes this is correct: but what matters on this aspect is the *default* isn't it? A bot does not have to specify LLVM_TARGETS_TO_BUILD, and as long as the default does not include the experimental target that seems fine to me.

IIRC the intention was that in-tree work should not be burdened by experimental targets, up to and including breaking them. E.g. an in-tree API migration wouldn't have to update experimental targets. So in that sense LLVM_TARGETS_TO_BUILD=all is working as intended I think.

Yes this is correct: but what matters on this aspect is the *default* isn't it? A bot does not have to specify LLVM_TARGETS_TO_BUILD, and as long as the default does not include the experimental target that seems fine to me.

Yes the patch as is is perfectly fine and we can push it right away.
However it obviously smells like the next thing happening is a commit to zorg putting LLVM_TARGET_TO_BUILD=all on some buildbots. Before this patch it wasn't possible to do that so conveniently as this. So I am raising my complaint early so nobody does that hypethical zorg commit without discussion on llvm-dev!

Another possible naming convention that might avoid the scenario Matthias is worried about is all/everything like clang's -W options.

Another possible naming convention that might avoid the scenario Matthias is worried about is all/everything like clang's -W options.

I actually find -Weverything vs. -Wall rather silly (but understandable from history). For LLVM_TARGETS_TO_BUILD I'd rather just have all.

I prefer "everything", but "stable/all" would be better since they are more descriptive.

rengolin edited edge metadata.Jul 1 2017, 5:09 AM

Changing the default to stable removes any concerns about zorg. I quite like this change as I do agree that all should be all. I also don't like the -W distinction, and this is where this change makes sense.

If we can still use the old way to build experimental targets, this LGTM. If not, as in we build all or nothing, then maybe the proposed change to have "stable;Foo" would be nice to have if not now, very soon.