-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add ConcatCoalescer transformer #41
Conversation
@leonardt not sure why coverage dropped... |
I never figured it out, but I don't think gcov/lcov handles templated implementations in header files properly (I think it might be because those those lines don't actually ever get executed, but they get expanded into multiple instantiations and those lines aren't properly mapped back to the original source) |
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, covers the basic case.
What about something like
{I[3], I[2], I[1], I[0], J[3], J[2], J[1], J[0]}
Ideally in the case we'd emit something like
{I[3:0], J[3:0]}
I think it would use the same logic, but need to handle the case when you concat just a subset of the args, and replace those args with the slice expression, then restart on the rest of the args.
Ah just read your comment, yea I think we can merge this in and see if other cases actually come up in user code. |
Although I think the above pattern I showed was something I saw, basically what they do is they create a big array, then wire up elements of the array as ranges of bits from other places. For example, get the lower 8 bits of a bunch of array fields. In this case, they'd want something like |
yeah, I think adding that more general functionality isn't too hard. I'll take a pass at it |
Refactored tests and added tests for multiple runs and no runs.
603d03b
to
eaf85c7
Compare
@leonardt added func. and tests fro multiple runs within a single concat. should solve the |
We'll need to add the logic to invoke this pass into the coreir verilog backend, should we guard it behind a flag? Or just run it by default? |
Maybe we can use this as an opportunity to modularize the code generation options? Perhaps the best path is to create a coreir branch with this enhancement and let the people who need it use it as a one off. I think I can do the code gen options refactoring by end of this week, then we can merge it. |
First pass at adding a pass to make
{x[n], x[n - 1], ..., x[k]}
look likex[n:k]
. The transformation right now only looks for fully contiguous concat args. We could have other cases like multiple contiguous runs, etc., but that support can easily be added to this pass and I think this covers the most useful case.