-
Notifications
You must be signed in to change notification settings - Fork 53
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
[relay] Simulating lenet
, add Relay to fud
, other bug fixes.
#504
Conversation
Blocked on Dahlia #372. |
@cgyurgyik The Dahlia blocker has been fixed? Do you need to do anything else before we review? |
Nope. As usual, apologies for PR size. Most of it should be extra tests for cases that failed earlier. The slight variation to the expected |
Cool. Can I get help from some subset of @sgpthomas @EclecticGriffin and/or @sampsyo reviewing this? |
I can take a look later today (early evening) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general, I threw in a few comments here and there, but I don't have an amazing handle on relay and the existing infrastructure so don't take anything I say too seriously.
Think this is good to go pending approval from someone other than me!
let __max :{data_type} = {data.id.name}[0][0]; | ||
for (let __i: ubit<{index_size0}> = 0..{size0}) {{ | ||
for (let __j: ubit<{index_size1}> = 0..{size1}) {{ | ||
if ({data.id.name}[__i][__j] > __max) {{ __max := {data.id.name}[__i][__j]; }} | ||
}} | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A general (and unhelpful) comment, but this is a good example of what I'm thinking so I'll leave it here.
I get nervous when we have situations where there are load-bearing strings and am wondering if at some point it would be possible to refactor into a nice set of constructors. Though that would likely require having either an internal representation of Dahlia or a Dahlia python library. Obviously this isn't actionable in the short term nor in the scope of this PR, so please don't worry about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same page. The entire dahlia_impl.py
file hurts my eyes; unfortunately I am also the one to blame.
For context, when we first started this in the fall, our goal was to simulate a net using Calyx. In an ideal world, all of our Relay call nodes would be implemented in Verilog or Calyx... hopefully one day that can actually be the case. For now, we decided using Dahlia to implement the function and then lower it to Calyx would be a more efficient process.
While outside the scope of this review, the next question is what are the steps to make this easier to maintain and read. Possibilities:
- Python AST for Dahlia. Perhaps a good intermediary step, but still keeps Dahlia in the picture (part of the reason I've been hesitant to make any significant changes to this file for now).
- Use Python scripts to generate the correct Calyx component for each Relay call node (similar to the
exp
generator). Adrian mentions a cleaner way to do this here. - SystemVerilog implementations of the Relay call nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! If there are new python libraries needed to get the ONNX net stuff working, we should document that.
Added to the Relay frontend docs. |
Lenet simulation
Given that our softmax output is producing negative numbers, there is still something wrong. I presume it is partly due to the fact that our current
exp
operator computese^-x
as1/e^x
. Tracking in #505.Script to run ONNX models
More generally, I've written a script that takes in:
lenet
"mnist", "imagenet"
.(1)
tvm
, Running the Relay program on the TVM executor and printing the finalsoftmax
array.(2)
calyx
, Writing the Calyx program and data to files.(3)
relay
, Writing the Relay program to a file.Example:
Other updates
nn.softmax
by subtracting the max value first. As mentioned above, theexp
operator still needs to be revamped.batch_flatten
.reshape
(necessary for lenet), anddropout
for future use.relay
tofud
.round_float_to_fixed
flag to theverilog
stage that is currently defaulted to True. This flag essentially says any input from.data
files will automatically be rounded to the nearest fixed point number.