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

[VTA] [Chisel] support for different inp/wgt bits, rewrote DotProduct for clarity #3605

Merged
merged 22 commits into from
Jul 26, 2019
Merged

Conversation

BenjaminTu
Copy link
Contributor

Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers.

Copy link
Member

@vegaluisjose vegaluisjose left a comment

Choose a reason for hiding this comment

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

Thanks for your first contribution Ben, let's start iterating on the code.

vta/hardware/chisel/src/main/scala/core/TensorGemm.scala Outdated Show resolved Hide resolved
vta/hardware/chisel/src/main/scala/core/TensorGemm.scala Outdated Show resolved Hide resolved
@BenjaminTu BenjaminTu changed the title support for different inp/wgt bits, rewrote DotProduct for clarity [VTA] [Chisel] support for different inp/wgt bits, rewrote DotProduct for clarity Jul 23, 2019
@vegaluisjose
Copy link
Member

Also, now that we are doing this. I am thinking we can update the name of the Adder module to PipeAdder and the MatrixVectorCore with MatrixVectorMultiplication just to be more explicit on naming.

@tmoreau89
Copy link
Contributor

Thanks @BenjaminTu for this PR! This is looking good

@tmoreau89
Copy link
Contributor

Question: do we want to only implement addition tree, or have an FIR style chain of adders as another option; I believe that the latter is more DSP slice friendly, and easier to place and route due to its regular pattern. However only FPGA synthesis, placement and route will actually tell; it might be a good experiment to try.

Copy link
Contributor

@tmoreau89 tmoreau89 left a comment

Choose a reason for hiding this comment

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

LGTM

@tmoreau89
Copy link
Contributor

@vegaluisjose I notice a certain spacing style that looks like a*b + c, and should instead look like a * b + c as per: https://docs.scala-lang.org/style/method-invocation.html#symbolic-methodsoperators

I think that we should perhaps discuss a plan to put a scala lint test in place to avoid to have to do too many style edits in the future.

Copy link
Member

@vegaluisjose vegaluisjose left a comment

Choose a reason for hiding this comment

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

@vegaluisjose I notice a certain spacing style that looks like a*b + c, and should instead look like a * b + c as per: https://docs.scala-lang.org/style/method-invocation.html#symbolic-methodsoperators

I think that we should perhaps discuss a plan to put a scala lint test in place to avoid to have to do too many style edits in the future.

Yeah I like the idea and we should start working on that.

vta/hardware/chisel/src/main/scala/core/TensorGemm.scala Outdated Show resolved Hide resolved
@vegaluisjose
Copy link
Member

Question: do we want to only implement addition tree, or have an FIR style chain of adders as another option; I believe that the latter is more DSP slice friendly, and easier to place and route due to its regular pattern. However only FPGA synthesis, placement and route will actually tell; it might be a good experiment to try.

We can have both or more, this is one of the advantage of the language. However, in terms of the FIR style chain we would have to make a compromise, because right now we are assuming that the MatrixVectorMultiplication unit can consume operands every cycle. With the FIR style design, if I understood the idea correctly, the operands need to available during the entire pipeline because multiplication and accumulation happens at every stage in the pipeline. The benefit of this style as you said is that you will end up with blockOut * blockIn DSP but at the expense of performance hit because operands has to available during the entire computation.

This was actually how I started the design. The Baseline bar in the following figure show the cycles spent by VTA using this approach (FIR style), the other two are using the current style.

@tmoreau89 tmoreau89 merged commit 87e18a4 into apache:master Jul 26, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request Aug 9, 2019
… for clarity (apache#3605)

* support for different inp/wgt bits, rewrote dot for clarity

* [VTA] [Chisel] support for different inp/wgt bits, rewrote DotProduct for clarity

* [VTA] [Chisel] support for different inp/wgt bits, rewrote DotProduct for clarity

* change back to sim

* fix index

* fix index

* fix indent

* fix indent

* fix indent

* fix trailing spaces

* fix trailing spaces

* change to more descriptive name

* matric->matrix

* fix spacing

* fix spacing & added generic name for dot

* better parameter flow

* spacing

* spacing

* spacing

* update requirement (tested) for dot, spacing

* function call convention

* small edit
wweic pushed a commit to neo-ai/tvm that referenced this pull request Sep 6, 2019
… for clarity (apache#3605)

* support for different inp/wgt bits, rewrote dot for clarity

* [VTA] [Chisel] support for different inp/wgt bits, rewrote DotProduct for clarity

* [VTA] [Chisel] support for different inp/wgt bits, rewrote DotProduct for clarity

* change back to sim

* fix index

* fix index

* fix indent

* fix indent

* fix indent

* fix trailing spaces

* fix trailing spaces

* change to more descriptive name

* matric->matrix

* fix spacing

* fix spacing & added generic name for dot

* better parameter flow

* spacing

* spacing

* spacing

* update requirement (tested) for dot, spacing

* function call convention

* small edit
tqchen pushed a commit to tqchen/tvm that referenced this pull request Mar 29, 2020
… for clarity (apache#3605)

* support for different inp/wgt bits, rewrote dot for clarity

* [VTA] [Chisel] support for different inp/wgt bits, rewrote DotProduct for clarity

* [VTA] [Chisel] support for different inp/wgt bits, rewrote DotProduct for clarity

* change back to sim

* fix index

* fix index

* fix indent

* fix indent

* fix indent

* fix trailing spaces

* fix trailing spaces

* change to more descriptive name

* matric->matrix

* fix spacing

* fix spacing & added generic name for dot

* better parameter flow

* spacing

* spacing

* spacing

* update requirement (tested) for dot, spacing

* function call convention

* small edit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants