[WIP] Implement extension chaining #183

MaikKlein · 2019-02-03T11:50:59Z

This is only a proof on concept right now. I'll finish it up next week.

What are your thoughts? Right now this requires a cast from *const to *mut.

Ralith · 2019-02-03T19:09:33Z

generator/src/lib.rs

+        pub struct ExtensionChainMut {
+            pub s_type: StructureType,
+            pub p_next: *mut c_void
+        }


Ralith · 2019-02-03T19:13:26Z

generator/src/lib.rs

+        pub struct ExtensionChain {
+            pub s_type: StructureType,
+            pub p_next: *mut c_void
+        }


Shouldn't this be named BaseStructure or similar, along the lines of upstream?

There is also vk::BaseOutStructure which we might just reuse.

That makes sense; then the loop helper can just be a top-level function, which perhaps shouldn't even be exported; it doesn't seem very broadly useful, and reducing API surface area is nice.

Ralith · 2019-02-03T19:15:19Z

generator/src/lib.rs

+                unsafe{
+                    let ptr = &mut self.inner as *mut _ as *mut c_void;
+                    let last_extension = ExtensionChain::last_chain(ptr);
+                    (*last_extension).p_next = next.deref_mut() as *mut T as *mut c_void;


I kinda like the idea of prepending to the chain rather than appending, since it means we don't have to rely on the optimizer to erase the loop. On the other hand, the resulting sequence of links is less intuitive, and I'm not certain that the ordering never matters.

Ralith · 2019-02-28T01:35:49Z

Difficult to review this due to the mangled history. Rebase?

Ralith · 2019-03-02T16:36:28Z

examples/src/lib.rs

+            use std::ops::Deref;
+            println!("--");
+            println!("{:?}", corner.deref() as *const _);
+            println!("{:?}", variable_pointers.deref() as *const _);


Do we still need these prints?

No I'll convert this code to a test. Not quite sure where to put it though. Possibly into the vk.rs.

Ralith · 2019-03-02T16:39:47Z

generator/src/lib.rs

                unsafe{
-                    let last_extension = ptr_chain_iter(&mut self.inner)
-                        .last()
-                        .expect("Initial ptr was null");


Can we get rid of ptr_chain_iter too, now, and keep the API narrower?

I wanted to use it for tests. Maybe we can just define it inside the test module.

Ralith · 2019-03-02T16:40:02Z

generator/src/lib.rs

-                        .expect("Initial ptr was null");
-                    // Append the extension at the end
-                    (*last_extension).p_next = next as *mut T as *mut _;
+                    let next_ptr: *mut BaseOutStructure = ::std::mem::transmute(next);


Transmute isn't needed for casting between pointers.

Ralith · 2019-03-02T16:44:23Z

generator/src/lib.rs


-    let next_function = if has_next {
+    let next_extends = _struct.extends.as_ref().map(|extends| {


This variable name is confusing. Maybe something like extension_trait_name?

Ralith · 2019-03-02T16:44:42Z

generator/src/lib.rs

+    let next_extends = _struct.extends.as_ref().map(|extends| {
+        let extends = extends
+            .split(',')
+            .find(|extend| root_create_info_names.contains(&extend.to_string()))


When is this check necessary? Do there exist structs that extend non-root structs?

Do there exist structs that extend non-root structs?

Yes not all extend structs are root. I'll add a comment, and I to slightly rewrite this anyway.

Ralith · 2019-03-02T16:45:25Z

generator/src/lib.rs

+            .split(',')
+            .find(|extend| root_create_info_names.contains(&extend.to_string()))
+            .expect("Should have a root create info");
+        name_to_tokens(&format!("Extends{}", name_to_tokens(&extends)))


Shouldn't we be implementing the extension trait of every struct that this struct extends, rather than just an arbitrary one?

What do you mean by arbitrary one? Only root create infos will create a new trait, and only non root create infos that extend a root create info will implement this trait.

Unless I'm missing something, this seems to be used for deciding when to implement the trait, not when to define it. A single extension struct (e.g. VkPhysicalDeviceVariablePointerFeatures) can extend multiple root structs, and must implement all of their extension traits.

Ah yes that is what I wanted to implement. I just chose the first one.

Ralith · 2019-03-02T16:47:40Z

generator/src/lib.rs

@@ -1613,7 +1612,9 @@ pub fn derive_setters(
    // implement
    let next_trait = if has_next && _struct.extends.is_none() {
        quote! {
-            pub unsafe trait #extends_name {}
+            pub unsafe trait #extends_name {
+                unsafe fn as_ptr_mut(&mut self) -> *mut BaseOutStructure;


Instead of generating more code here, we should mark all builders as #[repr(transparent)] and coerce the pointers directly in push_next. This is legal because the builders contain only one non-zero-sized member.

That is a great idea, I completely forgot about #[repr(transparent)].

Ralith · 2019-03-02T21:30:49Z

We shouldn't be referring to extensible root structs as "create infos," because not every extensible struct is named SomethingCreateInfo. For example, consider output structs like VkPhysicalDeviceFeatures2.

if there are more than 1

MaikKlein · 2019-03-04T09:26:50Z

@Ralith I think I addressed all issues.

We shouldn't be referring to extensible root structs as "create infos," because not every extensible struct is named SomethingCreateInfo. For example, consider output structs like VkPhysicalDeviceFeatures2.

I don't think that I am? I think I only refer to root create infos if they are structs that are passed into a function.

Ralith · 2019-03-04T17:55:34Z

generator/src/lib.rs


    // We only implement a next methods for root create infos
-    let next_function = if has_next && next_extends.is_none() {
+    let next_function = if has_next && root_extends.is_empty() {
        quote! {
            /// Prepends the given extension struct between the root and the first pointer. This
            /// method only exists on create infos that can be passed to a function directly. Only


This method only exists on create infos that can be passed to a function directly

This is incorrect, because this method exists (and is useful) on output structs too.

Do you have an example?

For example, it should be (but seemingly is not yet) defined for VkPhysicalDeviceProperties2. The same goes for any other root output struct with a p_next member.

The current implementation doesn't allow for pointer chains to be added. As in push_next on VkPhysicalDeviceProperties2 and then insert VkPhysicalDeviceProperties2 into DeviceCreateInfo. We would need to iterate over the chains (which is fine). I don't think this is necessary though. push_next only on root create infos seems fine to me.

I don't understand what you're trying to say. VkPhysicalDeviceProperties2 is an output struct; it does not extend VkDeviceCreateInfo, it is used to return data from vkGetPhysicalDeviceProperties2. It, and every struct like it, needs exactly the same extension semantics as you've implemented so far for root CreateInfo structs.

Okay I see what you mean. It is correctly implemented (as in VkPhysicalDeviceProperties2 has a push_next fn). I'll just have to update the comments.

Oh, so it is, was just searching badly.

Ralith · 2019-03-04T17:56:31Z

generator/src/lib.rs

+            }
+        }
+    }
+}


Does this need to be in the generated code, rather than a regular module of the ash crate?

We could have this code outside of the generator, and if we have ash-sys it will then be part of that crate. I'll change it.

Ralith · 2019-03-08T16:46:13Z

generator/src/lib.rs

+        .iter()
+        .filter_map(|definition| match *definition {
+            vkxml::DefinitionsElement::Struct(ref _struct) => {
+                let is_root_create_info = _struct.extends.is_none();


This is incorrect for VkPhysicalDeviceProperties2, which is both a root getter struct and an extension struct for CreateInfo

Instead, why don't we use the set of things that appears in structextends attributes? I haven't seen any cases where those are layered, though maybe I haven't looked hard enough.

which is both a root getter struct and an extension struct for CreateInfo

What exactly do you mean by that? Afaik it is just used in vkGetPhysicalDeviceProperties2.

Disregard, I was confused; too early for code review...

Don't worry happens to me all the time :).

MaikKlein · 2019-03-08T20:06:45Z

Right now the only thing that is missing are better variable names and fixing some comments right?

Ralith · 2019-03-09T01:18:17Z

There's still the test code in the generator that doesn't need to be there.

MaikKlein · 2019-03-09T16:05:01Z

I believe we should still iterate through the chains even if it not necessary for the builder.(Attach head and tail) You can still create pointer chains without the builders and inserting those would not work. Otherwise we could implement ExtendXXX only for builders but I don't think that would be better.

Ralith · 2019-03-09T17:24:23Z

How about Iterating through the chain of the argument to push_next instead of self? That way we preserve everything, but in the overwhelmingly common case of single structs being supplied at a time, no extra work needs to be done.

MaikKlein · 2019-03-09T17:31:15Z

How about Iterating through the chain of the argument to push_next instead of self? That way we preserve everything, but in the overwhelmingly common case of single structs being supplied at a time, no extra work needs to be done.

Yeah that was the idea :).

Ralith · 2019-03-10T06:56:16Z

generator/src/lib.rs

@@ -2144,6 +2198,23 @@ pub fn write_source_code(path: &Path) {
    let source_code = quote! {
        use std::fmt;
        use std::os::raw::*;
+        /// Iterates through the pointer chain. Includes the item that is passed into the function.
+        /// Stops at the last `BaseOutStructure` that has a null `p_next` field.
+        pub unsafe fn ptr_chain_iter<T>(


Is there a good reason for this to be part of the public API?

It is used in the test api. I am not sure, is it even possible to share pub(crate) code in the external tests module? I could move the tests into lib.rs, and it should work with use super::vk.

That sounds like a better solution to me. You could also have an internal tests module, rather than cramming everything into lib.rs.

We should be trying to minimize the amount of code the generator emits (because it's inconvenient to manipulate) and the amount of public API that doesn't have a specific strong motivation (for ease of maintenance).

Agreed, a lot of the trait / macro definitions could be defined outside of the generator.

Ralith · 2019-03-10T06:57:29Z

ash/src/extensions/experimental/amd.rs

@@ -657,11 +657,18 @@ impl<'a> ::std::ops::Deref for PhysicalDeviceWaveLimitPropertiesAmdBuilder<'a> {
    }
 }
 impl<'a> PhysicalDeviceWaveLimitPropertiesAmdBuilder<'a> {
-    pub fn next<T>(mut self, next: &'a mut T) -> PhysicalDeviceWaveLimitPropertiesAmdBuilder<'a>
+    pub fn push_next<T>(


I'm confused about where this definition is coming from. It's missing the chain support added to the generated version. I guess it's hand-written? Is that necessary?

Yes it is handwritten. I am not sure what we should do about it. Maybe we should remove all the builder stuff because it will probably get out of date anyway?

I am going to update it anyway.

Everything in experimental won't adhere to semver, so we can remove some parts in the future. I probably should document that.

Not a blocker, then, but why isn't this generated?

Oh this is not included in the vk.xml. That is why it is inside the experimental module. @msiglreith basically reversed the API :). This will be deleted when it is officially included in the vk.xml.

Ralith

Remaining issues aren't stability-sensitive, and can be fixed in future work.

Ralith · 2019-03-10T18:54:20Z

ash/src/extensions/experimental/amd.rs

@@ -657,11 +657,18 @@ impl<'a> ::std::ops::Deref for PhysicalDeviceWaveLimitPropertiesAmdBuilder<'a> {
    }
 }
 impl<'a> PhysicalDeviceWaveLimitPropertiesAmdBuilder<'a> {
-    pub fn next<T>(mut self, next: &'a mut T) -> PhysicalDeviceWaveLimitPropertiesAmdBuilder<'a>
+    pub fn push_next<T>(


Not a blocker, then, but why isn't this generated?

Ralith · 2019-03-10T18:56:05Z

generator/src/lib.rs

@@ -2144,6 +2198,23 @@ pub fn write_source_code(path: &Path) {
    let source_code = quote! {
        use std::fmt;
        use std::os::raw::*;
+        /// Iterates through the pointer chain. Includes the item that is passed into the function.
+        /// Stops at the last `BaseOutStructure` that has a null `p_next` field.
+        pub(crate) unsafe fn ptr_chain_iter<T>(


Because it's static code, this should probably be defined in ash/lib.rs, not spliced into the generator output.

MaikKlein · 2019-03-10T19:07:40Z

bors r+

183: [WIP] Implement extension chaining r=MaikKlein a=MaikKlein This is only a proof on concept right now. I'll finish it up next week. What are your thoughts? Right now this requires a cast from `*const` to `*mut`. Co-authored-by: Maik Klein <[email protected]>

bors · 2019-03-10T19:14:00Z

Build succeeded

MaikKlein mentioned this pull request Feb 3, 2019

Experimental AMD extensions #184

Merged

Ralith reviewed Feb 3, 2019

View reviewed changes

MaikKlein mentioned this pull request Feb 22, 2019

Extension structs can't be chained #165

Closed

MaikKlein force-pushed the extension-structs branch from e7d9494 to b7863d1 Compare February 23, 2019 11:57

MaikKlein force-pushed the extension-structs branch 2 times, most recently from 5b427dd to b5257bf Compare February 28, 2019 09:58

MaikKlein added 8 commits February 28, 2019 10:59

Update to 1.1.97

681eec5

Implement extension chaining

6d72bb5

Replace ExtensionChain with a simple iterator

c46a41e

Implement push_next

4f7ca18

Implement ExtendsXXX for all builders as well

3de2ca9

Add lifetime to push_next

c66ae6b

Add some documentation to push_next

1e13b99

Update experimental AMD extension

d6a6aa3

MaikKlein force-pushed the extension-structs branch from b5257bf to d6a6aa3 Compare February 28, 2019 10:00

Ralith requested changes Mar 2, 2019

View reviewed changes

MaikKlein added 6 commits March 3, 2019 14:33

Use repr transparent for builders

38fe16f

Implement ExtendXXX for multiple root create infos

215511f

if there are more than 1

Transform the ptr chain sample into a test case

04dbf20

Reformat experimental amd

2ecc483

Remove mut

4c1c8aa

Rename root_extends

88b1ac0

MaikKlein requested a review from Ralith March 4, 2019 09:56

Ralith requested changes Mar 4, 2019

View reviewed changes

MaikKlein mentioned this pull request Mar 8, 2019

Pass constant size arrays by reference #191

Merged

Ralith reviewed Mar 8, 2019

View reviewed changes

Add example

d667e58

MaikKlein force-pushed the extension-structs branch from e61bfd6 to c8253cf Compare March 9, 2019 18:35

MaikKlein added 3 commits March 9, 2019 19:51

Replace root create info with root_struct

bf29ebb

next can contain a pointer chain and we need to correct insert it.

c8c8f69

Move ptr chain tests out of the generator

1744159

MaikKlein force-pushed the extension-structs branch from c8253cf to 1744159 Compare March 9, 2019 18:51

Add ptrchain external test file

2d730cd

MaikKlein requested a review from Ralith March 9, 2019 21:10

Ralith reviewed Mar 10, 2019

View reviewed changes

MaikKlein force-pushed the extension-structs branch from b4c5ffa to 2635e86 Compare March 10, 2019 11:48

MaikKlein added 2 commits March 10, 2019 12:56

Move external tests to lib.rs

7d5d200

Update experimental builders

cbc96b2

MaikKlein force-pushed the extension-structs branch from 2635e86 to cbc96b2 Compare March 10, 2019 11:56

Ralith approved these changes Mar 10, 2019

View reviewed changes

bors bot merged commit cbc96b2 into master Mar 10, 2019

MarijnS95 deleted the extension-structs branch April 13, 2021 09:21


		let next_function = if has_next {
		let next_extends = _struct.extends.as_ref().map(\|extends\| {

[WIP] Implement extension chaining #183

[WIP] Implement extension chaining #183

Conversation

MaikKlein commented Feb 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith commented Feb 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith commented Mar 2, 2019

MaikKlein commented Mar 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaikKlein commented Mar 8, 2019

Ralith commented Mar 9, 2019

MaikKlein commented Mar 9, 2019

Ralith commented Mar 9, 2019

MaikKlein commented Mar 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaikKlein commented Mar 10, 2019

bors bot commented Mar 10, 2019

Build succeeded