added check for conv implementation

[jicampos]

Description

This change adds a check in the Vivado backend to skip instruction calculations for CONV1D and CONV2D when using linebuffer implementations. Since linebuffers do not require calculating instructions, min_height, or min_width, performing these calculations is redundant.

Note: Please delete options that are not relevant.

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change which adds functionality)

Tests

Should not break current tests

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]

I will just add that the reason for this change was because in some cases the instructions became huge and made compilation more difficult, even though they were then unused.

[jmitrevs]

It does seem to cause some test failures that should be investigated.

[jmitrevs]

One should maybe make this check case-insensitive.

[bo3z]

Would it make sense to add this check in the match() method rather than the transform() method? That way the optimizer will never even be executed. Also, since this only applies to encoded implementation, should we rename this optimizer to reflect that?

[jmitrevs]

I think "Instructions" implies encoded to me, but maybe not to everyone. But I do like the idea of moving this to the match. There may need to be some changes in the parameter creation (in convolution_templates.py for Vivado and Catapult) to make sure unused variables aren't implicitly required, but that should be fairly easy to fix. Only Vivado and Catapult implement encoded, I believe. Vitis, Quartus, and oneAPI do not, if I remember correctly.

[jicampos]

Would it make sense to add this check in the match() method rather than the transform() method? That way the optimizer will never even be executed. Also, since this only applies to encoded implementation, should we rename this optimizer to reflect that?

We would still need to run the optimizer to add dummy weights in the case of LineBuffer implementations but we can skip generating instructions with the added condition in the if statement.

[jmitrevs]

I think the suggestion from @bo3z is the better approach. Can we change it so that the check is on the match? There may be some downstream minor fixes needed with that approach, though, mainly allowing for undefined instructions and min widths/heights.

[jicampos]

I think the suggestion from @bo3z is the better approach. Can we change it so that the check is on the match? There may be some downstream minor fixes needed with that approach, though, mainly allowing for undefined instructions and min widths/heights.

I've updated the optimizer match() to check the IOType and implementation. The LineBuffer default values are now added to Conv2DConfigTemplate and Conv1DConfigTemplate.

[jmitrevs]

We should wait to make sure all the pytests pass first before merging. (Maybe got ahead of myself approving.)

[jicampos]

We should wait to make sure all the pytests pass first before merging. (Maybe got ahead of myself approving.)

It seems the tests are failing because of a connection issue in the qkeras tests, and is unrelated to the PR.

Before rerunning any tests, should we update the catapult backend with the same changes?

[jmitrevs]

I agree that the test failure is unrelated. If you can add the same to Catpult, that would be great. (No other backend has encoded.)

[jicampos]

I agree that the test failure is unrelated. If you can add the same to Catpult, that would be great. (No other backend has encoded.)

That sounds good, the catapult backend has been updated.

[jmitrevs]

I think these can be replaced with just params['min_witdth'] = node.get_attr('in_width') and similar. Note that params is created on line 82, so params['min_witdth'] is not set. The setdefaults has the same effect as params['min_witdth'] = node.get_attr('in_width'), so it's better to just use the more straightforward form.