Add support for tesc, tese and geom to EliminateDead*Components #4990

greg-lunarg · 2022-11-16T01:01:25Z

No description provided.

greg-lunarg · 2022-11-16T01:36:51Z

Keenuts · 2022-11-16T12:01:48Z

source/opt/eliminate_dead_input_components_pass.cpp

@@ -55,23 +63,37 @@ Pass::Status EliminateDeadInputComponentsPass::Process() {
    if (ptr_type == nullptr) {
      continue;
    }
+    auto sclass = ptr_type->storage_class();


Keenuts · 2022-11-16T12:03:01Z

source/opt/eliminate_dead_input_components_pass.cpp

@@ -42,6 +43,13 @@ Pass::Status EliminateDeadInputComponentsPass::Process() {
  // Current functionality assumes shader capability.
  if (!context()->get_feature_mgr()->HasCapability(spv::Capability::Shader))
    return Status::SuccessWithoutChange;
+  // Current functionality assumes vert, frag, tesc, tese or geom shader


nit: missing full stop.

source/opt/eliminate_dead_input_components_pass.cpp

Keenuts · 2022-11-16T12:13:05Z

source/opt/eliminate_dead_input_components_pass.cpp

+    // For tesc, or input variables in tese or geom shaders,
+    // only optimize arrays and skip the first level of arrayness.


Can you explain why? IMO a comment should not describe what the code does, but why the code is doing something.

In general, these particular variables MUST be arrays. The outer array is called the "per-vertex-array". It is completely orthogonal to this analysis/optimization and can and really must be ignored. This is a somewhat tricky and confusing aspect of input/output variables in these shaders. Please refer to the GLSL spec and search for per-vertex-array.

I will improve the comment per this explanation.

For example, if the vertex shader looks like this:

#version 450 layout (location = 0) out Vertex { vec4 Cd; vec2 uv; } oVert; layout (location = 0) in vec4 P; void main() { oVert.Cd = P; }

the following tesc shader will match:

#version 450 layout (vertices = 4) out; layout (location = 0) in Vertex { vec4 Cd; vec2 uv; } iVert[]; layout (location = 0) out vec4 position[4]; void main() { vec4 pos = iVert[gl_InvocationID].Cd; position[gl_InvocationID] = pos; }

Note oVert is scalar in the vert shader, but the matching iVert is an array in the tesc shader. See also 15.1.3 in the Vulkan spec.

Ok, thanks for the explanation. Thought it could be applied to other arrays, like uniforms and such, but they don't have the Input storage class.
And because this only is applied on the unsafe pass, which means input/output will undergo the same behavior, we shouldn't have any stride issue.
Thanks for taking the time to explain all that!

source/opt/eliminate_dead_input_components_pass.cpp

Keenuts · 2022-11-16T12:33:08Z

source/opt/eliminate_dead_input_components_pass.cpp

-  const analysis::Struct* struct_ty = ptr_type->pointee_type()->AsStruct();
+  auto core_type = ptr_type->pointee_type();
+  // Check for array of struct from tesc, tese and geom
+  auto arr_type = core_type->AsArray();


Keenuts · 2022-11-16T12:35:08Z

source/opt/eliminate_dead_input_components_pass.cpp

-        unsigned idx_id = use->GetSingleWordInOperand(kAccessChainIndex0InIdx);
+        unsigned in_idx = skip_first_index ? kAccessChainIndex1InIdx
+                                           : kAccessChainIndex0InIdx;
+        unsigned idx_id = use->GetSingleWordInOperand(in_idx);


nit: const (base_id, in_idx, idx_id)

Keenuts · 2022-11-16T12:35:26Z

source/opt/eliminate_dead_input_components_pass.cpp

@@ -116,12 +138,14 @@ Pass::Status EliminateDeadInputComponentsPass::Process() {
 }

 unsigned EliminateDeadInputComponentsPass::FindMaxIndex(Instruction& var,
-                                                        unsigned original_max) {
+                                                        unsigned original_max,


you can mark all those parameters as const (especially the reference)

greg-lunarg · 2022-11-16T17:36:36Z

@Keenuts Thank you for the prompt review! It is much appreciated!

greg-lunarg · 2022-11-17T01:12:01Z

@Keenuts I believe I have addressed all your concerns. PTAL.

Keenuts

Thanks! some additional items and we should be good to go!

Keenuts · 2022-11-17T11:05:20Z

source/opt/eliminate_dead_input_components_pass.cpp

@@ -27,6 +27,7 @@ namespace {

 const uint32_t kAccessChainBaseInIdx = 0;
 const uint32_t kAccessChainIndex0InIdx = 1;
+const uint32_t kAccessChainIndex1InIdx = 2;


there is PR that should land today (that will change const into constexpr for all opt/ folder), we'll see which one is submitted first.

Yep. Merged with that.

Keenuts · 2022-11-17T11:06:41Z

source/opt/eliminate_dead_input_components_pass.cpp

  if (stage != spv::ExecutionModel::Vertex && vertex_shader_only_)
    return Status::SuccessWithoutChange;
  // Current functionality assumes shader capability.
  if (!context()->get_feature_mgr()->HasCapability(spv::Capability::Shader))
    return Status::SuccessWithoutChange;
+  // Current functionality assumes vert, frag, tesc, tese or geom shader.
+  // TODO(greg-lunarg): Add GLCompute.


maybe TODO(issue #4988) ?
(so people can have context without having to contact you)

Keenuts · 2022-11-17T11:59:47Z

source/opt/eliminate_dead_input_components_pass.cpp

+    // For tesc, or input variables in tese or geom shaders,
+    // only optimize arrays and skip the first level of arrayness.


Ok, thanks for the explanation. Thought it could be applied to other arrays, like uniforms and such, but they don't have the Input storage class.
And because this only is applied on the unsafe pass, which means input/output will undergo the same behavior, we shouldn't have any stride issue.
Thanks for taking the time to explain all that!

Keenuts · 2022-11-17T12:46:49Z

source/opt/eliminate_dead_input_components_pass.cpp

+  // Check for array of struct from tesc, tese and geom
+  const auto arr_type = core_type->AsArray();
+  if (arr_type) core_type = arr_type->element_type();
+  const analysis::Struct* struct_ty = core_type->AsStruct();


missed this in the first review, sorry.

this is called ChangeStructType, and was a straightforward function: take a struct, change its length.

now it's doing some magic with arrays, allowing one level of array top be skipped. I understand the goal in this context, but don't think this should be done here.

What about:

this function takes a struct type, and returns a new type with the proper size.

outside, you do struct_var->setResultType(resized_type);

This way, the function remains true to its name, and doesn't have some less known behaviors.

Good point. But rather than change the function to be true to the name, I decided to change the name :)

I changed the name to ChangeIOVarStructLength, which is to say, change the (outermost) struct length in the IO var. I also renamed the argument from struct_var to io_var to reflect that the var may not be be a struct (eg a per-vertex-array of struct). I also improved the header comment of this function to explain this all.

greg-lunarg · 2022-11-17T19:21:53Z

@Keenuts I believe I have addressed your latest concerns. PTAL.

Keenuts

Thanks. One small nit and we are good. (Will drop later tonight to re-approve after your push so you can submit before the weekend)

Keenuts · 2022-11-18T10:09:58Z

source/opt/eliminate_dead_input_components_pass.cpp

 namespace {
+


consistency with other files, no spacing around those namespaces and before

greg-lunarg · 2022-11-18T18:15:24Z

@Keenuts I believe I have addressed your most recent concerns. PTAL.

Keenuts

Thanks!

greg-lunarg requested a review from Keenuts November 16, 2022 01:37

greg-lunarg force-pushed the edic8 branch from 4f39595 to 958d5b2 Compare November 16, 2022 01:52

Keenuts reviewed Nov 16, 2022

View reviewed changes

greg-lunarg force-pushed the edic8 branch from 958d5b2 to 4a44613 Compare November 17, 2022 00:28

greg-lunarg force-pushed the edic8 branch from 4a44613 to 6be48cf Compare November 17, 2022 01:37

Keenuts requested changes Nov 17, 2022

View reviewed changes

greg-lunarg force-pushed the edic8 branch 2 times, most recently from 05944aa to a9050dc Compare November 17, 2022 18:33

Keenuts previously approved these changes Nov 18, 2022

View reviewed changes

Add support for tesc, tese and geom to EliminateDead*Components

ce85332

greg-lunarg dismissed Keenuts’s stale review via ce85332 November 18, 2022 17:51

greg-lunarg force-pushed the edic8 branch from a9050dc to ce85332 Compare November 18, 2022 17:51

Keenuts approved these changes Nov 18, 2022

View reviewed changes

greg-lunarg merged commit 46ca66e into KhronosGroup:master Nov 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for tesc, tese and geom to EliminateDead*Components #4990

Add support for tesc, tese and geom to EliminateDead*Components #4990

greg-lunarg commented Nov 16, 2022

greg-lunarg commented Nov 16, 2022

Keenuts Nov 16, 2022

greg-lunarg Nov 16, 2022

Keenuts Nov 16, 2022

greg-lunarg Nov 16, 2022

Keenuts Nov 16, 2022

greg-lunarg Nov 16, 2022

greg-lunarg Nov 16, 2022 •

edited

Keenuts Nov 17, 2022

Keenuts Nov 16, 2022

greg-lunarg Nov 17, 2022

Keenuts Nov 16, 2022

greg-lunarg Nov 17, 2022

Keenuts Nov 16, 2022

greg-lunarg Nov 17, 2022

greg-lunarg commented Nov 16, 2022

greg-lunarg commented Nov 17, 2022

Keenuts left a comment

Keenuts Nov 17, 2022

greg-lunarg Nov 17, 2022

Keenuts Nov 17, 2022

Keenuts Nov 17, 2022

Keenuts Nov 17, 2022

greg-lunarg Nov 17, 2022

greg-lunarg commented Nov 17, 2022

Keenuts left a comment

Keenuts Nov 18, 2022

greg-lunarg Nov 18, 2022

greg-lunarg commented Nov 18, 2022

Keenuts left a comment

		// For tesc, or input variables in tese or geom shaders,
		// only optimize arrays and skip the first level of arrayness.

Add support for tesc, tese and geom to EliminateDead*Components #4990

Add support for tesc, tese and geom to EliminateDead*Components #4990

Conversation

greg-lunarg commented Nov 16, 2022

greg-lunarg commented Nov 16, 2022

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

greg-lunarg Nov 16, 2022 • edited

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

greg-lunarg commented Nov 16, 2022

greg-lunarg commented Nov 17, 2022

Keenuts left a 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

greg-lunarg commented Nov 17, 2022

Keenuts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-lunarg commented Nov 18, 2022

Keenuts left a comment

Choose a reason for hiding this comment

greg-lunarg Nov 16, 2022 •

edited