Skip to content
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

Add cleanup transformation sorting inputs of commutative operations #84

Open
iksnagreb opened this issue Nov 14, 2023 · 0 comments · May be fixed by #85
Open

Add cleanup transformation sorting inputs of commutative operations #84

iksnagreb opened this issue Nov 14, 2023 · 0 comments · May be fixed by #85
Labels
enhancement New feature or request

Comments

@iksnagreb
Copy link
Contributor

Details

Most graph transformations (here as well as in FINN) assume a particular order of node inputs, even if the operation is actually commutative, e.g. the add operation. There seems to be a clear distinction between "dynamic" (i.e., produced upstream) and initializer inputs. As far as I can tell, the assumption is always that dynamic inputs are listed first, followed by the initializers. Concretely, node.input[0] is used to refer to the (single) dynamic input to node. However, this does not always hold true and I have encountered multiple occasions where valid transformations did not apply or were applied incorrectly due to violations of this assumption: Xilinx/finn#878

New behavior

I propose to add a cleanup transformation to sort the node input list of such commutative operations to have the initializer inputs last. For all other types of transformations, input order already has special meaning and seems to be handled correctly.

Motivation

Ideally, transformations of commutative operations should not care for the input order. But as order is assumed in a lot of places right now, it seems to be easier to introduce a cleanup transformation to ensure the assumptions hold true.

Parts of QONNX being affected

A new cleanup transformation will be introduced and will be added to the default cleanup transformations of the ModelWrapper.

@iksnagreb iksnagreb added the enhancement New feature or request label Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant