Common Subgraph Elimination #5752

mzient · 2024-12-16T13:48:04Z

Category:

New feature (non-breaking change which adds functionality)

Description:

This change introduces a graph-level optimization that removes common subgraphs.
The algorithm goes over graph nodes in topological order and computes a key from the operator's schema name, arguments and (renamed) inputs. If the operator is found, it's replaced with the previously found one and the output names are added to the renaming map. When subsequent operators are processed, the inputs of the replaced operator are renamed accordingly.
Example:

A = foo(arg=42)  # new
B = foo(arg=42)  # replaced with A
C = bar(A) # new
D = bar(B) # replaced with bar(A)
return C + D  # replaced with C + C

This optimization is particularly useful with constants, the use of .shape function and subscripts.
In the following example:

horz_center = (bbox[0] + bbox[2]) / 2
width = (bbox[2]  - bbox[0])

The bbox[0] and bbox[2] will be reused.

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Some Python tests needed to be adjusted.

Checklist

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: DALI-4159

Signed-off-by: Michal Zientkiewicz <[email protected]>

…ons. Signed-off-by: Michał Zientkiewicz <[email protected]>

mzient · 2024-12-16T13:50:40Z

dali/pipeline/operator/argument.cc

@@ -41,7 +41,8 @@ inline std::shared_ptr<Argument> DeserializeProtobufVectorImpl(const DaliProtoPr
  auto args = arg.extra_args();
  std::vector<T> ret_val;
  for (auto& a : args) {
-    const T& elem = DeserializeProtobuf(a)->Get<T>();


After changing Argument::Get to returning a reference, this became a bug: returning a reference to a field of the result of DeserializeProtobuf - since we get a temporary shared_ptr, it would be destroyed (along with the referenced field!) before the field reference could be used.

mzient · 2024-12-16T13:51:33Z

dali/test/python/test_pipeline.py

+            ref = pipe_out_cpu
+        else:
+            check_batch(pipe_out_cpu, ref)


This test checked too little and took too long - now it's more accurate and much faster.

dali-automaton · 2024-12-16T13:53:05Z

CI MESSAGE: [21516079]: BUILD STARTED

- restore _module and _display name in the default schema - add preserve(_name) in Pipeline tests where CSE is unwanted - fix and extend Python tests - extend documentation Signed-off-by: Michał Zientkiewicz <[email protected]>

Signed-off-by: Michał Zientkiewicz <[email protected]>

dali-automaton · 2024-12-16T16:04:46Z

CI MESSAGE: [21520256]: BUILD STARTED

dali-automaton · 2024-12-17T08:01:19Z

CI MESSAGE: [21520256]: BUILD PASSED

Signed-off-by: Michal Zientkiewicz <[email protected]>

dali-automaton · 2024-12-17T18:10:21Z

CI MESSAGE: [21574171]: BUILD STARTED

dali-automaton · 2024-12-18T07:45:57Z

CI MESSAGE: [21574171]: BUILD PASSED

banasraf · 2024-12-18T09:35:02Z

dali/pipeline/graph/cse.cc

+class CSE {
+ public:
+  void Run(OpGraph &graph) {
+    for (auto &node : graph.OpNodes())


OpNodes returns the nodes in topological order?

Yes, the graph is sorted by the Builder.

Signed-off-by: Michal Zientkiewicz <[email protected]>

dali-automaton · 2024-12-18T12:13:07Z

CI MESSAGE: [21610648]: BUILD STARTED

szkarpinski · 2024-12-18T12:41:11Z

dali/pipeline/operator/op_schema.h

+  bool per_frame  = false;  //< The (tensor) argument can be expanded to multiple frames
+  bool internal   = false;  //< The argument cannot be set by the user in Python
+  bool hidden     = false;  //< The argument doesn't appear in the documentation
+  bool ignore_cmp = false;  //< Two operators can be considered equal if this argument differs


Nitpick

Suggested change

bool ignore_cmp = false; //< Two operators can be considered equal if this argument differs

bool ignore_cmp = false; //< Two operators can be considered equal even if this argument differs

szkarpinski · 2024-12-18T12:45:37Z

dali/pipeline/operator/op_schema.cc

+  AddInternalArg("preserve_name", R"(When true, the operator cannot be renamed.
+This disables merging this operator with another one with a different name.)",
+                 false);


Shouldn't this be the case always when an operator was explicitly named by the user?

I understand that having two equal ops (Readers?) but with different names is not a clever way to use DALI, but it is possible for the user to do that. Let's say that for whatever reason I have two readers doing the same work, but named differently ("Reader1", "Reader2"). I wrapped the pipeline into something (an iterator?) that relies on a presence of "Reader2" in the pipeline. This is not a good code, but it works. And after CSE it doesn't

That's exactly how it works. If you specify a name manually, then preserve_name is set to True.

Ah, I see:

self._spec.AddArg("preserve_name", not self._autoname)

Thank you!

szkarpinski · 2024-12-18T12:50:44Z

dali/test/python/test_pipeline.py

@@ -2333,3 +2332,61 @@ def def_ref():
        (ref,) = ref_pipe.run()
        check_batch(cpu, ref, bs, 0, 0, "HWC")
        check_batch(gpu, ref, bs, 0, 0, "HWC")
+
+
+def test_cse():


How about adding a test with External Source (to make sure it's not eliminated) and manually named operators?

Manually named operator: see line 2343.
ES - we already have tests with multiple external sources and they did fail before adding the fix, so I'd call it covered.

dali-automaton · 2024-12-19T03:14:23Z

CI MESSAGE: [21610648]: BUILD PASSED

mzient and others added 6 commits December 16, 2024 14:49

[WIP]

bb78a3c

Signed-off-by: Michal Zientkiewicz <[email protected]>

[WIP]

05050b0

Signed-off-by: Michal Zientkiewicz <[email protected]>

Something's terribly broken

1168e6d

Signed-off-by: Michal Zientkiewicz <[email protected]>

WIP

ade0416

Signed-off-by: Michal Zientkiewicz <[email protected]>

Almost working. TODO: external source, protobuf WTF.

cb256d3

Signed-off-by: Michal Zientkiewicz <[email protected]>

Use protobuf for operator comparison. Fix bugs. Remove compare functi…

2c5b91a

…ons. Signed-off-by: Michał Zientkiewicz <[email protected]>

mzient force-pushed the CSE branch from d945331 to 2c5b91a Compare December 16, 2024 13:49

mzient commented Dec 16, 2024

View reviewed changes

mzient added 3 commits December 16, 2024 16:58

Assorted fixes:

4aaa045

- restore _module and _display name in the default schema - add preserve(_name) in Pipeline tests where CSE is unwanted - fix and extend Python tests - extend documentation Signed-off-by: Michał Zientkiewicz <[email protected]>

Reorder arguments.

327cfe8

Signed-off-by: Michał Zientkiewicz <[email protected]>

Remove unnecessary change.

cd937b2

Signed-off-by: Michał Zientkiewicz <[email protected]>

dali-automaton assigned banasraf and szkarpinski Dec 17, 2024

Documentation.

a332acf

Signed-off-by: Michal Zientkiewicz <[email protected]>

banasraf reviewed Dec 18, 2024

View reviewed changes

Add environment variables to disable graph optimization.

03bde04

Signed-off-by: Michal Zientkiewicz <[email protected]>

szkarpinski reviewed Dec 18, 2024

View reviewed changes

banasraf approved these changes Dec 18, 2024

View reviewed changes

szkarpinski approved these changes Dec 18, 2024

View reviewed changes

mzient merged commit 785b2ed into NVIDIA:main Dec 18, 2024
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common Subgraph Elimination #5752

Common Subgraph Elimination #5752

mzient commented Dec 16, 2024 •

edited

Loading

mzient Dec 16, 2024 •

edited

Loading

mzient Dec 16, 2024

dali-automaton commented Dec 16, 2024

dali-automaton commented Dec 16, 2024

dali-automaton commented Dec 17, 2024

dali-automaton commented Dec 17, 2024

dali-automaton commented Dec 18, 2024

banasraf Dec 18, 2024

mzient Dec 18, 2024

dali-automaton commented Dec 18, 2024

szkarpinski Dec 18, 2024

szkarpinski Dec 18, 2024

mzient Dec 18, 2024

szkarpinski Dec 18, 2024

szkarpinski Dec 18, 2024

mzient Dec 18, 2024

dali-automaton commented Dec 19, 2024

	bool ignore_cmp = false; //< Two operators can be considered equal if this argument differs
	bool ignore_cmp = false; //< Two operators can be considered equal even if this argument differs

Common Subgraph Elimination #5752

Common Subgraph Elimination #5752

Conversation

mzient commented Dec 16, 2024 • edited Loading

Category:

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Checklist

Documentation

DALI team only

Requirements

mzient Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Dec 16, 2024

dali-automaton commented Dec 16, 2024

dali-automaton commented Dec 17, 2024

dali-automaton commented Dec 17, 2024

dali-automaton commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Dec 18, 2024

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

dali-automaton commented Dec 19, 2024

mzient commented Dec 16, 2024 •

edited

Loading

mzient Dec 16, 2024 •

edited

Loading