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

Enums MVP. #819

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

Enums MVP. #819

wants to merge 2 commits into from

Conversation

ElectronicRU
Copy link
Contributor

@ElectronicRU ElectronicRU commented Dec 7, 2021

Supported cases:

  • non-repr-C enums without non-null optimization;
  • repr-C enums that are dataless / have the same layout for every case
  • enums with non-null optimization that don't use pointers (e.g. Option).

Future work directions:

  • Option<&_> and such - currently emits zombies/invalid SPIR-V, likely requires adaptations in mem2reg
  • repr(C) enums that can be represented as (discriminant, union) - some of the groundwork is already there

Supported cases:
- non-repr-C enums without non-null optimization;
- repr-C enums that are dataless / have the same layout for every case
- enums with non-null optimization that don't use pointers (e.g. Option<NonZeroUsize>).

Future work directions:
- Option<&_> and such - currently emits invalid SPIR-V, likely requires adaptations in mem2reg
- repr(C) enums that can be represented as (discriminant, union) - some of the groundwork is already there
@repi repi removed the request for review from khyperia January 30, 2022 13:08
@eddyb eddyb added the s: waiting on review PRs that blocked on a team member's review. label May 21, 2022
@joeftiger
Copy link

joeftiger commented Aug 25, 2022

Would this mean that one can use things like Option<T> or algebraic enums, e.g.?
As long as T is not a pointer?

I didn't get to play around with rust-gpu that much yet, my understanding was that so far only data-less enums work (#78).

@ElectronicRU
Copy link
Contributor Author

@joeftiger yes. With pointers, it can generate incorrect SPIR-V, because operations on pointers are heavily restricted and amending that would require basically a full-program analysis that I haven't come up with.

@Keavon
Copy link

Keavon commented Nov 12, 2022

This has been open for nearly a year. Is it still ready and just waiting for a review?

@djdisodo
Copy link

i've roughly merged this into latest version here

i compiled this code

struct Nah {
    a: u32
}

enum Foo {
    A(Nah),
    B,
}

#[spirv(compute(threads(64)))]
pub fn main(
    #[spirv(global_invocation_id)] id: UVec3,
    #[spirv(storage_buffer, descriptor_set = 0, binding = 1)] output: &mut [u32],
) {
    let aa = Foo::A(Nah { a: 0xffffffff });

    if let Foo::A(a) = aa {
        output[id.x as usize] = a.a;
    } else {
        output[id.x as usize] = 99;
    }
}

it fails to compile

however if i revert providers.layout_of changes it works

need further research

@ElectronicRU
Copy link
Contributor Author

@djdisodo do you mean the deoverlay_enum_fields change etc?
@Keavon there were some unresolved issues with this code, and ultimately, not much desire from Embark to do this. No-one wanted to do a massive code review, I guess.

@eddyb there seems to still be interest in this feature - I can probably bring it up to date, write some more tests, etc. Unfortunately I never did summon the courage required to make it work with pointers.

@Keavon
Copy link

Keavon commented Feb 2, 2023

@ElectronicRU that would be awesome if you could return to this and get it landed now that the project seems to be picking back up on Embark's end. Our project https://github.com/GraphiteEditor/Graphite needs it for our node graph compositing shaders, which are written in Rust, to work with (some degree of) generic types in our node graph system.

@brynnbrancamp
Copy link

This is fundamental Rust, we need some form of this feature; if not, this.

@eddyb eddyb added the s: qptr may fix This might be fixed by the "qptr" experiment (https://github.com/EmbarkStudios/spirt/pull/24) label Mar 29, 2023
@Lucky4Luuk
Copy link

What is the status on this feature? I'm running into issues with Option that I feel are related to this, specifically with Option where T is a struct.

@ElectronicRU
Copy link
Contributor Author

@Lucky4Luuk sorry - life & work take up a lot of time, I don't have any to work on this and won't for the foreseeable future :( Someone else would need to push it through for now, sorry.

@reinismu
Copy link

Noticed rust-gpu project and got so excited today, but after few hours encountered unsupported Option<T>. Gave it a try to merge this PR in main, but a lot of things have changed and I don't have the know how :/

Would really appropriate if someone picked this up again 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: qptr may fix This might be fixed by the "qptr" experiment (https://github.com/EmbarkStudios/spirt/pull/24) s: waiting on review PRs that blocked on a team member's review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants