Implement relaxed rule for opaque struct members by sbourasse · Pull Request #3365 · KhronosGroup/glslang

sbourasse · 2023-10-16T14:01:46Z

This PR is a companion to another PR we recently opened on KhronosGroup/GLSL : KhronosGroup/GLSL#218
It implements the procedure described in the suggested changes to GL_EXT_vulkan_glsl_relaxed.

Samplers are extracted out of structures and moved to the topmost level available. In the context of uniform, that's the global uniform block. For structures passed as function parameters, we create invisible parameters and properly route arguments to them.

We also extend the vk.relaxed.frag test shader to demonstrate these added capabilities.

CLAassistant · 2023-10-16T14:01:54Z

All committers have signed the CLA.

arcady-lunarg · 2023-10-17T18:06:33Z

Looks like ASAN found some issues with your patch, do you want to address them before I take a look to review the changes? Also, is there some sequencing needed between this and the PR to then GLSL repo?

sbourasse · 2023-10-18T09:08:03Z

I wanted to try and merge both PR at about the same time so repos can remain in sync, but if changes are requested on the GLSL part, this could mean additional work on this side. I don't mind waiting on the other one if you don't want to invest time on a change that's not yet approved.

I haven't entirely figured out how memory ownership is handled within the codebase, so I figured there would be some leaks in the end. I'll look into them by the end of the week.
In the meantime and depending on your own schedule, I wouldn't mind a first review. I kind of forced my changes into the codebase so any feedback is appreciated.

ncesario-lunarg · 2023-10-18T18:44:17Z

I haven't entirely figured out how memory ownership is handled within the codebase

Some classes use the TPoolAllocator rather than the standard C++ allocator. Allocations made from the pool allocator are "free" (this is covered at the end of the README). I plan on making these allocations more obvious, so that it's easier to know which allocations must be cleaned up and which ones don't need to.

ncesario-lunarg

I think some of the logic around newParams in the vkRelaxedRemapFunctionParameter needs to be reworked. Let me know if you have more questions about memory allocations in the parser.

ncesario-lunarg · 2023-10-18T19:03:59Z

+                      memberType->getQualifier().storage = param.type->getQualifier().storage;
+                      memberType->clearArraySizes();
+
+                      TParameter* memberParam = new TParameter();


TParameter does not use the pool allocator (by default at least), so this is using the standard allocator and would need to be cleaned up. I'm not sure if this is the right way to go about this. You could use the pool allocator, but it seems like you could also retrieve the necessary parameters from function after vkRelaxedRemapFunctionParameter is called (though you may need to add an additional field to TParameter to pick out the parameters you want)?

I replaced the last function parameter with a vector of integer to avoid changing TParameter, but maybe it would be better to properly flag parameters that are implicitly created during this pass ?

ncesario-lunarg · 2023-10-18T19:08:14Z

+
+    ForEachOpaque(type, identifier,
+                  [&publicType, &loc, this](const TType& type, const TString& path) {
+                      TString& structMemberName = *(new TString(path));


This is another spot ASAN is complaining about. You can use NewPoolTString, but does this need to be dynamically allocated here?

~~True, this should be a straight copy~~
Nevermind. It looks like we need the allocation, because declareVariable takes a TString& as argument

sbourasse · 2023-10-24T14:05:26Z

Thanks for reviewing ! My latest changes should fix leaks

arcady-lunarg

I have a couple of minor nits but overall this seems fine. It's a lot of fairly complicated code but that code is only used in relaxed mode.

arcady-lunarg · 2023-11-15T23:33:18Z

-        $1.function->addParameter(param);
-        $$.function = $1.function;
-        $$.intermNode = $2;
+        if (!(parseContext.spvVersion.vulkan > 0


I would prefer you invert the condition of the if-statement so that it's not negated, I think it's easier to read "if (vulkanRelaxed && containsOpaque) { // do something unusual } else { // do the normal thing".

arcady-lunarg · 2023-11-16T00:10:55Z

 }

-bool TParseContext::vkRelaxedRemapUniformVariable(const TSourceLoc& loc, TString& identifier, const TPublicType&,
+bool TParseContext::vkRelaxedRemapUniformVariable(const TSourceLoc& loc, TString& identifier, const TPublicType& publicType,


publicType appears to be unused, you should keep it anonymous to prevent a compiler warning.

It's forwarded to vkRelaxedRemapUniformMembers. I looked across the file for another publicType that seemed unused but didn't find any.

arcady-lunarg · 2023-11-16T17:54:31Z

-        $$.intermNode = parseContext.intermediate.growAggregate($1.intermNode, $3, $2.loc);
+        if (!(parseContext.spvVersion.vulkan > 0
+              && parseContext.spvVersion.vulkanRelaxed)
+            || !$3->getType().containsOpaque())


Same comment re inverting the condition to make it clearer.

arcady-lunarg

Thanks, looks good now.

Implement relaxed rule for opaque struct members.

f8e376a

arcady-lunarg added the kokoro:run Trigger Google bot runs label Oct 16, 2023

kokoro-team removed the kokoro:run Trigger Google bot runs label Oct 16, 2023

ncesario-lunarg requested changes Oct 18, 2023

View reviewed changes

sbourasse force-pushed the bourasse/relaxed-opaque-fix branch from c62630d to 9ab942f Compare October 24, 2023 13:49

Fix TString and TParameter leaks

9ab942f

arcady-lunarg added the kokoro:run Trigger Google bot runs label Oct 26, 2023

kokoro-team removed the kokoro:run Trigger Google bot runs label Oct 26, 2023

arcady-lunarg requested changes Nov 16, 2023

View reviewed changes

Reorder conditions to make them clearer.

0289ca1

arcady-lunarg approved these changes Nov 28, 2023

View reviewed changes

arcady-lunarg added the kokoro:run Trigger Google bot runs label Nov 28, 2023

kokoro-team removed the kokoro:run Trigger Google bot runs label Nov 28, 2023

arcady-lunarg merged commit c59b876 into KhronosGroup:main Nov 29, 2023

Conversation

sbourasse commented Oct 16, 2023

Uh oh!

CLAassistant commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arcady-lunarg commented Oct 17, 2023

Uh oh!

sbourasse commented Oct 18, 2023

Uh oh!

ncesario-lunarg commented Oct 18, 2023

Uh oh!

ncesario-lunarg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ncesario-lunarg Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

sbourasse Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

ncesario-lunarg Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

sbourasse Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbourasse commented Oct 24, 2023

Uh oh!

arcady-lunarg left a comment

Choose a reason for hiding this comment

Uh oh!

arcady-lunarg Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

arcady-lunarg Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

sbourasse Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

arcady-lunarg Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

arcady-lunarg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Oct 16, 2023 •

edited

Loading

sbourasse Oct 24, 2023 •

edited

Loading