Add ArmPassManager #3749

oscarandersson8218 · 2024-05-27T08:30:09Z

Add ArmPassManager that's responsible for pass handling.
Add RemoveClones-pass.
Add initial pass test.

Change-Id: I71e1e0787d0788a835a412c608ae75331fe65cc2

pytorch-bot · 2024-05-27T08:30:13Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/3749

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

[PREEMPTIVE] CPU amd64 non-GPU instances migration to Linux AMZN2023

❌ 2 New Failures

As of commit a9c550b with merge base 074a81e ():

NEW FAILURES - The following jobs have failed:

pull / unittest / linux / linux-job (gh)
curl: (22) The requested URL returned error:
pull / unittest / macos / macos-job (gh)
RuntimeError: Command bash /Users/ec2-user/runner/_work/_temp/exec_script failed with exit code 137

This comment was automatically generated by Dr. CI and updates every 15 minutes.

digantdesai · 2024-05-28T22:49:40Z

backends/arm/test/passes/test_tag_io_quant_pass.py

+                    permute_memory_to_nhwc=True,
+                    quantize_io=True,


This is one way to do it. But why hide it from the user? I would just run them at the top level i.e.

... e = to_edge() e = helper_run_pass(e, pass_foo) # new method e = partition(my_delegate_partitioner()) e = helper_run_pass(e, pass_bar) ...

In theory delegate specific passes should only run on the "partitioned" subgraph i.e. inside the preprocess() to make sure the graph you are mutating has no external side effect.

This is different, and I understand the rationale, and this is mutating the graph before the partitioning. I am not sure if we want to hide this mutation from the user.

What do you think?

But why hide it from the user?

The reasoning is that different compile_specs may require different passes and applying all the passes at the top level would leave all the responsibility on the end-user. With this pass manager, all the user needs to do is to configure the compile_spec and then the passes would be applied for "free".

On the second part I think it is fine that this specific pass (TagIOQuantPass) is hidden since it does not change the graph, it only makes sure that the delegate does not consume certain nodes. Pre-partition passes that does mutate the graph should probably not be hidden though as you say...

well, you can still do it outside, I am not sure which one is better

e = helper_run_pass(e, pass, compile_spec)

@mergennachin - what do you think?

- Add ArmPassManager that's responsible for pass handling. - Add RemoveClones-pass. - Add initial pass test. Change-Id: I71e1e0787d0788a835a412c608ae75331fe65cc2 Signed-off-by: Oscar Andersson <oscar.andersson@arm.com>

Change-Id: I5e8e868c49b9c4d3b365e2154430a286028d8dfc Signed-off-by: Oscar Andersson <oscar.andersson@arm.com>

digantdesai · 2024-07-02T05:20:40Z

backends/arm/arm_partitioner.py

-                passes(exported_program.graph_module)
+        exported_program = ArmPassManager(
+            exported_program=exported_program
+        ).transform_partition_pipeline(compile_spec=self.delegation_spec.compile_specs)


Perhaps print a warning or something here?

digantdesai · 2024-07-02T05:22:14Z

backends/arm/passes/remove_clone_pass.py

+from executorch.exir.pass_base import PassResult
+
+
+class RemoveClone(ArmPass):


Pass suffix is commonly used

backends/arm/test/passes/test_tag_io_quant_pass.py

facebook-github-bot · 2024-07-02T05:26:33Z

@digantdesai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mergennachin · 2024-07-02T17:22:16Z

backends/arm/arm_partitioner.py

+        graph_module = ArmPassManager().transform_partition_pipeline(
+            graph_module=exported_program.graph_module,
+            compile_spec=self.delegation_spec.compile_specs,
+        )


This is not gonna work.

The invariant is that input and output ofpartitioner() should be the same, except a tag.

https://github.com/pytorch/executorch/blob/main/exir/backend/backend_api.py#L368-L371

As @digantdesai mentioned, this should be a user pass before calling to_backend(ArmPartitioner) for now.

We might introduce a first class citizen API for backend specific passes, but for now, we will ask users to add this one-line call to ArmPassManager()

Functionally nothing has changed since #3056. The tag_io_quant_pass has been moved from one file to another. Also, that pass only annotates a tag.

If I understand you correctly, you simply want to remove this call from the partitioner? You don't want the PassManager to be changed?

Perhaps we can merge this PR, which just sheds light on the structural issues with quantized IO, and continue the more structural discussions in a new Github ticket?

@oscarandersson8218 @robell @mergennachin @digantdesai

- Update README - Suffix passes with Pass Signed-off-by: Oscar Andersson <oscar.andersson@arm.com> Change-Id: I32b5683ea373fe8d0706da1a004eca8f5478de3c

Change-Id: Ib7200c1eeb7ac2646f9177c1931ddac72919fd52

Signed-off-by: Oscar Andersson <oscar.andersson@arm.com> Change-Id: I98f8b68361a05452cf100f67ec4479690bc8699d

Change-Id: I9b628ea6705ce6bc4f95479607594a31f6c52064

Signed-off-by: Oscar Andersson <oscar.andersson@arm.com> Change-Id: I7eabfee97c52cdec81f12986f08b2ed590d5681a

digantdesai · 2024-07-12T21:57:37Z

IIUC - with a9c550b - there is no use of ArmPassManager in Partition fn. While it unblocks this PR, we should still treat the underlying issue as a priority. I will try to follow up on this.

facebook-github-bot · 2024-07-12T22:09:48Z

@digantdesai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-07-15T16:20:29Z

@digantdesai merged this pull request in dd7fa6a.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 27, 2024

oscarandersson8218 added the partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm label May 27, 2024

digantdesai reviewed May 28, 2024

View reviewed changes

oscarandersson8218 requested a review from digantdesai June 7, 2024 05:47

oscarandersson8218 marked this pull request as ready for review June 13, 2024 12:39

oscarandersson8218 mentioned this pull request Jun 17, 2024

Remove memory-format workaround for Arm backend #3981

Closed

oscarandersson8218 added 2 commits June 28, 2024 13:45

Add ArmPassManager

9175a43

- Add ArmPassManager that's responsible for pass handling. - Add RemoveClones-pass. - Add initial pass test. Change-Id: I71e1e0787d0788a835a412c608ae75331fe65cc2 Signed-off-by: Oscar Andersson <oscar.andersson@arm.com>

ArmPassManager fixes

7e7c6be

Change-Id: I5e8e868c49b9c4d3b365e2154430a286028d8dfc Signed-off-by: Oscar Andersson <oscar.andersson@arm.com>

oscarandersson8218 force-pushed the pass_manager branch from 9c1281e to 7e7c6be Compare June 28, 2024 12:08

digantdesai approved these changes Jul 2, 2024

View reviewed changes

mergennachin requested changes Jul 2, 2024

View reviewed changes

oscarandersson8218 added 5 commits July 3, 2024 10:22

Address review comments for ArmPassManager

39d9538

- Update README - Suffix passes with Pass Signed-off-by: Oscar Andersson <oscar.andersson@arm.com> Change-Id: I32b5683ea373fe8d0706da1a004eca8f5478de3c

Merge remote-tracking branch 'origin/upstream/main' into HEAD

ca57e95

Change-Id: Ib7200c1eeb7ac2646f9177c1931ddac72919fd52

Pass quantize_io flag to get_u55_compile_spec

2bde11e

Signed-off-by: Oscar Andersson <oscar.andersson@arm.com> Change-Id: I98f8b68361a05452cf100f67ec4479690bc8699d

Merge remote-tracking branch 'origin/upstream/main' into HEAD

c233860

Change-Id: I9b628ea6705ce6bc4f95479607594a31f6c52064

Restore call to TagIOQuantPass

a9c550b

Signed-off-by: Oscar Andersson <oscar.andersson@arm.com> Change-Id: I7eabfee97c52cdec81f12986f08b2ed590d5681a

oscarandersson8218 added the ciflow/trunk label Jul 12, 2024

digantdesai approved these changes Jul 12, 2024

View reviewed changes

digantdesai requested a review from mergennachin July 12, 2024 21:58

facebook-github-bot closed this in dd7fa6a Jul 15, 2024

facebook-github-bot added the Merged label Jul 15, 2024

		from executorch.exir.pass_base import PassResult


		class RemoveClone(ArmPass):

Add ArmPassManager #3749

Add ArmPassManager #3749

Uh oh!

Conversation

oscarandersson8218 commented May 27, 2024

Uh oh!

pytorch-bot bot commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/3749

❗ 1 Active SEVs

❌ 2 New Failures

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot commented Jul 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

digantdesai commented Jul 12, 2024

Uh oh!

facebook-github-bot commented Jul 12, 2024

Uh oh!

facebook-github-bot commented Jul 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pytorch-bot bot commented May 27, 2024 •

edited

Loading