Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that PRs against Halide main are compiled vs all supported LLVMs #150

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

steven-johnson
Copy link
Contributor

Halide policy is (still) that our main branch should be usable against $LLVM_RELEASE-1, which currently means LLVM10 thru 13 (inclusive). Recent changes to the Halide buildbots cause Halide PRs to only get tested against 12 and 13.

This changes so that PRs against Halide main get at least one compile-and-test against all LLVMs we support.

(Note that this only affects PRs, not periodic compiles/packages of Halide main branch; the latter are still targeting only LLVM 12 and 13.)

Halide policy is (still) that our main branch should be usable against $LLVM_RELEASE-1, which currently means LLVM10 thru 13 (inclusive). Recent changes to the Halide buildbots cause Halide PRs to only get tested against 12 and 13.

This changes so that PRs against Halide main get at least one compile-and-test against all LLVMs we support.

(Note that this only affects PRs, not periodic compiles/packages of Halide main branch; the latter are still targeting only LLVM 12 and 13.)
@abadams
Copy link
Member

abadams commented Feb 18, 2021

In the past we would test against the two most recent releases plus trunk, which right now would be 10,11,13. LLVM seems to be cutting release branches a long time ahead of the actual release now though, so testing the in-between version (12) is probably also a good idea? I'm not sure it's essential though. Nobody is downloading and using llvm 12 right now.

@steven-johnson
Copy link
Contributor Author

My original impetus for this was halide/Halide#5758, which makes some changes to LLVM-related code that may well need special-casing for LLVM_VERSION... then I realized we weren't testing against 11 or 10 for these.

Re: testing against 12, IMHO it would be a false economy to have zero test coverage against the release-in-progress branch (otherwise we'll just fix breakages later, with more effort).

Re: the policy in general... with our move to separate release branches, it might be worth revisiting the policy to see if we would be better served by saying "one previous release + trunk". I don't know how many folks are relying on Halide trunk + LLVM 10, but my first thought would be that if you are constrained to use an older version of LLVM, you might be well-served by also using the corresponding older version of Halide.

@abadams
Copy link
Member

abadams commented Feb 18, 2021

I think it's one thing to stop testing against a certain version of llvm, and it's another thing to actively delete support for it including an assertion that the linked llvm is newer than that. In practice old llvms still work. Updating LLVM is more painful than updating Halide (in terms of build time), so it's reasonable for people to pull Halide more regularly than LLVM. I do this a lot on my rarely-used-for-development machines (e.g. my gaming box).

Maybe instead of deleting support for it, for RELEASE-1 would could add a warning: "Halide is no longer tested against this version of LLVM. If you encounter bugs, try updating to the latest LLVM release."

The our update PRs would delete support for RELEASE-2 while bumping the version warning for RELEASE-1

@alexreinking
Copy link
Member

Halide PRs to only get tested against 12 and 13.

Well, 11 (-2), 12 (-1), and 13 (0), actually.

@steven-johnson
Copy link
Contributor Author

In practice old llvms still work

Well, yeah, until a PR like the one I pointed out lands -- then things may break silently. Warning that "Halide is no longer tested against this version of LLVM" is just a polite way of saying "we aren't supporting this version of LLVM in this branch"; as the song says, "If you liked it, you should have put some testing on it" (or something like that).

@abadams
Copy link
Member

abadams commented Feb 18, 2021

It's very frustrating to have to recompile LLVM for no reason other than we decided to stop testing against that version of LLVM on the bots, when I know it would still work fine. In general the things that might break break noisily (e.g. adding support for the newer ptx isas).

Our current strategy requires that I build every other version of LLVM to stay on Halide master on my various odd devices, because we support two released versions. I don't want to use unreleased versions because then when I come back to the device next I have no idea what "llvm-trunk" means and have to rebuild from scratch every time. The new strategy would require building every released version of LLVM, and I'd get no warning that my current LLVM is about to stop working - I do a merge with master, and now I can't make any more progress for the day without pulling and building a new LLVM.

@steven-johnson
Copy link
Contributor Author

Er, it seems to me like the strategy I'm proposing would reduce the need for you do pull extra LLVMs, since you could rely on the buildbots to do the build-and-test for the unlikely breakages. Am I missing something?

@abadams
Copy link
Member

abadams commented Feb 18, 2021

I don't build LLVMs to test on different LLVM versions. I build LLVM to have a working LLVM to use for development.

This issue comes up when I have to test something on a platform that I haven't used for >6 months. I could imagine corp users that want to use a release LLVM and Halide master could have the same issue though. Updating the release version of LLVM used in a codebase is potentially much more of a headache than updating the Halide version.

You get the same antipattern in a corp environment, where you have a Halide bug, it's fixed in master, so you go to update Halide to master, and now Halide refuses to accept your LLVM version so you need to go update that too.

@abadams
Copy link
Member

abadams commented Feb 19, 2021

To clarify: My sole argument is that I want to be able to keep using LLVM 10 until LLVM officially releases LLVM 12, whether we have testing for LLVM 10 or not. Obviously testing it would be better than not, so I'm in favor of the PR. I was responding to your comments about the policy in general.

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.

None yet

3 participants