Skip to content

Lack of unrolling and very high register usage #462

Open

Description

@nazar-pc

I have many places in the code that can be condensed to something like this:

const MAX_BUCKET_SIZE: usize = 512;
const WORKGROUP_SIZE: u32 = 256;

unsafe {
    core::hint::assert_unchecked(matches_count <= MAX_BUCKET_SIZE);
}
for index in (local_invocation_id..matches_count as u32).step_by(WORKGROUP_SIZE as usize) {

Can also be rewritten in a way that will probably produce some helpful runtime code in SPIR-V since I suspect the hint will be lost in translation:

const MAX_BUCKET_SIZE: usize = 512;
const WORKGROUP_SIZE: u32 = 256;

for index in (local_invocation_id..matches_count.min(MAX_BUCKET_SIZE) as u32)
    .step_by(WORKGROUP_SIZE as usize)
{

It is not difficult for me to see that there will be at most two loop iterations here per invocation. However, it is not something compiler sees today.

The result is much higher register usage, impacting occupancy in a big way.

Rewriting it to inner function that is called twice with explicit bounds checks fixes register usage (though I hit #461 when doing so), but is far from idiomatic Rust and is quite painful to do manually in all such cases.

I wish end-to-end compilation chain was aware of things like this, it is a very important pattern for performance.

In fact loop unrolling is extremely bad right now, even fixed loops with 3-4 iterations and one or several simply ALU instructions in it are not unrolled and balloon register usage.

Activity

  1. Firestar99 commented on Nov 26, 2025

    @Firestar99
    Member

    assert_unchecked calls crate::intrinsics::assume, so looking into our assume codegen, it's just getting discarded:

    // FIXME(eddyb) `assume` is not implemented atm, so all of its forms should
    // avoid computing its (potentially illegal) bool input in the first place.
    fn assume(&mut self, _val: Self::Value) {}
    fn expect(&mut self, cond: Self::Value, _expected: bool) -> Self::Value {
    // TODO: llvm.expect
    cond
    }

    // FIXME(eddyb) `assume` is not implemented atm, so all of its forms should
    // avoid computing its (potentially illegal) bool input in the first place.
    fn assume_integer_range(&mut self, _imm: Self::Value, _ty: Self::Type, _range: WrappingRange) {}
    fn assume_nonnull(&mut self, _val: Self::Value) {}

    However, what isn't getting ignored is unreachable:

    fn unreachable(&mut self) {
    self.emit().unreachable().unwrap();
    }

    Could you try something like this?

    if matches_count <= MAX_BUCKET_SIZE {
        unsafe {
            core::hint::unreachable();
        }
    }

    If this works, I'm willing to just wire assume to be that

  2. Firestar99 commented on Nov 26, 2025

    @Firestar99
    Member

    There's an extension called SPV_KHR_expect_assume that adds OpAssumeTrueKHR and OpExpectKHR. The Vulkan part VK_KHR_shader_expect_assume is promoted to be required in Vulkan1.4. So we could even emit those, and have a fallback to unreachable.

    Problem: rspirv on crates.io doesn't provide those symbols yet... gfx-rs/rspirv#259

  3. nazar-pc commented on Nov 26, 2025

    @nazar-pc
    ContributorAuthor

    Unreachable should have the same effect as the second loop example I have provided. Handling of loops feels completely broken right now in terms of optimizations.

  4. self-assigned this
    on Nov 26, 2025
  5. 39ali commented on Apr 20, 2026

    @39ali
    Contributor

    i've hit a similar issue, i'll fix this in spirt and if the pr passes i'll update rust-gpu as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions