Skip to content

Add an optimizer to replace SeparableConv by Depthwise + Conv (pointwise) #1022

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Aug 21, 2024

Conversation

jmitrevs
Copy link
Contributor

@jmitrevs jmitrevs commented Jun 13, 2024

Description

When looking at automatic type inference, reuse factor setting, stream buffer optimization, and eventual oneAPI implementation with task sequences, it became clear that treating separable convolutions as two layers instead of one was easier. The different layers can have different accumulator precisions, reuse factors, etc.

This optimizer converts a SeperableConv*D layer to a DepthwiseConv*D layer followed by a Conv*D layer for the pointwise convolution. (For backends that have an explicit pointwise implementation, a subsequent optimizer changes the Conv*D to PointwiseConv*D.) Layer-wise configurations are also created for the new depthwise and pointwise convolutions so that type inference can be done on the individual layers. Hence, this optimizer should be run before the automatic type inference. (The qonnx PR #979 adds a number of other optimizers than also need to run before the type inference, so this will be a common feature.)

In this PR I added parameters but did not remove any. In particular, reuse factor and accumulator type are ambiguous, and unused in the new implemenation, being split between depthwise and pointwise reuse factors and accumulators. However, if this optimizer is disabled, the old scheme should still work, with care by the user.

Decided it was cleaner to remove the unused parameters.

I believe this PR also adds support for multiplier factors other than 1, but it's untested. It was motivated by #1008 .

Multiplier factors other than 1 will come in a separate PR. This implements some parsing but at the end an assert is added for the Vitis/Vivado templates asserting that only a multiplier factor of 1 is currently supported.

Type of change

Updated implementation that

Note: Please delete options that are not relevant.

  • Bug fix (non-breaking change that fixes an issue)
  • Other (Specify) - more maintainable implementation

Tests

This should not cause changes to the standard depth_multiplier=1 separable convolutions not using automatic type inference, so the default tests should be fine. The automatic type inference will be tested in a following PR that makes auto the default.

Checklist

  • I have read the guidelines for contributing.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have installed and run pre-commit on the files I edited or added.
  • I have added tests that prove my fix is effective or that my feature works.

@jmitrevs jmitrevs added this to the v1.0.0 milestone Jun 13, 2024
@jmitrevs jmitrevs added the please test Trigger testing by creating local PR branch label Jun 13, 2024
@jmitrevs jmitrevs marked this pull request as draft June 13, 2024 01:05
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jun 13, 2024
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jun 13, 2024
@jmitrevs jmitrevs marked this pull request as ready for review June 13, 2024 20:24
@jmitrevs
Copy link
Contributor Author

I am somewhat torn as to whether to remove the regular accum_t and reuse_factor, which are not used if the separable is split, which is the default.

@jmitrevs jmitrevs requested a review from vloncar June 25, 2024 21:47
@jmitrevs jmitrevs marked this pull request as draft June 28, 2024 20:00
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jul 2, 2024
@jmitrevs
Copy link
Contributor Author

I decided that it was cleaner to remove the unused attributes.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jul 15, 2024
@jmitrevs jmitrevs marked this pull request as ready for review July 15, 2024 21:27
@jmitrevs jmitrevs removed the please test Trigger testing by creating local PR branch label Jul 15, 2024
@jmitrevs jmitrevs added the please test Trigger testing by creating local PR branch label Jul 15, 2024
@jmitrevs
Copy link
Contributor Author

Comment for reviewers: Note the changes in graph.py for easier passing of the configuration from one layer to child layers after transformations. This is used also in the qonnx PR. It's probably useful to check here.

@jmitrevs
Copy link
Contributor Author

jmitrevs commented Jul 23, 2024

There are some template changes in the Vivado backend from when I thought I would make this PR handle multiplier factors != 1. In particular, I changed n_chan to n_filt in some places, which for multiplier_factor == 1 are identical. In the end I added an assert that the multiplier factor == 1. I did not put those in other backends, though all only work with multiplier factor == 1. I figured that would be taken up by the multiplier_factor != 1 PR.

@jmitrevs jmitrevs requested review from JanFSchulte and bo3z August 20, 2024 22:08
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Aug 20, 2024
@JanFSchulte JanFSchulte merged commit 2898ab2 into fastmachinelearning:main Aug 21, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants