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

Pass constants into #[spirv(compute(threads(...)))] ? #697

Closed
hrydgard opened this issue Jul 29, 2021 · 7 comments
Closed

Pass constants into #[spirv(compute(threads(...)))] ? #697

hrydgard opened this issue Jul 29, 2021 · 7 comments
Labels
t: enhancement A new feature or improvement to an existing one. t: ergonomics Language ergonomics discussions and experimentation

Comments

@hrydgard
Copy link
Contributor

hrydgard commented Jul 29, 2021

Quite often, an algorithm will have a working data size configurable by a constant, and this size is correlated with the thread group size. Here's what I would like to do, but doesn't work:

const BUF_SIZE: u32 = 64;

// use BUF_SIZE to declare arrays and what not

#[spirv(compute(threads(BUF_SIZE, 1, 1)))]
pub fn main_cs(...) { ... }

Instead I have to fall back to:

#[spirv(compute(threads(64, 1, 1)))]  // 64 == BUF_SIZE

I guess the way attributes work might make this impossible? Or is there a way?

In GLSL/HLSL, this is trivial but that's because there's a preprocessor which just injects a #define-d value through text replacement.

@hrydgard hrydgard added the t: bug Something isn't working label Jul 29, 2021
@DJMcNab
Copy link
Contributor

DJMcNab commented Jul 29, 2021

Seems like you might be able to use rust-lang/rust#83366

@hrydgard
Copy link
Contributor Author

yeah! Would have to define a tiny macro though instead of using a constant :)

@hrydgard hrydgard added t: enhancement A new feature or improvement to an existing one. t: ergonomics Language ergonomics discussions and experimentation and removed t: bug Something isn't working labels Jul 29, 2021
@DJMcNab
Copy link
Contributor

DJMcNab commented Jul 31, 2021

Perhaps unsurprisingly, that doesn't actually work, unfortunately.

I've been investigating doing this properly, and it has me stumped.

Basically, I can't find any way to go from TyCtxt and a name (in some form) to a constant item (having spent a couple of hours poking at rust-analyzer's suggestions and reading the source code). I say it might be trivial because such a method might exist (I may as well ask @bjorn3, who I think is the person most likely to know). I can't think of any standard attributes which need to do name resolution (I doubt that you can define a niche based on a constant's value)

But if there is no way to do this, I have to take it as intentional - during the couse of my search this zulip message:

I had thought there were (well-established?) issues with putting variables/expressions into attributes, in terms of how that interacts with hygiene and name-resolution.

Although this is in a very specific context, so I might be over extrapolating.

If this is truly impossible, then one way we could do this is to abuse const generics, i.e. basically:

const N_THREADS: u64 = 100;
#[spirv(compute)]
fn my_entry_point(threads: NumberOfThreads<{N_THREADS}, 0, 0>){}

@bjorn3
Copy link
Contributor

bjorn3 commented Jul 31, 2021

Basically, I can't find any way to go from TyCtxt and a name (in some form) to a constant item (having spent a couple of hours poking at rust-analyzer's suggestions and reading the source code).

The Resolver no longer exists once the HIR is built. Rustdoc uses a hack to keep it alive for intra doc links, but this has caused ICEs and is impossible for hotpluggable codegen backends, only custom drivers can do this.

Clippy uses this function: https://github.com/rust-lang/rust-clippy/blob/43905d9f8d0f27d7ad71008bec9ef3c0b0982782/clippy_utils/src/lib.rs#L476 It doesn't handle for example hygiene though and it requires an absolute path.

@charles-r-earp
Copy link
Contributor

Is there any reason the entry can't have const generic parameters?

#[spirv(compute)]
fn my_entry<N_THREADS>( .. ) { .. }

@bjorn3
Copy link
Contributor

bjorn3 commented Aug 1, 2021

That won't resolve to the const. It will introduce a completely new binding.

@oisyn
Copy link
Contributor

oisyn commented Nov 17, 2022

Closing this as a won't fix. Our hands are tied wrt Rust support, and we're not considering another design atm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: enhancement A new feature or improvement to an existing one. t: ergonomics Language ergonomics discussions and experimentation
Projects
None yet
Development

No branches or pull requests

5 participants