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

Make im2col default option for quartus #1010

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

calad0i
Copy link
Contributor

@calad0i calad0i commented May 3, 2024

A# Description

With using the Quartus backend, with conv2d with kernel size 3x3 or conv1d with kernel size 3, the framework makes the default implementation Winograd instad of im2col. As the current Winograd implementation does not seem to play nicely with low bitwidth and hard to achieve bit-accuracy (e.g., with automatic precision setting), I would suggest not enable it by default.

For the Winograd impl, type uint8_t from <cstdint> is used. This header is not included by default in many environments, explicit #include <cstdint> is added to corresponding files.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation update
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • A new research paper code implementation
  • Other (Specify)

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
Copy link
Contributor

jmitrevs commented May 3, 2024

I don't have a strong opinion on this. Let's see what others think.

@jmitrevs jmitrevs added the please test Trigger testing by creating local PR branch label May 3, 2024
@@ -3,6 +3,7 @@

#include "nnet_common.h"
#include "nnet_dense.h"
#include <cstdint>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we use from this header?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This header provides the uint8_t used in that file, and is not included by default in all platforms. Though, I am not sure if using uint8_t provides any performance benefit as that index should probably be optimized out when compiling to hdl...

@vloncar
Copy link
Contributor

vloncar commented May 4, 2024

I'm fine with this type of change. But last I remember, im2col often didn't even synthesize so we added some heuristic to at least try winograd if possible. @bo3z might remember better.

@bo3z
Copy link
Contributor

bo3z commented May 5, 2024

I'm fine with this type of change. But last I remember, im2col often didn't even synthesize so we added some heuristic to at least try winograd if possible. @bo3z might remember better.

Generally, some designs would fail HLS compilation and it just seemed that Winograd was more successful. This could be tied to a specific version of Intel HLS or some implementation detail we had at the time. If I recall correctly, the heuristic was such that if it's 3x3 (and no stride, padding etc.) we set it to use Winograd. But it's well known that Winograd doesn't perform well at extreme quantisations (which granted were never tested at the time).

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.

None yet

4 participants