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

Move vulkan codegen generators 2 #1874

Open
wants to merge 73 commits into
base: dev
Choose a base branch
from

Conversation

MarkY-LunarG
Copy link
Contributor

This builds on #1851. For this review, start at commit 216cd68

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 300829.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5337 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5337 passed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 300955.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5338 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5338 passed.

@MarkY-LunarG MarkY-LunarG changed the title Marky move vulkan codegen generators 2 Move vulkan codegen generators 2 Nov 15, 2024
Copy link
Contributor

@jzulauf-lunarg jzulauf-lunarg left a comment

Choose a reason for hiding this comment

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

Review is WIP: book mark review progress at: codegen: Start using ApiData content b7f0c70

Copy link
Contributor

@jzulauf-lunarg jzulauf-lunarg left a comment

Choose a reason for hiding this comment

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

Review WIP: Bookmark codegen: Rename extraVulkanHeaders to extra_headers
278e534

Copy link
Contributor

@jzulauf-lunarg jzulauf-lunarg left a comment

Choose a reason for hiding this comment

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

Review WIP: Bookmark codegen: Update struct->json body af283a2

)

self.newline()
write('GFXRECON_END_NAMESPACE(util)', file=self.outFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this part be part of an end file function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can fix in the future. Nothing is broken by leaving this in the khronos files for now.

@@ -347,6 +347,8 @@ def __init__(
self.handle_names = set() # Set of current API's handle typenames
self.dispatchable_handle_names = set() # Set of current API's dispatchable handle typenames
self.flags_types = dict() # Map of flags types
self.flags_type_aliases = dict() # Map of flags type aliases
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment says "move" but this doesn't seem to have been deleted from anywhere.

''')
body = remove_trailing_newlines(indent_cpp_code(body))
write(body, file=self.outFile)

Copy link
Contributor

Choose a reason for hiding this comment

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

move up yapf enable

def make_pnext_body(self):
body = ''
for struct in self.pnext_extension_structs:
for struct in self.all_extended_structs:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not clear why these two are equivalent

err_file=err_file,
warn_file=warn_file,
diag_file=diag_file
self, err_file=err_file, warn_file=warn_file, diag_file=diag_file
Copy link
Contributor

Choose a reason for hiding this comment

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

why the reformatting?

GFXRECON_BEGIN_NAMESPACE(decode)
''')
body += "\n"
GFXRECON_BEGIN_NAMESPACE(decode)''')
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this not change whitespace in the output? Seems like it should, but there's no h/cpp diff in the commit. Hmmm...

@@ -107,7 +107,8 @@ class ApiData():
base_in_struct - The base input structure defined in this Khronos API
base_out_struct - The base output structure defined in this Khronos API
extended_struct_variable - The extended struct varible name used in this Khronos API
extended_struct_func_prefix - The function prefix to use for extended struct functions for this Khronos API.=
extended_struct_func_prefix - The function prefix to use for extended struct functions for this Khronos API.
Copy link
Contributor

Choose a reason for hiding this comment

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

trailing "." not consistent w/ comments above.

@@ -25,6 +25,7 @@
from base_generator import *
from reformat_code import format_cpp_code, indent_cpp_code, remove_leading_empty_lines, remove_trailing_newlines


Copy link
Contributor

Choose a reason for hiding this comment

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

stray newline

Copy link
Contributor

@jzulauf-lunarg jzulauf-lunarg left a comment

Choose a reason for hiding this comment

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

Not clear if this is generate_vulkan.py bisectable, at least one stray commit would indicate it isn't. A couple places where the two parts a "move" are in different commits. For changes where the contents of a generate_feature are being moved to endFile time, way cleaner to rename the functions/variables accessed in place. Better for review and maintenance.

@@ -160,25 +159,20 @@ def writeBodyContents(self):
body = remove_trailing_newlines(indent_cpp_code(body))
write(body, file=self.outFile)

stype_var = self.getStructTypeVarName()
Copy link
Contributor

Choose a reason for hiding this comment

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

Another way to do this would be to get the ApiData as a dictionary, pass that to format, using named {} fields

@@ -14013,7 +14013,7 @@ void FieldToJson(nlohmann::ordered_json& jdata, const Decoded_VkPipelineColorWri

FieldToJson(jdata["sType"], decoded_value.sType, options);
FieldToJson(jdata["attachmentCount"], decoded_value.attachmentCount, options);
FieldToJson(jdata["pColorWriteEnables"], meta_struct.pColorWriteEnables, options);
Bool32ToJson(jdata["pColorWriteEnables"], &meta_struct.pColorWriteEnables, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually an array, I wonder if this is correct in the original or this update... Hmmmm.

# Determine the appropriate type. If it's an alias, get
# the original type
value_type = value.base_type
if (self.is_struct(value.base_type) and value.base_type in self.all_struct_aliases):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is alias lookup a common operation?

@@ -82,6 +82,16 @@ def beginFile(self, gen_opts):

def endFile(self):
"""Method override."""
for cmd in self.get_all_filtered_cmd_names():
info = self.all_cmd_params[cmd]
proto = info[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR: What an awful way to store and retrieve this information, a tuple or array, with different types of contents at fixed indices...

@@ -126,17 +138,3 @@ def need_feature_generation(self):
if self.feature_cmd_params:
return True
return False

def generate_feature(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Easier to review if you simply rename the function/variables in situ, and then call from endFile

@@ -233,37 +262,6 @@ def need_feature_generation(self):
return True
return False

def generate_feature(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment. Rename the function, touch up the variable names in place, and then call from endfile. Easier to review, more maintainable.

Move the Vulkan generator scripts under a khronos_generators folder
to prepare it for for sharing with other khronos APIs.
DX12 was still relying on one file in the base_generators folder.
Make a copy in the DX12 folder, and remove all references to the Vulkan
and base_generators folders in the generate_dx12.py script.
Move the reformat_code.py into the base of khronos_generators.
Move common items from the vulkan_generators/base_generator.py
to the khronos_generators/khronos_base_generator.py
Move more common type parsing into the khronos_base_generator
from the vulkan/base_generator.
base_generator code still has vulkan-specific items in it.
This starts the process of pulling those out.
Remove more vulkan-specific items in the base_generator code
path.
Modify the handling of 'pNext' extended struct variables in the base_generators
to be more Khronos-generic instead of Vulkan-specific.
First phase moving of base_generator files to have a khronos
identifier
Use the ApiData for the structure type variable names and enum names.
This is a first step to moving the content into a separate file.
Move the actual methods for generating the decode of the pnext items
into a khronos generic file.
Move the encoding of the pnext items into separate methods.
Move the pNext encode functionality into the Khronos generator
since it is now API agnostic.
Update the writeCommonVulkanHeaders method so that it is in the
base Khronos api directory with a generic name.
Move the structure type util generation into the Khronos base.
Move the generation of the struct to json code for the header to
endFile time instead of generate_feature time.
Update the struct to json code so that more of the common functionality
sits in the khronos generators folder.
Change the struct to json body code so that the bulk of the functionality is in
separate methods.
Update the struct to json generation to use the extended structure information
and the structure type information from the khronos base.
Move the common struct to json body code into a Khronos file now.
Update the type used in the struct body generation to take
into account an alias.
Add the ability to generate the correct information when we encounter
a parent/child structure in the JSON output.
Move the generation of content to endFile time for the
vulkan_command_buffer_util generation.
Move code generation to endFile time for the
Vulkan API call encoder functionality.
Update more Vulkan python scripts to generate at endFile time instead
of generate_feature time.
Generate the Vulkan struct handle wrapping content at endFile time
instead of generate_feature time.
@MarkY-LunarG MarkY-LunarG force-pushed the marky-move-vulkan-codegen-generators-2 branch from 375b952 to b999a29 Compare November 21, 2024 20:09
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 307849.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5368 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5368 passed.

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.

3 participants