update to 1.4 - #52
SimonDanisch wants to merge 15 commits into
Conversation
|
I remember attempting to update to even just a more recent 1.3 patch version about a year ago, and was faced with nontrivial issues in the generated code that required adjustments on the generator side (and possibly on VulkanSpec.jl to adapt to a changing representation of the contents of the specification). Support for 1.4 might require quite some thought to get it to a working state. I have essentially moved on from Julia/Vulkan work but I'm happy to review changes in the generator parts (I will trust that you check tests pass in Vulkan.jl and most importantly in your downstream packages before we merge). If updating the generator proves to be too complex/requires to be too intimately familiar with how it works internally then feel free to let me know and I'll see if I can find some time to do it. |
|
Oh wait, I thought this was a PR to Vulkan.jl, not to VulkanCore.jl, my mistake. My previous comment applies to the former. Seems like CI has issues fetching the SDK, would be nice to get that working for at least Linux CI. |
| [[deps.Clang]] | ||
| deps = ["CEnum", "Clang_jll", "Downloads", "Pkg", "TOML"] | ||
| git-tree-sha1 = "d78c2973d7a752be377fe173bc9ff2dc2d9c3ed6" | ||
| deps = ["CEnum", "Clang_unified_jll", "Downloads", "Pkg", "TOML"] | ||
| git-tree-sha1 = "abf4d804064811b1948978a4aee478ccc76d6c9d" | ||
| uuid = "40e3b903-d033-50b4-a0cc-940c62c95e31" | ||
| version = "0.17.6" | ||
| version = "0.19.3" |
There was a problem hiding this comment.
Updating to Clang 0.19.3 seems to have a fairly different representation for structs (they are generated with a single .data field that is an NTuple with getproperty/propertynames overloaded to represent the different "fields"), as in
struct VkRenderingAttachmentInfo
sType::VkStructureType
pNext::Ptr{Cvoid}
imageView::VkImageView
imageLayout::VkImageLayout
resolveMode::VkResolveModeFlagBits
resolveImageView::VkImageView
resolveImageLayout::VkImageLayout
loadOp::VkAttachmentLoadOp
storeOp::VkAttachmentStoreOp
clearValue::VkClearValue
endpreviously and now this:
struct VkRenderingAttachmentInfo
data::NTuple{72, UInt8}
end
function Base.getproperty(x::Ptr{VkRenderingAttachmentInfo}, f::Symbol)
f === :sType && return Ptr{VkStructureType}(x + 0)
f === :pNext && return Ptr{Ptr{Cvoid}}(x + 8)
f === :imageView && return Ptr{VkImageView}(x + 16)
f === :imageLayout && return Ptr{VkImageLayout}(x + 24)
f === :resolveMode && return Ptr{VkResolveModeFlagBits}(x + 28)
f === :resolveImageView && return Ptr{VkImageView}(x + 32)
f === :resolveImageLayout && return Ptr{VkImageLayout}(x + 40)
f === :loadOp && return Ptr{VkAttachmentLoadOp}(x + 44)
f === :storeOp && return Ptr{VkAttachmentStoreOp}(x + 48)
f === :clearValue && return Ptr{VkClearValue}(x + 52)
return getfield(x, f)
end
function Base.getproperty(x::VkRenderingAttachmentInfo, f::Symbol)
r = Ref{VkRenderingAttachmentInfo}(x)
ptr = Base.unsafe_convert(Ptr{VkRenderingAttachmentInfo}, r)
fptr = getproperty(ptr, f)
GC.@preserve r unsafe_load(fptr)
end
function Base.setproperty!(x::Ptr{VkRenderingAttachmentInfo}, f::Symbol, v)
unsafe_store!(getproperty(x, f), v)
end
function Base.propertynames(x::VkRenderingAttachmentInfo, private::Bool = false)
(:sType, :pNext, :imageView, :imageLayout, :resolveMode, :resolveImageView, :resolveImageLayout, :loadOp, :storeOp, :clearValue, if private
fieldnames(typeof(x))
else
()
end...)
end
function VkRenderingAttachmentInfo(sType::VkStructureType, pNext::Ptr{Cvoid}, imageView::VkImageView, imageLayout::VkImageLayout, resolveMode::VkResolveModeFlagBits, resolveImageView::VkImageView, resolveImageLayout::VkImageLayout, loadOp::VkAttachmentLoadOp, storeOp::VkAttachmentStoreOp, clearValue::VkClearValue)
ref = Ref{VkRenderingAttachmentInfo}()
ptr = Base.unsafe_convert(Ptr{VkRenderingAttachmentInfo}, ref)
ptr.sType = sType
ptr.pNext = pNext
ptr.imageView = imageView
ptr.imageLayout = imageLayout
ptr.resolveMode = resolveMode
ptr.resolveImageView = resolveImageView
ptr.resolveImageLayout = resolveImageLayout
ptr.loadOp = loadOp
ptr.storeOp = storeOp
ptr.clearValue = clearValue
ref[]
endIt feels like it might be working fine but at the same time it could easily break code that used to introspect into e.g. fieldnames instead of the user-facing propertynames.
It might be just fine but I'd prefer to have confirmation downstream, outside the VulkanCore.jl test suite, that the changes from updating to latest Clang don't behave unexpectedly.
Otherwise it might be preferable to keep this PR limited to updating to 1.4 bindings and keep the same version for Clang.jl as we used before. We can always do a PR later that bumps Clang.jl to the latest version if needed.
There was a problem hiding this comment.
I remembered that there were alignment bugs in 0.17.x.
I summoned Claude to review this now.
A package that cannot work on this machine should not cost anything to depend
on. VulkanCore compiled 34,000 generated lines of ccall wrappers, Vulkan
127,000 more plus a dispatch table and an 8,573-name export list, and Lava a
whole Julia->SPIR-V compiler — all on every Mac in this tree, for code Mantle
declares and, under its own `@static if Sys.isapple()`, never imports.
VulkanCore probes the loader once at precompile time and exposes the answer as
HAS_LOADER; Vulkan and Lava gate their bodies on it, and Vulkan re-exposes it
outside its own gate so a dependent can ask without first checking whether
there is anything to ask. The extensions are gated too — Vk.Format does not
exist when the bindings do not, and those were the last thing still failing.
The gated text is UNCHANGED and unindented on purpose: with a loader present
each module is byte-for-byte the one it was, so the only case this can break is
the empty one.
Measured on macOS with no libvulkan: VulkanCore 3670 -> 554 ms, Vulkan 13098 ->
363 ms, and Lava now precompiles at all — gated out, it no longer evaluates the
`NativeEmitter{Out, Flats}` its body wants and this KernelAbstractions checkout
does not have. All three load in 0.4 s. `using Metal, RayMakie` takes 3.1 s
from a cold RayMakie cache, where it used to error.
A precompile-time answer, so installing a driver later needs
Pkg.precompile(; force = true).
NOT verified with a loader present — there is none on this machine.
The version tracks the Vulkan API version, and the bindings have been generated against Vulkan_Headers_jll 1.4.321 since "update to 1.4".
"update to 1.4" regenerated thirteen platform files and added a fourteenth, lib/x86_64-unknown-freebsd13.2.jl -- the target Clang names now -- but left lib/x86_64-unknown-freebsd.jl untouched at 1.3.240, and that is the one this file still included. FreeBSD therefore loaded 1.3 bindings under a 1.4 wrapper: of the 3000 Vk names Vulkan.jl's bsd.jl mentions, around a hundred did not exist. Now 2, both Wayland, and those are a separate and older problem: the wrapper generator enables PLATFORM_WAYLAND for BSD while no FreeBSD binding here has ever wrapped Wayland, in this version or the previous one. The 1.3 file is deleted rather than left beside its replacement.
`JULIA_VULKAN_LIBNAME` is read here, while the package precompiles, and an `ENV` read is not part of the precompile hash. So setting it takes effect only if something else happens to invalidate the cache. Observed directly on a driverless Mac: set the variable, restart Julia, and the cache built WITHOUT it was reused -- `HAS_LOADER` stayed false on a machine that by then had a perfectly good driver available. Only editing a source file forced the rebuild. A preference is hashed, so changing it rebuilds the bindings against the new library on the next load. `Vulkan.set_driver` writes it. The environment variable still works and still comes before the system default, but it is the fallback now rather than the mechanism.
The SDK download was pinned to LunarG 1.3.239.0, which LunarG no longer serves: every job failed on a 404 before Julia was installed. Mesa's lavapipe comes from apt instead, through the same action Vulkan.jl uses, so a runner with no GPU still has something to create an instance against. That alone leaves CI red. `test/glfw.jl` asserted `@test_broken result == VK_SUCCESS` whenever `JULIA_GITHUB_ACTIONS_CI` was ON, because the runner had no driver -- with one present that is an Unexpected Pass, which is an error. The whole env-var gate is gone: the tests ask `HAS_LOADER`, the same question the package itself gates on, so CI and a developer machine now run the same suite, `old_tests.jl` included. `using GLFW` moves under that gate too, so the no-driver job needs no window toolkit. Verified with lavapipe as the only ICD, which is what the runner will have: 10 of 10. Same on this machine's real drivers. Also: macOS keeps a job with no driver on purpose, since that is where the `HAS_LOADER` case is actually met; the stale commented-out Windows job goes; and `julia-uploadcodecov` is archived, replaced by `julia-processcoverage` plus `codecov-action`.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #52 +/- ##
===========================================
+ Coverage 33.87% 61.02% +27.15%
===========================================
Files 3 5 +2
Lines 62 136 +74
===========================================
+ Hits 21 83 +62
- Misses 41 53 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Skipping the API is right on a machine that has no Vulkan loader, and a silent pass on a machine that is supposed to have one: if the lavapipe install ever breaks, the Linux job would quietly drop from 10 tests to 2 and stay green. The job that installs the driver now says so, and a missing loader there is a failure rather than a skip. This makes CI assert MORE than a dev machine, never less -- the opposite of the gate it replaced. Verified both ways: 11 of 11 with the requirement set and lavapipe as the only ICD, and the guard fails when the loader is missing.
The env var of the previous commit was patching around the wrong thing. The suite does not need to know whether it may skip: a machine with no Vulkan loader is a supported state, but not one this suite can conclude anything from, so it simply requires one. No gate, no skip, no second job. macOS gets MoltenVK, the loader and the validation layers from Homebrew, which is how Vulkan is reached there anyway and what the old LunarG SDK job was aiming at. The loader is named through a preference rather than JULIA_VULKAN_LIBNAME, since only the preference is part of the precompile hash and `julia-actions/cache` restores package caches; Pkg.test copies it into the test sandbox. Linux verified locally with lavapipe as the only ICD: 11 of 11. macOS is unverified from here -- no Mac.
The macOS runner found both of these. MoltenVK is a portability driver, and since loader 1.3.216 an instance that does not ask for portability enumeration is not allowed to see one: `vkCreateInstance` answers VK_ERROR_INCOMPATIBLE_DRIVER. `with_portability` asks the loader whether it offers the extension rather than asking what platform this is, so there is one code path everywhere -- the Linux loader advertises it too, and the flag is set there as well. Second, `old_tests.jl` then carried on with the handle vkCreateInstance had never written and segfaulted inside the loader, because `@test` inside a testset records a failure and continues. It stops there now. Linux with lavapipe as the only ICD: 11 of 11. Forcing the failure with a bogus driver file raises rather than crashing.
The previous commit's edit took the LocalPreferences.toml step out along with the comment block above it, so VulkanCore precompiled without being told which loader to bind to. It surfaced as `UndefVarError: VkExtensionProperties` from vkhelper.jl, which is a bad way to say "no loader": the helpers name types the gate did not compile. The suite says it outright now, before including anything. Linux with lavapipe: 11 of 11.
|
I spend some time to get macOS CI running, which found two bugs: MoltenVK needs portability enumeration, and |
|
Did you read and consider this comment? I still don't see why we would want to change the current generated code. I am not sure about its implications and whether it is even desirable. I believe the "opaque struct everywhere" generation mode can be turned off in the |
|
Also, given the number of changes, before merging/releasing it would be preferable to test that in a downstream package that relies on VulkanCore, such as https://github.com/SimonDanisch/Lava.jl. Did you try it with JuliaGPU/Vulkan.jl#62? |
Yes this has been developed as part of testing Vulkan and Lava on ~5 platforms (windows x linux x apple x nvidia x amd), with pretty heavy Vulkan applications (raytracing, a new makie vulkan backend, and lots of compute). |
C unions were already handled as binary blobs with the right
It's good to have this confirmation. I would have also tested myself with my own experimental Vulkan applications but they're not in a functional state at the moment (some bugs popping out apparently due to various changes in packages and/or in system Vulkan drivers) and I'm not planning to resume work on them in the foreseeable future. Before merging I'd ideally prefer to have another perspective on this given that it's a bunch of changes. I remember that @Gnimuc used VulkanCore.jl without going through Vulkan.jl (AFAIK), if he'd be keen to share his thoughts/review and make sure that doesn't break his code. Other reviewer suggestions are welcome too of course, as I'm not sure who is actively depending on this/maintaining this at the moment (I'm just around to look at the code and provide feedback but no longer use it actively). |
Review of the current head ( High1. The blob structs' constructors are strictly typed, which breaks the released Vulkan.jl ( This adds to the open thread on VkClearAttachment(VK_IMAGE_ASPECT_COLOR_BIT, 0, clear_value)
# master: works
# this PR: MethodError: no method matching VkClearAttachment(::VkImageAspectFlagBits, ::Int64, ::VkClearValue)The registered Vulkan.jl 0.6.28–0.6.30 have VulkanCore compat The layout argument holds for some structs: on master, Possible fixes:
2. A cache built without a loader stays empty after one is installed (
On master the same sequence fixes itself:
A typical way to hit this: run The non-fatal Medium3. This is the documented recovery for item 2, but Pkg has no Base.compilecache(Base.identify_package("VulkanCore"))4.
This test used to pass
Fix: keep the 5. In item 2's stale state, Low6. An empty Master ignored a set-but-empty variable. Here 7. The released In Vulkan.jl 0.6.30, 8. Output file names still follow Clang's versioned triples (
Two ways to catch it:
9. Nit: a test that can't fail (
How this was checked:
Not checked: JuliaGPU/Vulkan.jl#62, or any platform other than Linux x86_64. CI is green on the head commit. Generated by Claude Code |
3 participants
Vulkan 1.4 has lots of nice features e.g. to use hardware accelerated matmul.