Add optimization patterns fusing ops into custom ops #1636

xadupre · 2024-06-19T11:06:27Z

No description provided.

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

codecov · 2024-06-19T11:10:10Z

Codecov Report

Attention: Patch coverage is 84.83607% with 111 lines in your changes missing coverage. Please review.

Project coverage is 74.53%. Comparing base (c06e7ab) to head (3b4d700).

Files	Patch %	Lines
...nxscript/rewriter/custom_ops/llm_rule_sets_cuda.py	80.63%	42 Missing and 25 partials ⚠️
...ipt/rewriter/custom_ops/llm_rule_sets_cuda_test.py	90.10%	29 Missing and 8 partials ⚠️
onnxscript/rewriter/generic_pattern.py	30.00%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1636      +/-   ##
==========================================
- Coverage   74.88%   74.53%   -0.36%     
==========================================
  Files         242      245       +3     
  Lines       25869    26592     +723     
  Branches     4665     4827     +162     
==========================================
+ Hits        19373    19821     +448     
- Misses       5627     5851     +224     
- Partials      869      920      +51

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ustom

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

…ustom

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

…ustom

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

onnxscript/rewriter/generic_pattern.py

onnxscript/rewriter/custom_ops/llm_rule_sets_cuda.py

Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

justinchuby · 2024-06-24T15:29:39Z

Helpful to create a Pr description? Thanks! https://google.github.io/eng-practices/review/developer/cl-descriptions.html

gramalingam · 2024-06-26T16:34:51Z

onnxscript/rewriter/generic_pattern.py

+            pattern_value.producer() is not None for pattern_value in pattern_node.inputs
+        )
+
+        if has_predecessor_in_pattern:


Is this condition meant to be an optimization (with no effect in behavior)? Trying to understand

I think this fix might be related to an issue Jingyan is running into. Does this fix allow pattern-inputs to have other uses outside of the pattern?

I think so. I think we should change this slightly, by applying the condition for each pattern_value separately, as mentioned in comment below.

Fill free to rewrite the code. If the unit test are passing, it means it is good.

onnxscript/rewriter/custom_ops/llm_rule_sets_cuda.py

gramalingam · 2024-06-26T16:46:52Z

onnxscript/rewriter/onnxruntime/fused_matmul_rule_sets.py

-            orp.make_rewrite_rule_from_class(TransposeFusedMatMul1, True),
-            orp.make_rewrite_rule_from_class(TransposeMatMul2, True),
-            orp.make_rewrite_rule_from_class(TransposeFusedMatMul2, True),
+            orp.make_rewrite_rule_from_class(FusedMatMulDiv1, generic=True),


Trying to understand why you have this generic=True? Is it for debugging?

No it is using the generic pattern matcher (multi output pattern)

If the pattern has multiple outputs, the implementation should automatically choose the generic matcher ... user doesn't have to specify it.

gramalingam · 2024-07-02T22:36:36Z

onnxscript/rewriter/generic_pattern.py

+
+        if has_predecessor_in_pattern:
+            for graph_input, pattern_input in zip(graph_node.inputs, pattern_node.inputs):
+                if len(graph_input.uses()) != len(pattern_input.uses()):


Instead of the has_predecessor_in_pattern check above, I think we should do the check here: add if pattern_input.producer() is not None here ... that will handle the case where some inputs have producer and others don't.

gramalingam · 2024-07-02T22:38:53Z

onnxscript/rewriter/generic_pattern.py

            pattern_pred = pattern_value.producer()
+            if pattern_pred is not None and len(graph_value.uses()) != len(


I am confused ... isn't this already handled above? Why this check on the number of uses again?

The rest of the loop can be executed for the first input if it satisfies the conditions and there is a case where it should not be done.

Fill free to find a better solution. The unit test should catch any weird behaviour.

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

onnxscript/tools/transformers_models/__init__.py

@@ -41,6 +41,7 @@
            prog = torch.onnx.export(model, args, dynamo=True)  # pylint: disable=no-value-for-parameter
        else:
            prog = torch.onnx.dynamo_export(model, *args)
+


onnxscript/tools/transformers_models/__init__.py

@@ -41,6 +41,7 @@
            prog = torch.onnx.export(model, args, dynamo=True)  # pylint: disable=no-value-for-parameter
        else:
            prog = torch.onnx.dynamo_export(model, *args)
+


…ttern matcher (#1731) The multi-output pattern does not support a useful case, where the pattern's input variables are reused elsewhere. This PR adds a test-case illustrating the scenario. I was hoping that some changes in the in-progress PR #1636 by Xavier might fix this. But I don't think it does. Adding the test-case for now. Need to figure out how to support this. --------- Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>

Add custom ops patterns

b149002

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

xadupre added 7 commits June 19, 2024 14:25

Merge branch 'main' of https://github.com/microsoft/onnxscript into c…

9b5617e

…ustom

rename

a52695d

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

complete patterns

d926354

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

Merge branch 'main' of https://github.com/microsoft/onnxscript into c…

8207f79

…ustom

fix custom pattern

ae4385e

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

fix a bug

2855d9d

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

Merge branch 'main' of https://github.com/microsoft/onnxscript into c…

7f95544

…ustom

xadupre mentioned this pull request Jun 21, 2024

[Broken CI] test_llama_p0_rule_set_slice_split fails #1642

Open

bug

afb88d7

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

xadupre marked this pull request as ready for review June 21, 2024 09:12

one fix

a19396d

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

xadupre self-assigned this Jun 21, 2024

justinchuby reviewed Jun 21, 2024

View reviewed changes

onnxscript/rewriter/generic_pattern.py Outdated Show resolved Hide resolved

justinchuby reviewed Jun 21, 2024

View reviewed changes

onnxscript/rewriter/generic_pattern.py Outdated Show resolved Hide resolved

justinchuby reviewed Jun 21, 2024

View reviewed changes

onnxscript/rewriter/custom_ops/llm_rule_sets_cuda.py Show resolved Hide resolved

justinchuby reviewed Jun 21, 2024

View reviewed changes

onnxscript/rewriter/custom_ops/llm_rule_sets_cuda.py Outdated Show resolved Hide resolved

xadupre and others added 5 commits June 24, 2024 10:04

Update onnxscript/rewriter/generic_pattern.py

1e04d54

Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>

Update onnxscript/rewriter/generic_pattern.py

4c831bb

Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>

comments

4d683dc

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

deprecation

f490f2b

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

change

50accea

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

gramalingam reviewed Jun 26, 2024

View reviewed changes

onnxscript/rewriter/custom_ops/llm_rule_sets_cuda.py Show resolved Hide resolved

gramalingam reviewed Jun 26, 2024

View reviewed changes

gramalingam reviewed Jul 2, 2024

View reviewed changes

update

0152dc9

Signed-off-by: Xavier Dupre <xadupre@microsoft.com>

github-advanced-security bot found potential problems Jul 3, 2024

View reviewed changes

gramalingam approved these changes Jul 3, 2024

View reviewed changes

gramalingam and others added 4 commits July 3, 2024 09:26

Merge branch 'main' into custom

9d40708

Merge branch 'main' into custom

1dd97df

Merge branch 'main' into custom

ee30f74

Merge branch 'main' into custom

3b4d700

gramalingam mentioned this pull request Jul 16, 2024

Add test case for unsupported input-variable reuse in multi-output pattern matcher #1731

Merged

justinchuby marked this pull request as draft September 17, 2024 00:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optimization patterns fusing ops into custom ops #1636

Add optimization patterns fusing ops into custom ops #1636

xadupre commented Jun 19, 2024

codecov bot commented Jun 19, 2024 •

edited

Loading

justinchuby commented Jun 24, 2024

gramalingam Jun 26, 2024

gramalingam Jul 2, 2024

gramalingam Jul 2, 2024

xadupre Jul 3, 2024

gramalingam Jun 26, 2024

xadupre Jun 26, 2024

gramalingam Jul 2, 2024

gramalingam Jul 2, 2024

xadupre Jul 3, 2024

gramalingam Jul 2, 2024

xadupre Jul 3, 2024

xadupre Jul 3, 2024

		pattern_pred = pattern_value.producer()
		if pattern_pred is not None and len(graph_value.uses()) != len(

Add optimization patterns fusing ops into custom ops #1636

Are you sure you want to change the base?

Add optimization patterns fusing ops into custom ops #1636

Conversation

xadupre commented Jun 19, 2024

codecov bot commented Jun 19, 2024 • edited Loading

Codecov Report

justinchuby commented Jun 24, 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

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

codecov bot commented Jun 19, 2024 •

edited

Loading