Skip to content

Conversation

AdrianLundell
Copy link
Collaborator

@AdrianLundell AdrianLundell commented Mar 6, 2025

These passes both removes redundant ops from the graph:

  • FuseViewCopyTransform pass is added from backends/transforms to merge sequential view ops.
  • FuseConstantOpsPass is created to compute ops with constant inputs AOT.
    • This is not done in cases where the result is a larger tensor, to avoid increasing the constant memory size.
    • For BI, ops are quantized with the q/dq-ops as to not change the behaviour of the graph.
    • Pass order is important: the pass must be placed after all passes which may add constant ops, but before the
      InsertTableOpsPass, since it doesn't handle TOSA _table-ops.

cc @digantdesai @freddan80 @per @zingo @oscarandersson8218

These passes both removes redundant ops from the graph:
- FuseViewCopyTransform pass is added from backends/transforms to merge
  sequential view ops.
- FuseConstantOpsPass is created to compute ops with constant inputs AOT
  - This is not done in cases where the result is a larger tensor, to
    avoid increasing the constant memory size.
  - For BI, ops are quantized with the q/dq-ops as to not change the
    behaviour of the graph.
  - Pass order is important: the pass must be placed after all passes
    which may add constant ops, but before the InsertTableOpsPass, since
    it doesn't handle TOSA _table-ops.

Change-Id: I855b2cd969dce24ad6d3c21d9a3f5473ddc984b8
Signed-off-by: Adrian Lundell <adrian.lundell@arm.com>
@AdrianLundell AdrianLundell added partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm ciflow/trunk topic: not user facing labels Mar 6, 2025
Copy link

pytorch-bot bot commented Mar 6, 2025

🔗 Helpful Links

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

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

✅ No Failures

As of commit 9d82c07 with merge base d3cca89 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@facebook-github-bot 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 Mar 6, 2025
@zingo zingo merged commit 3a7c231 into pytorch:main Mar 6, 2025
124 of 125 checks passed
zonglinpeng pushed a commit that referenced this pull request Mar 6, 2025
…ass_manager (#8997)

Add FuseViewCopyTransform and FuseConstantsPass in arm_pass_manager

These passes both removes redundant ops from the graph:
- FuseViewCopyTransform pass is added from backends/transforms to merge
  sequential view ops.
- FuseConstantOpsPass is created to compute ops with constant inputs AOT
  - This is not done in cases where the result is a larger tensor, to
    avoid increasing the constant memory size.
  - For BI, ops are quantized with the q/dq-ops as to not change the
    behaviour of the graph.
  - Pass order is important: the pass must be placed after all passes
    which may add constant ops, but before the InsertTableOpsPass, since
    it doesn't handle TOSA _table-ops.

Signed-off-by: Adrian Lundell <adrian.lundell@arm.com>
@kirklandsign
Copy link
Contributor

Sorry @zingo would you mind if we revert and land this again from internal? This is breaking our internal tests.

cc @zonglinpeng do you think a forward fix is possible?

@zonglinpeng
Copy link
Contributor

zonglinpeng commented Mar 8, 2025

Sorry @zingo would you mind if we revert and land this again from internal? This is breaking our internal tests.

cc @zonglinpeng do you think a forward fix is possible?

I'm not familiar with arm passes. But based on the error message ModuleNotFoundError: No module named 'executorch.backends.transforms.fuse_view_copy, fuse_view_copy needs to added or change reference as the fix.

@zingo
Copy link
Collaborator

zingo commented Mar 8, 2025

Of course, if there is a problem let's revert so it works again, and we can figure out the problem much calmer. Unfortunately I'm not at my desk right now so I cannot do it right now. Feel free to go ahead if you can. 🙏

@zingo
Copy link
Collaborator

zingo commented Mar 8, 2025

Created #9070 if still needed
I cant self review it so please feel free to review and merge if this is still an issue.

@kirklandsign
Copy link
Contributor

Hi @zingo

We have the following internal error log:

        debug_mode = "ALL" if logger.level <= logging.DEBUG else None
>       outputs_np, status = tosa_reference_model.run(
            graph,
            inputs_np,
            verbosity=_tosa_refmodel_loglevel(logger.level),
            tosa_profile=tosa_profile,
            initialize_variable_tensor_from_numpy=1,  # True
            debug_mode=debug_mode,
        )
E       AttributeError: 'NoneType' object has no attribute 'run'

Do you have idea?

@kirklandsign
Copy link
Contributor

@AdrianLundell
Copy link
Collaborator Author

Hi @kirklandsign , can you give more context here:

  1. The first error indicates that the tosa_reference_model is none, but it's impossible to say why without knowing how it was created.
  2. Where is the unittest failing, as they passed when running on the PR?
    Thanks!

@kirklandsign
Copy link
Contributor

Hi @AdrianLundell

Sorry I don't have the full context about how the CI is run on our internal CI. Maybe @digantdesai will give more context.

I'm not sure why tosa_reference_model is none here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm topic: not user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants