Refactor Verilog pass to use verilogAST #770
Conversation
|
There's also more opportunities to improve the output verilog (e.g. wire up outputs of the module to instance ports when they can be bulk connected). We can do this in the verilog code generation logic, but we can also do this as passes on the AST (e.g. remove intermediate wires). I think it will be more robust and extensible to do this as AST passes, so I'd vote to "freeze" the verilog code generator as is in terms of output code quality (we can/should still fix bugs in the functionality of the produced code), and move logic to improve the code quality to be part of the verilogAST library (I'm thinking first passes will be module inlining for primitives and wire collapsing logic to remove intermediate/unnecessary wires). |
leonardt
changed the title
|
Also, this updates the requirements to gcc-7 which is for verilogAST. To do this, I also had to update the catch.hpp unit test framework. This didn't seem to cause any issues with the existing code/tests, but may have an impact on users. |
|
Reviewing now. In general I am not a big fan of some of the style. I know some of the repo is not completely consistent, but it would be good to start especially with such a large change. In particular I would prefer: |
|
@rdaly525 can you point to an example of each thing you're talking about? 2 space indents are for proper "block" indents (while loops, for loops, function body, class body, etc.). 4 space indents should be used for line-break indents. Both LLVM and Google C++ style are in agreement on this, and the |
There was a problem hiding this comment.
The content of the code looks good as far as I can tell. I think it was pretty close to some of the original translation code I had but actually using a legitimate AST! This should make verilog backend much nicer. I think the next step is to make verilog code a first class citizen of CoreIR and maybe attaching the verilogAST definitions directly to coreIR modules instead of the hacky metadata way.
Some issues:
-You should never really need to call getKind() anywhere. the isa<>, dyn_cast<>, and cast<> should cover most cases. For direction, the functions isInput(), isOutput() and isInOut() should suffice.
-Some of the files were using 4 spaces instead of 2 spaces. A lot of the multiline assignments would become single line when using 2 spaces.
-style which I described in a previous comment.
src/passes/analysis/verilog.cpp
Outdated
| this->vmods._verilator_debug = true; | ||
| } | ||
| void Passes::Verilog::initialize(int argc, char **argv) { | ||
| cxxopts::Options options("verilog", "translates coreir graph to verilog"); |
src/passes/analysis/verilog.cpp
Outdated
| } | ||
| void Passes::Verilog::initialize(int argc, char **argv) { | ||
| cxxopts::Options options("verilog", "translates coreir graph to verilog"); | ||
| options.add_options()("i,inline", "Inline verilog modules if possible")( |
There was a problem hiding this comment.
Please change this back to original style
src/passes/analysis/verilog.cpp
Outdated
| std::unique_ptr<vAST::Expression> convert_value(Value *value) { | ||
| if (auto arg_value = dyn_cast<Arg>(value)) { | ||
| return std::make_unique<vAST::Identifier>(arg_value->getField()); | ||
| } else if (auto int_value = dyn_cast<ConstInt>(value)) { |
src/passes/analysis/verilog.cpp
Outdated
| // `Vector` node if the signal is of type Array | ||
| std::variant<std::unique_ptr<vAST::Identifier>, std::unique_ptr<vAST::Vector>> | ||
| process_decl(std::unique_ptr<vAST::Identifier> id, Type *type) { | ||
| if (type->getKind() == Type::TK_Array) { |
There was a problem hiding this comment.
You should use if (ArrayType* array_type = dyn_cast<ArrayType>(type)) {
In general getKind() should not be used much. dyn_cast<> and isa<> should be used instead.
See the doc/ProgrammingGuide.md
src/passes/analysis/verilog.cpp
Outdated
| // bit), iteratively because perhaps you can name and named type? | ||
| // This is just a safety check for internal code, to improve performance, | ||
| // we could guard this assert logic behind a macro | ||
| while (type->getKind() == Type::TK_Named) { |
There was a problem hiding this comment.
I am thinking about getting rid of named types anyways. That level of typing should probably be done at the magma level
| // the output ports of each instance. We return a vector of a variant type for | ||
| // compatibility with the module AST node constructor, even though we only ever | ||
| // create Wire nodes. | ||
| std::vector<std::variant<std::unique_ptr<vAST::StructuralStatement>, |
There was a problem hiding this comment.
This templating is difficult to parse. I would prefer the arguments to variant to each be on their own line.
src/passes/analysis/verilog.cpp
Outdated
| for (auto port : | ||
| cast<RecordType>(instance.second->getModuleRef()->getType()) | ||
| ->getRecord()) { | ||
| if (port.second->getDir() == Type::DK_Out) { |
src/passes/analysis/verilog.cpp
Outdated
| // **TODO** Need to add support for inouts | ||
| std::map<ConnMapKey, std::vector<ConnMapEntry>> build_connection_map( | ||
| std::set<Connection, ConnectionCompFast> connections, | ||
| std::map<std::string, Instance *> instances) { |
There was a problem hiding this comment.
Please put closing parenthesis on new line
|
Also I suspect the C++17 change will break almost everything using CoreIR right now. Before we merge this to master, it might be prudent to add a buildkite with all our tools |
|
Also in terms of the catch.hpp, I think this was from https://github.com/catchorg/Catch2. It says that it supports C++17, so we should probably just add that as another external dependency. |
|
FWIW the style provided in these changes are completely automatically formatted by clang-format, but I'm okay with making the changes you suggested to make the code consistent with the repo. I personally don't care much about these details and prefer automation but that's mainly because it avoids extra effort on my part. Will also address the cast/direction check issues. |
|
I think it's better to have new code follow an automated and standard format then retro-fit new code to the non-uniform standard in the code base. Makes it more likely to improve the code quality overtime. |
|
I would be willing to go with this style. It hits most of my preferences and we can use clang-format with it. |
|
Okay, I merged that branch into this and ran |
|
Removed the explicit Kind logic and changed it to use |
|
Merging |
Finally ready for review, I'd mainly first like to get comments on code design/quality (e.g. refactor repeated things, encapsulate that into an object, etc..). This code passes the CoreIR test suite, but I'm a little worried about code paths that haven't been captured by our tests (e.g. the garnet/lassen flow). We should make sure those work before merging this into master. We could also merge this into dev, but as of creating this PR, it seems that dev is behind master so when I target dev, the diff includes commits that aren't a part of this PR, so I'll leave it as master now.
There are some limitations, but I think it's important to get this merged ASAP so we avoid future integration pains. Inlining doesn't work yet, but my goal is to have the functionality provided as generic AST passes by verilogAST, so that work will mostly be done in a separate code path (the backend will just need to invoke those passes). Thus, I've disabled the inlining tests for now (I don't think they were working on travis anyways) since this feature shouldn't block users and I'd rather not block this PR for that.
Another limitation is that this backend doesn't support
inoutconnections in module definitions. It doesn't seem like any tests in the coreir suite cover this use case, so perhaps this is not a blocking feature, but we should make sure that is true for our upstream users (garnet/lassen). It's not hard to add this support, but again, I just wanted to get this merged in ASAP to avoid integration pains (rather than waiting until every single edge case was covered).