Skip to content

Conversation

@steven-johnson
Copy link
Contributor

@steven-johnson steven-johnson commented May 16, 2021

Followup from recent Halide meeting, this changes build/test defaults to require opt-in for GPU and/or HVX testing, based on labels:

With this in place, we vary testing depending on how a PR is labeled:

  • If 'buildbot_test_nothing' is present, no building/testing is done and other labels mentioned here are ignored. (The already-existing 'skip_buildbots' is a synonym for this.)
  • Otherwise, f 'buildbot_test_everything' is present, we do all possible testing.
  • Otherwise, we default to doing (at least) baseline CPU testing on all targets.
    • if 'buildbot_test_gpu' is present, we also test all GPU targets (cuda, opencl, metal, d3d12, etc)
    • if 'buildbot_test_hvx' is present, we also test Hexagon
    • if 'buildbot_test_wasm' is present, we also test WebAssembly

This has been (lightly) tested on a local buildbot and seems pretty close, but needs more testing on a proper setup.

Avoid redundant testing for Make builds, on the assumption that CMake handles most of them:
- Skip all GPU and HVX testing
- Skip the error, warning, performance, and tutorials (basically, just correctness and generator)
- Continue to skip python & apps where we used to
- Continue to skip wasm for Make entirely
Followup from recent Halide meeting, this changes build/test defaults to require opt-in for GPU and/or HVX testing, based on labels:

We vary testing depending on how a PR is labeled:
- if 'buildbot_test_nothing' is present, no testing is done and the rest are ignored. ('skip_buildbots' is a synonym for this.)
- if 'buildbot_test_everything' is present, we do all possible testing.
- Otherwise, we default to doing (at least) baseline CPU testing on all targets.
  - if 'buildbot_test_gpu' is present, we also test all GPU targets (cuda, opencl, metal, d3d12, etc)
  - if 'buildbot_test_hvx' is present, we also test Hexagon

This has been (lightly) tested on a local buildbot and seems pretty close, but needs more testing on a proper setup.
Base automatically changed from srj-lessmake to master May 16, 2021 21:57
@alexreinking
Copy link
Member

It would be nice if these labels corresponded to something that already exists: eg. the actual target/feature names, the app names, or the CTest label names. It's fine to have meta-labels like gpu, but a PR that only patches a bug in the D3D12 backend might want to test only on D3D12.

We also still need to do the work of labeling the GPU-enabled correctness tests and the like, so we don't re-run the exact same test with irrelevant target flags.

I'm also not sold on the naming convention. Having buildbot_ in there seems a little redundant and using all underscores makes the namespace feel a bit flat. Since this PR selects which targets to test, maybe we should use target:none for skip and target:hvx for enabling HVX testing and so on. Later, we might add switches for CTest labels like ctest:no_performance to skip performance testing, etc. This is kind of a nit, but it crossed my mind, so I thought I'd say it.

@alexreinking
Copy link
Member

It would also be good to address #92 while we're making these changes (or decide that #181 met those needs)

@alexreinking
Copy link
Member

How bad would it be to dust this off and land it?

@mcourteaux
Copy link

I'm inclined to argue it's dangerous to disable parts of the test suite. It has happened many times that changing something that feels unrelated at first glance breaks some codegen on another platform.

What I think might be useful is to have a buildbot_build_only, if it's to make sure the compiler likes it, but we know it won't affect codegen or testing.

Perhaps another useful application of the same mechanism would be to disable LLVM-main-build or LLVM-main-build-and-test, if we already know LLVM-main is broken. This will save time on the buildbot machines for useful work. Suggestion for the tag: buildbot_skip_llvm_main_build_and_test and buildbot_skip_llvm_main_test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants