-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Drop support for LLVM11 #6396
Drop support for LLVM11 #6396
Conversation
With Halide 13 released, we should drop support for LLVM11 in Halide trunk, since we only promise to support LLVM trunk + two releases.
attn @alinas |
@alexreinking -- not sure about the packaging-for-Ubuntu workflow, I just updated to use LLVM12 instead, but not sure if that's kosher |
@@ -1182,20 +1173,9 @@ void CodeGen_LLVM::optimize_module() { | |||
asan_options, use_global_gc, | |||
use_odr_indicator, destructor_kind)); | |||
}); | |||
#elif LLVM_VERSION >= 120 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like you deleted an llvm 12 block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think it's just that the >=120 block was very similar to the else block here (just an extra arg to the lambda passed to registerPipelineStartEPCallback)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm except for two bits that confused me
@@ -1746,13 +1746,8 @@ class SimdOpCheck : public SimdOpCheckTest { | |||
check("v128.const", 8 * w, f32_1 * f32(42)); | |||
check("v128.const", 4 * w, f64_1 * f64(42)); | |||
} else { | |||
if (Halide::Internal::get_llvm_version() == 120) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also looks like an llvm 12 block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is, but we are keeping it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's an == 120 block, not a >= 120 block, so by keeping it aren't we testing the wrong thing on llvm 13?
Or is the original check wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump. I'm confused about why this isn't going to cause simd op check failures on llvm 13+
Or were we only testing wasm on llvm 12 before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old code:
if (Halide::Internal::get_llvm_version() == 120) {
check("i64x2.splat", 8 * w, u16_1 * u16(42));
check("i64x2.splat", 4 * w, u32_1 * u32(42));
} else {
check("i16x8.splat", 8 * w, u16_1 * u16(42));
check("i32x4.splat", 4 * w, u32_1 * u32(42));
}
New code:
check("i64x2.splat", 8 * w, u16_1 * u16(42));
check("i64x2.splat", 4 * w, u32_1 * u32(42));
I think the new code is just the llvm_version() == 120 version, so aren't we changing the expected output for llvm 13/14?
I can't figure out what I'm missing.
@steven-johnson - you need to update Halide/packaging/ubuntu/config.cmake Lines 63 to 67 in 6071cf6
|
We were only testing on llvm14 before, actually, but I still think you are misreading the diff. I'll test this locally on 12/13/14 to verify, though. |
Passes for all three, so I think we are good. |
You're missing the fact that the >= 130 case is handled above -- the section you're looking at is in the else case. |
Ah! Everything is now clear. |
With Halide 13 released, we should drop support for LLVM11 in Halide trunk, since we only promise to support LLVM trunk + two releases.