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

gen: extended headers #270

Merged
merged 10 commits into from
Mar 3, 2024
Merged

Conversation

rajveermalviya
Copy link
Collaborator

@rajveermalviya rajveermalviya commented Feb 11, 2024

This PR finalizes the generations of implementation specific headers from custom yaml specifications aside from webgpu.yml.

Apart from the general struct extensions, the PR now allows extending enums, bitflags and objects with extra entires and functions across multiple yaml specifications.

Enums

Because C/C++ can't allow extending enums, we need to use casts, the downside being we loose exhaustive switches in C++ over new entries.

#if !defined(__WGPU_EXTEND_ENUM)
#ifdef __cplusplus
#define __WGPU_EXTEND_ENUM(E, N, V) static const E N = E(V)
#else
#define __WGPU_EXTEND_ENUM(E, N, V) static const E N = (E)(V)
#endif
#endif // !defined(__WGPU_EXTEND_ENUM)

__WGPU_EXTEND_ENUM(WGPUFeatureName, WGPUFeatureName_PushConstants, 0x30000000);
__WGPU_EXTEND_ENUM(WGPUFeatureName, WGPUFeatureName_MultiDrawIndirect, 0x30000001);

__WGPU_EXTEND_ENUM(WGPUSType, WGPUSType_ShaderModuleGLSLDescriptor, 0x30000000);
__WGPU_EXTEND_ENUM(WGPUSType, WGPUSType_InstanceExtras, 0x30000001);

Bitflags

Currently extended bitflag entries are similar to enums, but I think we should change generation of all bitflag entries to be static const T N = V instead of enum, because we need to make them 64bit anyway. Vulkan does the same for 64bit bitflags

__WGPU_EXTEND_ENUM(WGPUShaderStage, WGPUShaderStage_Mesh, 0x00000008);

webgpu_ext.h

Though not needed, but with this PR we can now create a separate common extension header webgpu_ext.h and webgpu_core.h for core API. So that we can move any native-specific extensions (SPIRV shader) that we currently have in webgpu.h to webgpu_ext.h and webgpu_core.h would try to contain only JS spec API.

Suffixes

Generator currently doesn't implicitly add any suffixes (_DAWN, _WGPU) to non-extended identifiers in extension headers, those will need to be added in the corresponding yaml specification explicitly, which I think is what we want because it's flexible that way.

@rajveermalviya
Copy link
Collaborator Author

Here's a sample with yaml and corresponding generated header.
https://gist.github.com/rajveermalviya/812ee0398ca88c1962469a0978a4845c

@kainino0x
Copy link
Collaborator

kainino0x commented Feb 16, 2024

EDIT: Moved this comment to #273

@kainino0x kainino0x added the !discuss Needs discussion (at meeting or online) label Feb 16, 2024
Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. let me copy that comment to a new issue.

#ifdef __cplusplus
#define __WGPU_EXTEND_ENUM(E, N, V) static const E N = E(V)
#else
#define __WGPU_EXTEND_ENUM(E, N, V) static const E N = (E)(V)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need a WGPU_ENUM_EXTENDED_VALUE_ATTRIBUTE here. I'm not really clear on when Swift needs NS_REFINED_FOR_SWIFT exactly, but it seems like it probably would need one here in order to refine our enums into Swift enums?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know syntactically where the NS_REFINED_FOR_SWIFT would need to go.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This does not block landing the PR. I don't think #179 is fully resolved right now anyway.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just defer this to when someone asks for it.

@rajveermalviya
Copy link
Collaborator Author

Namespace indicators

The extension yaml parameter types can use namespace field to indicate where the mentioned type comes from. For example when using the webgpu.yml types in wgpu.yml.

re: Suffixes

Now generator has a flag extsuffix (on by default), which will insert suffixes to the identifiers in appropriate format. The suffix is identified by the yaml file name.

wgpu-native will now use this generator to generate it's wgpu.h header. See gfx-rs/wgpu-native#373 for the new wgpu.yml & the resulting generated header.

@kainino0x
Copy link
Collaborator

The suffix is identified by the yaml file name.

I think it would be better to put the suffix in the yaml file, otherwise I think we wouldn't be able to split webgpu.yml into multiple files for example? Also generally I find it confusing to figure out where the suffix came from when it's implicit from the filename. (Took me a minute to figure this out even though you wrote it down.)

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good, I think you can land this whenever you want

@rajveermalviya
Copy link
Collaborator Author

Generator doesn't do anything implicit for extended bitflags yet, it expects custom values specified in yaml to be distinct.

@kainino0x added #214 back on agenda to specify the format of extended bitflags.

@rajveermalviya
Copy link
Collaborator Author

Merging, will make another PR for extended bitflags when the format is decided, also because it doesn't affect wgpu.h (it doesn't extend any core bitflags yet)

@rajveermalviya rajveermalviya merged commit da37690 into webgpu-native:main Mar 3, 2024
2 checks passed
@rajveermalviya rajveermalviya deleted the ext-gen branch March 3, 2024 09:58
@kainino0x kainino0x removed the !discuss Needs discussion (at meeting or online) label Mar 14, 2024
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.

2 participants