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

Require LLVM >= 16.0 #8003

Merged
merged 5 commits into from
Jan 17, 2024
Merged

Require LLVM >= 16.0 #8003

merged 5 commits into from
Jan 17, 2024

Conversation

steven-johnson
Copy link
Contributor

Per policy, we only support top-of-tree LLVM, plus two versions back; let's update to require LLVM >= 16, and drop workarounds for older versions.

Per policy, we only support top-of-tree LLVM, plus two versions back; let's update to require LLVM >= 16, and drop workarounds for older versions.
@zvookin
Copy link
Member

zvookin commented Dec 12, 2023

Why are you changing #if LLVM_VERSION < 170 to #if !(LLVM_VERSION >= 170)? This is difficult to read. If there is a strong reason for making the change, maybe add a comment. In general there likely ought to be a comment on every LLVM version conditional.

@abadams
Copy link
Member

abadams commented Dec 12, 2023

I think the reason we were delaying this is that vcpkg was stuck on llvm 15, and our install instructions say to use vcpkg's llvm. We should check that vcpkg has moved on before landing this.

@steven-johnson
Copy link
Contributor Author

Why are you changing #if LLVM_VERSION < 170 to #if !(LLVM_VERSION >= 170)? This is difficult to read. If there is a strong reason for making the change, maybe add a comment. In general there likely ought to be a comment on every LLVM version conditional.

IMHO it was easier to normalize everything to a form of >= but I don't feel strongly about this. I'll revert those.

@steven-johnson
Copy link
Contributor Author

I think the reason we were delaying this is that vcpkg was stuck on llvm 15, and our install instructions say to use vcpkg's llvm. We should check that vcpkg has moved on before landing this.

Good call, will do. Also, this should probably wait to land until after I create a fork for the Halide 17 release, since it ostensibly should still support LLVM 15..17.

@steven-johnson
Copy link
Contributor Author

I think the reason we were delaying this is that vcpkg was stuck on llvm 15, and our install instructions say to use vcpkg's llvm. We should check that vcpkg has moved on before landing this.

Good call, will do. Also, this should probably wait to land until after I create a fork for the Halide 17 release, since it ostensibly should still support LLVM 15..17.

I have verified that vcpkg for Windows now provides LLVM17 as default.

@steven-johnson
Copy link
Contributor Author

OK to land (finally)?

@abadams
Copy link
Member

abadams commented Jan 16, 2024

Fine by me

@steven-johnson steven-johnson merged commit 3a77204 into main Jan 17, 2024
19 checks passed
@steven-johnson steven-johnson deleted the srj/llvm-ver branch January 17, 2024 15:35
ardier pushed a commit to ardier/Halide-mutation that referenced this pull request Mar 3, 2024
* Require LLVM >= 16.0

Per policy, we only support top-of-tree LLVM, plus two versions back; let's update to require LLVM >= 16, and drop workarounds for older versions.

* LLVM_VERSION < 170
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.

4 participants