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

Systolic Array MM and Post Op Separated into Separate Components #1702

Merged
merged 21 commits into from
Sep 1, 2023

Conversation

calebmkim
Copy link
Contributor

@calebmkim calebmkim commented Aug 30, 2023

Apologies for the massive PR, but all it does is separate the systolic array MM component and the post op component into separate component. (One reason why the PR seems so big is because I've refactored the code into separate files and github is treating it as all new code. I also changed all of the systolic array stuff to always use fixed point meaning I had to rewrite a bunch of tests).

The key interface is a r0_valid, r0_value, r0_idx, r1_valid, ... that appears on both a) the output of the systolic array MM component and b) the input of the post-op component.
The main component simply wires these ports together in one big, RTL-like, group. It also instantiates memories, and connects the input memories to the systolic array MM component and the output memories to the post-op component.

Currently, the post-op component simply coordinates the execution of the post-op and not the actual computation of the post-op. For example, the post-op component will instantiate a (separately defined) leaky relu component, and all it does is coordinate when to trigger the leaky relu. Also, the post-op coordination component ends up looking rather RTL-like; one of the key reasons is that the systolic output r0_value is only available for one cycle before it goes onto the next cycle. This means the post-op coordination component needs to be continuously reading from r0_value and saving the value into a register for future reference.

@calebmkim
Copy link
Contributor Author

Also, this can subsume #1693.

# Global constant for the current bitwidth.
BITWIDTH = 32
INTWIDTH = 16
FRACWIDTH = 16
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably make these parameters to the input script at some point.

from calyx import py_ast
from gen_array_component import NAME_SCHEME
from gen_pe import BITWIDTH, INTWIDTH, FRACWIDTH
from fud.stages.verilator import numeric_types
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, weird that the numeric types stuff still resides in the verilator stage. Can you open an issue saying that it should be moved to calyx-py instead?

Add ports "r_{row_num}_valid", "r_{row_num}_value", "r_{row_num}_idx" to comp.
These ports are meant to read from the systolic array output.
"""
comp.input(NAME_SCHEME["systolic valid signal"].format(row_num=row_num), 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This name scheme thing has always been a terrible back and probably unmaintainable in the long-term. It was needed before we had the builder stuff but we can probably get rid of this now. Any thoughts on how hard it would be?

this.idx_reg_write_en = write_mem.out @ 1

# Write to memory.
g.asgn(write_en_port, 1, write_mem.out)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to use asgn here instead of the = stuff?

Copy link
Contributor Author

@calebmkim calebmkim Sep 1, 2023

Choose a reason for hiding this comment

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

The port name depends on an input parameter row_num. So the port's name is this.out_mem_{row_num}_write_en (and I don't think the Python builder supports things like this).

Copy link
Contributor

@rachitnigam rachitnigam left a comment

Choose a reason for hiding this comment

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

Awesome stuff! I think there are a couple of places we can try to clean up the code but I'm going to leave it at your discretion.

@calebmkim
Copy link
Contributor Author

I cleaned up some of the code. For the stuff I didn't get to in this PR I created a tracker (#1712) to keep track of the cleanup.
Going to merge.

@calebmkim calebmkim merged commit ede694a into master Sep 1, 2023
@calebmkim calebmkim deleted the sys-improvemenets branch September 1, 2023 15:21
rachitnigam pushed a commit that referenced this pull request Feb 16, 2024
* working separated relu

* correct implementation of separate relu unit

* removed unnecessary file

* python lint

* some progress on refactoring

* some progress on leaky relu

* working leaky relu post op

* deleted unnec file

* cleaner code

* modified gen-systolic

* small changes

* python formatting

* rewrote tests

* simplified gen-systolic

* simplified code + design

* py format

* rewrote tests

* documentation

* some code cleaning

* code cleaning
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.

2 participants