Add pass to eliminate dead output components #4982

greg-lunarg · 2022-11-03T18:46:39Z

This pass eliminates components of output variables that are not stored to. Currently this just eliminates trailing components of arrays and structs, all of which are dead.

WARNING: This pass is not designed to be a standalone pass as it can cause interface incompatibiliies with the following shader in the pipeline. See the comment in optimizer.hpp for best usage. This pass is currently available only through the API; it is not available in the CLI.

This commit also fixes a bug in CreateDecoration() which is part of the system of generating SPIR-V from the Type manager.

greg-lunarg · 2022-11-03T20:49:03Z

@Keenuts Please take a look. This is another piece of the cross-shader dead code elimination. Once dead output variable stores are eliminated, this pass removes unset components of all output variables.

spencer-lunarg · 2022-11-04T07:21:09Z

source/opt/eliminate_dead_input_components_pass.h

@@ -57,6 +58,9 @@ class EliminateDeadInputComponentsPass : public Pass {

  // Change the length of the struct |struct_var| to |length|
  void ChangeStructLength(Instruction& struct_var, unsigned length);
+
+  // Process output variables instead
+  bool output_instead_;


Any reason to not just rename this pass EliminateDeadInterfaceComponentsPass? It is definitely not intuitive that the output part is in the input file/pass as well (without the extra code mapping).

If we think the churn is too much, might be worth adding a note explaining that Input was here first and was easier to add this boolean then do a rename

I will rename the pass and the files.

Actually, I promise to rename the passes and files :) I have two other PRs that are already created, and renaming at this point would be a pain. The Create*() functions are the true interface for users and those are correctly named, so I don't think there is harm in delaying the renaming of the passes and files until these PRs have landed.

Keenuts · 2022-11-04T09:56:48Z

source/opt/type_manager.cpp

@@ -470,33 +470,32 @@ uint32_t TypeManager::FindPointerToType(uint32_t type_id,

 void TypeManager::AttachDecorations(uint32_t id, const Type* type) {
  for (auto vec : type->decorations()) {
-    CreateDecoration(id, vec);
+    CreateDecoration(id, vec, false);


replace false with /* is_member */ false

Since is_member is now default false, went back to original.

Keenuts · 2022-11-04T09:56:59Z

source/opt/type_manager.cpp

  }
  if (const Struct* structTy = type->AsStruct()) {
    for (auto pair : structTy->element_decorations()) {
      uint32_t element = pair.first;
      for (auto vec : pair.second) {
-        CreateDecoration(id, vec, element);
+        CreateDecoration(id, vec, true, element);


replace true with /* is_member */ true

Keenuts · 2022-11-04T09:59:32Z

source/opt/type_manager.h

@@ -247,15 +252,11 @@ class TypeManager {
  // is created. The annotation is registered with the DefUseManager and the
  // DecorationManager.
  void CreateDecoration(uint32_t id, const std::vector<uint32_t>& decoration,
-                        uint32_t element = 0);
+                        bool is_member, uint32_t element = 0);


why not put is_member default value to false? or remove element default value?
Half optional half mandatory is IMO weird for 2 parameters that are linked.

OK. Will make is_member default false.

Keenuts · 2022-11-04T10:03:41Z

source/opt/optimizer.cpp

@@ -1034,6 +1034,11 @@ Optimizer::PassToken CreateEliminateDeadOutputStoresPass(
      MakeUnique<opt::EliminateDeadOutputStoresPass>(live_locs, live_builtins));
 }

+Optimizer::PassToken CreateEliminateDeadOutputComponentsPass() {
+  return MakeUnique<Optimizer::PassToken::Impl>(
+      MakeUnique<opt::EliminateDeadInputComponentsPass>(true));


/* output_instead */ true if you keep this as-is.

OK, since I am keeping as-is for the moment.

Keenuts · 2022-11-04T10:04:25Z

source/opt/eliminate_dead_input_components_pass.h

@@ -28,7 +28,8 @@ namespace opt {
 // See optimizer.hpp for documentation.
 class EliminateDeadInputComponentsPass : public Pass {
 public:
-  explicit EliminateDeadInputComponentsPass() {}
+  explicit EliminateDeadInputComponentsPass(bool output_instead = false)


If you rename the pass as spencer suggested, I'd be in favor to not default the value to false, but to make it mandatory.

Agreed. Will do so when I make the change.

Keenuts · 2022-11-04T10:20:15Z

source/opt/eliminate_dead_input_components_pass.cpp

@@ -138,18 +155,12 @@ void EliminateDeadInputComponentsPass::ChangeArrayLength(Instruction& arr_var,
  analysis::Array new_arr_ty(arr_ty->element_type(),
                             arr_ty->GetConstantLengthInfo(length_id, length));
  analysis::Type* reg_new_arr_ty = type_mgr->GetRegisteredType(&new_arr_ty);
-  analysis::Pointer new_ptr_ty(reg_new_arr_ty, SpvStorageClassInput);
+  auto sclass = output_instead_ ? SpvStorageClassOutput : SpvStorageClassInput;


Couldn't the storage class be taken from the source array? should remain the same no?

Keenuts · 2022-11-04T10:29:10Z

source/opt/eliminate_dead_input_components_pass.cpp

+    auto type_id = var->type_id();
+    auto type_inst = def_use_mgr->GetDef(type_id);
+    var->RemoveFromList();
+    var->InsertAfter(type_inst);


If I follow this:

we have a type instruction somewhere in the code, and somewhere down the line, the variable instruction.

we create a new type (resized array or resized struct), which is inserted somewhere, without constraint on location.

we move the variable declaration from it's original stop to after this new type.

Since we are only dealing with input/output variables, I guess they are on the global scope, so their position shouldn't change much, but not 100% sure. Could their position change cause something?
Also, why are we changing the variable declaration position, but not the type instruction to be just below the old type? (Since it's kept until the next DCE pass no?)

I created the new type through the type manager which will make sure that all the type instructions are sequenced correctly. That is the beauty of a type manager. But I still have to make sure the variable instruction is sequenced correctly relative to its type.

The type is the only think we need to worry about here. The only other field in OpVariable with an id is the optional initializer, but input and output don't take initializers.

Keenuts · 2022-11-04T10:39:03Z

include/spirv-tools/optimizer.hpp

+// A subsequent dead code elimination pass would be beneficial in removing
+// newly unused component types.
+//
+// WARNING: This pass can only be safely applied standalone to vertex shaders


Not a fan of this. I understand that the optimizer run() function now only takes one module, and thus this logic must be moved the the user, but could we add a "PipelineOptimizer" for such passes? It would overlay the Optimizer, and make sure those passes are correctly applied to all stages?

Is there cases where one wouldn't want to apply all those passes on some parts of the pipeline? (like you say here, running it only standalone on the vertex stage?)

Not a fan of this. I understand that the optimizer run() function now only takes one module, and thus this logic must be moved the the user, but could we add a "PipelineOptimizer" for such passes? It would overlay the Optimizer, and make sure those passes are correctly applied to all stages?

Yeah, I too would love to one day have a Pipeline Optimizer in spirv-tools. Think of these passes as building blocks and maybe even a blueprint toward that goal. By adding these building blocks now, we are making it that much easier for someone to create such a thing.

In the meantime, these passes will be used by our client to create a similar optimizer in their app. And someone has expressed an intent to investigate using these passes to optimize pipelines in an open-source Vulkan driver.

Is there cases where one wouldn't want to apply all those passes on some parts of the pipeline? (like you say here, running it only standalone on the vertex stage?)

Yes. Actually, my next PR will be to add CreateEliminateDeadInputComponentsSafe(), which will only process vertex shaders. I will add --eliminate-dead-input-components-safe, and will retarget --eliminate-dead-input-components to use the safe version as well.

This pass eliminates components of output variables that are not stored to. Currently this just eliminates trailing components of arrays and structs, all of which are dead. WARNING: This pass is not designed to be a standalone pass as it can cause interface incompatibiliies with the following shader in the pipeline. See the comment in optimizer.hpp for best usage. This pass is currently available only through the API; it is not available in the CLI. This commit also fixes a bug in CreateDecoration() which is part of the system of generating SPIR-V from the Type manager.

greg-lunarg · 2022-11-08T01:12:07Z

@Keenuts I believe I have addressed all your concerns. Can you please take another look?

greg-lunarg force-pushed the edoc2 branch 2 times, most recently from 150dc9e to bac493e Compare November 3, 2022 19:57

greg-lunarg requested a review from Keenuts November 3, 2022 20:15

spencer-lunarg reviewed Nov 4, 2022

View reviewed changes

Keenuts requested changes Nov 4, 2022

View reviewed changes

greg-lunarg force-pushed the edoc2 branch 2 times, most recently from 04b636d to f99c3b7 Compare November 7, 2022 19:25

greg-lunarg force-pushed the edoc2 branch from f99c3b7 to edbc6b6 Compare November 7, 2022 22:29

Keenuts approved these changes Nov 8, 2022

View reviewed changes

greg-lunarg merged commit 525bc38 into KhronosGroup:master Nov 8, 2022

greg-lunarg mentioned this pull request Nov 14, 2022

Rename EliminateDeadInputComponents to EliminateDeadInterfaceComponents #4989

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pass to eliminate dead output components #4982

Add pass to eliminate dead output components #4982

greg-lunarg commented Nov 3, 2022

greg-lunarg commented Nov 3, 2022

spencer-lunarg Nov 4, 2022

greg-lunarg Nov 4, 2022

greg-lunarg Nov 4, 2022 •

edited

Loading

Keenuts Nov 4, 2022

greg-lunarg Nov 4, 2022

Keenuts Nov 4, 2022

greg-lunarg Nov 4, 2022

Keenuts Nov 4, 2022

greg-lunarg Nov 4, 2022

Keenuts Nov 4, 2022

greg-lunarg Nov 4, 2022

Keenuts Nov 4, 2022

greg-lunarg Nov 4, 2022 •

edited

Loading

Keenuts Nov 4, 2022

greg-lunarg Nov 4, 2022

Keenuts Nov 4, 2022

greg-lunarg Nov 4, 2022

Keenuts Nov 4, 2022

greg-lunarg Nov 4, 2022

greg-lunarg commented Nov 8, 2022

Add pass to eliminate dead output components #4982

Add pass to eliminate dead output components #4982

Conversation

greg-lunarg commented Nov 3, 2022

greg-lunarg commented Nov 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-lunarg Nov 4, 2022 • edited Loading

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

greg-lunarg Nov 4, 2022 • edited Loading

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 8, 2022

greg-lunarg Nov 4, 2022 •

edited

Loading

greg-lunarg Nov 4, 2022 •

edited

Loading