Avoid burnout: rabbithole phenomenon (a.k.a. recursive bug fixes).

[mcourteaux]

So, a small story:

• Two years ago, I volunteered to implement a fast_atan function, much like we had fast_sin and fast_cos.
• I opened a PR, good feedback came, and I sank a lot of time into it to validate performance on all backends (GPU, and CPU), and measure bit-exact precision.
• This revealed we had issues with strict_float that basically made some of this impossible to do correctly.
• Andrew helps out, and overhauls some of the strict_float handling and intrinsics of Halide, such that I can continue (thanks a lot!).
• Working on this, I found a bug in the CodeGen_Vulkan_Dev regarding vector shuffles.
• Before the PR can be merged, I thought, let's fix the Vulkan shuffle bug: that's an easy 4 line fix.
• Feedback was that we need a test for this, so I add a test.
• The test reveals more issues: the Simplifier rewrites shuffles of shuffles sometimes, which blows up the maximal legal vector lane count on some GPU compute backends (Metal has limit 4, OpenCL limit 8, etc).
• Discussing the best solution to this, I volunteered to make a vector legalization pass.
• I sank a lot of time into it.
• Good feedback comes and requests more changes.
• Here, Andrew steps in and helps out and writes a fuzz tester to verify my work, which is a great idea, and I appreciated the help a lot!
• My work was mostly correct, and the fuzz tester showed a few small problems, which Andrew fixed (thanks again!).
• I merge in the fuzz tester and fixes into the PR, and let the buildbots have a go at it.
• The fuzz tester reveals failures related to at least three unique bugs in the ARM backend of Halide.

Hal fixing a light bulb (from Malcolm in the Middle S03E06 - Health Scare)

I am now like 17 levels deep of fixing this just one more other thing before I can continue with the thing above it. I just wanted to contribute a fast_atan. The fast_atan works already for more than a year, but it is blocked by several levels of issues waiting to be resolved, each one revealing more issues.
The vector legalization pass works, but it is blocked now by random ARM codegen issues. The point is I'm tired of it. On top of that, the legalization pass I wrote only targets GPU backends, because LLVM does vector legalization for us on x86 and ARM.

So I'm writing this down, because I want to open a discussion: is there anything we can do to avoid these seemingly endless rabbit holes? In my opinion, the desired outcome is that we should avoid blocking simple bug fixes or new correctly functioning code because a new accompanying test reveals unrelated bugs. Perhaps there are some CI tricks we can do, to avoid having to remove the test (because the test is good and reveals a bug), but still be able to move forward with good and finished work.

[abadams]

Agree we should talk about this at the dev meeting. What should we do when seemingly-correct new code is red on CI because it has uncovered pre-existing bugs in code already in main?

[abadams]

As a concrete example, pre-existing wasm and sve2 issues mostly in LLVM are stalling #8925

[mcourteaux]

Conclusion from the meeting:

• Uncovering an existing bug that was unrelated to the work of the PR may be handled by filing an issue to GH, AND disabling the test that triggers the crash/error under the specific configuration it does while referencing the issue number in the C++ code. We should use a findable/searchable/greppable tag for this kind of Skip. There is a judgement call to be made on whether the skip is acceptable:
  • Does the PR change behavior that uncovers a bug?
    => this might break code that's already in production. This must be resolved before merge.
  • The PR simply adds a new test that fails under certain conditions, which is not due to changes in behavior of Halide?
    => this is something that can be skipped. When we do this, we open a tracking issue, and the skip in the C++ code of the test mentions the issue number.
• Ideally, we want these "expected failures" to be still tested against, such that if they in the future heal, we still want to know about that, and be able to remove the skip from the test. This will be great to know when LLVM fixes some bug that we are currently deciding to ignore by skipping a test.
• Fuzz tests are great, but (depending on the quality of the fuzz tester) are sometimes a weak signal to the correctness of the PR. They might turn the build red, without being related to this particular PR. As such, we move fuzz tests to their own GH workflow. We should probably run those more frequently, and not only on PRs. Fuzz tests are allowed to fail in a PR if the cause is obviously not the PR.