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

Linker: speedup and debug info preservation #21

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Firestar99
Copy link
Member

Requires #20 to be merged first. Contains a lot of WIP commits.

Some improvements to the linker by @eddyb:

  • mem2reg speedups, which has been known to be exponentially slow
  • preserves debug info when the linker does inlining

This PR contains a lot of WIP commits, but it has worked flawlessly in tests and in my Project.

@schell
Copy link
Contributor

schell commented Sep 29, 2024

@Firestar99 I'm trying this out on renderling...

@schell
Copy link
Contributor

schell commented Sep 29, 2024

I ran into this compilation error with ahash: tkaitchuck/aHash#200

@schell
Copy link
Contributor

schell commented Sep 29, 2024

Running cargo update -p ahash seems to have fixed that error.

@schell
Copy link
Contributor

schell commented Sep 29, 2024

I'm getting a lot of one error (over and over) when building crabslab:

error: cannot cast between pointer types
       from `*f32`
         to `*f32x4`
   --> /Users/schell/.cargo/registry/src/index.crates.io-6f17d22bba15001f/crabslab-0.5.1/src/impl_slab_item/mod.rs:37:25
    |
37  |         let mut array = [T::default(); N];
    |                         ^^^^^^^^^^^^^^^^^
    |

The code is pretty innocuous, and compiled on the previous nightly:

impl<T: SlabItem + Copy + Default, const N: usize> SlabItem for [T; N] {
    const SLAB_SIZE: usize = { <T as SlabItem>::SLAB_SIZE * N };

    fn read_slab(index: usize, slab: &[u32]) -> Self {
        let mut array = [T::default(); N];
        for i in 0..N {
            let j = index + i * T::SLAB_SIZE;
            let t = T::read_slab(j, slab);
            let a: &mut T = crate::array_index_mut(&mut array, i);
            *a = t;
        }
        array
    }

    fn write_slab(&self, mut index: usize, slab: &mut [u32]) -> usize {
        for i in 0..N {
            let n = crate::slice_index(self, i);
            index = n.write_slab(index, slab);
        }
        index
    }
}

@schell
Copy link
Contributor

schell commented Sep 29, 2024

Now I guess the question is - is this related to changes in rust-gpu or is this something in the standard library or something in rustc?

EDIT: obviously not in the standard library as this is no_std 🤦

@schell
Copy link
Contributor

schell commented Sep 29, 2024

Adding a [T; N]: Default constraint to the impl and then changing line 37 to this:

        let mut array: [T; N] = Default::default();

got it compiling. After that everything seems to work as expected in my project.

But I think [T::default(); N] should work and it should probably be a synonym for Default::<[T; N]>::default(), do you agree?

@schell
Copy link
Contributor

schell commented Sep 29, 2024

The great news is that my shaders compile much faster on this branch. 5 seconds vs the previous 40 seconds! An ~87% reduction - that's pretty epic!

@Firestar99
Copy link
Member Author

Firestar99 commented Sep 30, 2024

If [T::default(); N] works the same as vec! does, it would expand to:

let t = ...;
[t.clone(), t.clone(), t.clone(), ..., t.clone()]
drop(t)

But curiously this code from my codebase compiles just fine: EDIT: I don't think any shader actually includes this code

#[inline]
unsafe fn read(from: Self::Transfer) -> Self {
    unsafe {
	let mut ret = [T::default(); N];
	for i in 0..N {
		*ret.index_unchecked_mut(i) = T::read(*from.index_unchecked(i));
	}
	ret
    }
}

Could you check your array_index_mut()? Can it potentially panic, cause that has been my main trouble maker in rust-gpu in the past (don't know about this new branch though). Maybe try spirv_std::arch::IndexUnchecked::index_unchecked?

@schell
Copy link
Contributor

schell commented Sep 30, 2024

If [T::default(); N] works the same as vec! does...

I don't think it would, as vec! is a macro but [T::default(); N] would get parsed as is.

Could you check your array_index_mut()? Can it potentially panic, cause that has been my main trouble maker in rust-gpu in the past (don't know about this new branch though). Maybe try spirv_std::arch::IndexUnchecked::index_unchecked?

array_index_mut is indeed using index_unchecked_mut, see here.

I'm not going to make a fuss, but something changed, and I think we should be able to initialize arrays this way, but it doesn't block me, and I've already released a new crabslab.

@LegNeato
Copy link
Collaborator

LegNeato commented Oct 1, 2024

I think we should at the very least track down what changed and why and then decide the way forward. Good catch!

@Firestar99 Firestar99 marked this pull request as ready for review October 10, 2024 16:40
@LegNeato
Copy link
Collaborator

LegNeato commented Nov 4, 2024

Minimal repro of what I think is the same issue:

use spirv_std::spirv;

#[spirv(fragment)]
pub fn main() {
    let x = [[1; 2]; 1];
}

Edit: If this is the same issue, it repros without this PR and bisects to 02cd324 . Filed #46.

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