Use vector of pairs instead of map for instance ports #39
Conversation
Updates for leonardt/verilogAST-cpp#39 so that the order of ports for module instances are preserved. The old logic used a std::map which uses lexical order, causing the output to sort the ports based on the names. Migrating to a std::vector preserve's CoreIR's internal ordering of the ports (which is based on insertion order)
Updates for leonardt/verilogAST-cpp#39 so that the order of ports for module instances are preserved. The old logic used a std::map which uses lexical order, causing the output to sort the ports based on the names. Migrating to a std::vector preserve's CoreIR's internal ordering of the ports (which is based on insertion order)
|
Bump |
|
It seems like whether or not things are ordered (this or port names, etc.) should be part of the spec., not an implementation detail. I think it would be good to put this behind a class abstraction like #include <algorithm>
class Connections {
public:
Connections() : connections() {}
~Connections() = default;
// Non-copyable class
Connections(const Connections&) = delete;
// Takes ownership of @expr.
void insert(std::string name, std::unique_ptr<Expression> expr) {
connections.push_back(std::make_pair(name, std::move(expr)));
}
// Releases ownership of expression at @name if exists, othwerwise throws error.
std::unique_ptr<Expression> at(std::string name) {
auto is_name = [name](auto& element) { return element.first == name; };
auto it = std::find_if(connections.begin(), connections.end(), is_name);
if (it != connections.end()) return std::move(it->second);
throw std::runtime_error("Could not find '" + name + "'");
}
private:
std::vector<std::pair<std::string, std::unique_ptr<Expression>>> connections;
}; |
|
I have this issue in CoreIR as well. I think it would be worthwhile to roll our own "insertion_order_map" class to be used by both this and CoreIR. Unfortunately I have had no luck finding an implementation of one online (other than using boost) |
|
there's this: https://github.com/Tessil/ordered-map but for what we're doing we can do something much simpler |
|
Updated to use Connections class |
| // NOTE: anonymous style of module connections is not supported | ||
| std::map<std::string, std::unique_ptr<Expression>> connections; | ||
| std::unique_ptr<Connections> connections; |
There was a problem hiding this comment.
don't need this to be dynamically allocated but fine either way.
There was a problem hiding this comment.
If I don't use a dynamic allocation I get an issue with a deleted copy constructor
/Users/leonardtruong/repos/verilogAST-cpp/include/verilogAST.hpp:533:9: err
or:
call to deleted constructor of 'verilogAST::Connections'
connections(std::move(connections)){};
^ ~~~~~~~~~~~~~~~~~~~~~~
/Users/leonardtruong/repos/verilogAST-cpp/include/verilogAST.hpp:490:3: not
e:
'Connections' has been explicitly marked deleted here
Connections(const Connections&) = delete;
I would need to add a copy constructor that clones the pointers
|
Also this branch changes generated verilog, so we'll want to stage magma/mantle/fault with this |
|
magma/mantle/fault test suites have been updated, will be merging these today. in theory shouldn't effect downstream tools that don't regress on generated verilog, but may be good to double check garnet, are there any other possible places? |
|
Checked that garnet is unaffected |
This changes the module instance logic to use a vector of pairs (port name, connection) to handle the port connections, rather than a map, so that the code generation uses insertion order rather than lexical order. CoreIR will need to be updated before this is merged.