Skip to content

Conversation

@christiangnrd
Copy link
Member

I wasn't comfortable force-pushing in #385 so I opened this one.

@codecov

This comment was marked as off-topic.

@simeonschaub
Copy link
Member

Unfortunately, this is currently blocked by #384 and potentially other issues. I thought I could work around it using the khronos backend, but looks like that is broken in other ways

Comment on lines 63 to 68
let BitInteger64 = Union{Int64, UInt64}
@device_override function Base.checkbounds(::Type{Bool}, v::StepRange{<:BitInteger64, <:BitInteger64}, i::BitInteger64)
@inline
return checkindex(Bool, eachindex(IndexLinear(), v), i)
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let BitInteger64 = Union{Int64, UInt64}
@device_override function Base.checkbounds(::Type{Bool}, v::StepRange{<:BitInteger64, <:BitInteger64}, i::BitInteger64)
@inline
return checkindex(Bool, eachindex(IndexLinear(), v), i)
end
end
BitInteger64 = Union{Int64, UInt64}
@device_override function Base.checkbounds(::Type{Bool}, v::StepRange{<:BitInteger64, <:BitInteger64}, i::BitInteger64)
@inline
return checkindex(Bool, eachindex(IndexLinear(), v), i)
end

I don't think we can use let here and @device_override global function is not recognized by the macro. With this change and JuliaPackaging/Yggdrasil#12341 all tests are passing for me locally on 1.12

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll make the change and revert my temporary test-reducing measures shortly.

Should we also limit POCL to 7.0 for the main package instead of just tests until someone figures out what’s causing the new test failures?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to restrict compatibility for the entire package. The issue is incomplete Float16 support, which we didnt test previously because the jlls had Float16 support disabled

christiangnrd and others added 2 commits October 20, 2025 09:50
Co-Authored-By: Simeon David Schaub <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to disable Float16 tests for the local pocl builds though, those should actually pass

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are still failing which is what I was testing

Copy link
Member

@simeonschaub simeonschaub Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's only on 1.10 and 1.11 though, not 1.12

Copy link
Member

@simeonschaub simeonschaub Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but the 1.12 failures are not due to Float16, they're #384 as I wrote earlier. Once the SPIRV-LLVM-Backend jll has been registered we can retrigger CI

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I understand now thank you for bearing with me. I'll push a change to skip float16 only on pocl_jll and to revert to using spirv_llvm_translator as soon as it's registered

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant SPIRV-LLVM-Backend, not translator, so that part is already correct. I was just experimenting with the khronos translator, but it's still pretty broken atm

@christiangnrd
Copy link
Member Author

@simeonschaub I just noticed that the local builds don't report Float16 support.

From the versioninfo():

 │  - Portable Computing Language
│    OpenCL 3.0, PoCL 7.2-pre main-0-gea1824d  Linux, Debug+Asserts, RELOC, SPIR-V, LLVM 19.1.7jl, SLEEF, DISTRO, POCL_DEBUG
└    · cpu-neoverse-n2-0xd49 (svm:c+f, usm:h+d, bda, fp64, il)

@simeonschaub
Copy link
Member

Ah, that's arm though, right? I believe only the x64 builds have Float16 support

@christiangnrd
Copy link
Member Author

Should we revert JuliaGPU/pocl@afa0682 and release pocl_jll v7.1.0+1?

@simeonschaub
Copy link
Member

simeonschaub commented Oct 21, 2025

That commit is harmless, the one we might want to revert is JuliaGPU/pocl@893558a. But yes, that might be the best way forward

Edit: opened JuliaPackaging/Yggdrasil#12355

@christiangnrd
Copy link
Member Author

Oops! I meant to link to the one you mentioned.

@christiangnrd
Copy link
Member Author

Okay I think that when JuliaPackaging/Yggdrasil#12355 is registered and my suggestion reverting the check is applied, this will be ready to merge.

the only question left is do we keep testing both 1.11 and 1.12 or only 1.12? Like in the original 1.12 pr?

@simeonschaub
Copy link
Member

the only question left is do we keep testing both 1.11 and 1.12 or only 1.12? Like in the original 1.12 pr?

We already have way too many CI jobs in my opinion. I think we can just drop 1.11 since it's not an officially supported release anymore

Co-authored-by: Christian Guinard <[email protected]>
@christiangnrd
Copy link
Member Author

I think the 1.12 macos runners just need more time.

@christiangnrd
Copy link
Member Author

christiangnrd commented Oct 21, 2025

There seems to be a huge performance regression with ARM macOS on 1.12.
ARM macOS has some performance issues but unrelated to 1.12

Copy link
Member

@simeonschaub simeonschaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can go ahead with this

@simeonschaub simeonschaub requested a review from maleadt October 22, 2025 13:48
@maleadt maleadt merged commit b58747a into JuliaGPU:master Oct 23, 2025
35 of 45 checks passed
@maleadt maleadt mentioned this pull request Oct 23, 2025
@christiangnrd christiangnrd deleted the 1.12 branch October 23, 2025 10:00
simeonschaub added a commit that referenced this pull request Oct 23, 2025
Co-authored-by: Simeon David Schaub <[email protected]>
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.

3 participants