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

Support parameterized multiports in C++ #387

Merged
merged 35 commits into from
Jul 20, 2021
Merged

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Jul 1, 2021

This PR adds support for parameterized multiport and bank widths for the C++ target. Since parameters are assigned at runtime in C++, this is achieved by resolving the precise widths at runtime and letting the generated code draw the connections as needed. For this to work, actually only a small library function was added to lib/lfutil.hh and no bigger changes in reactor-cpp where required.

This PR is based on #375, and should only be merged after merging #375.

Related to #386, #377, #343, #193, #209

Closes #349, #190

@cmnrd cmnrd added compiler cpp Related to C++ target labels Jul 1, 2021
@cmnrd cmnrd added this to the 0.1.0 Beta milestone Jul 1, 2021
@cmnrd cmnrd requested review from lhstrh and oowekyala July 1, 2021 13:43
@cmnrd cmnrd linked an issue Jul 1, 2021 that may be closed by this pull request
cmnrd added 5 commits July 1, 2021 16:21
This change adds widthof() operator to the grammar which allows to determine the
width of a port. This is optionally used in ASTUtils to precisely specify the
bank width for multiport connections based on all the port widths on the right
side of the connection.
@cmnrd cmnrd changed the base branch from master to cpp-scoping July 2, 2021 13:54
@cmnrd
Copy link
Collaborator Author

cmnrd commented Jul 2, 2021

After delays on multiport connections with (potentially) parameterized width turned out to be really tricky. The problem is, that the AST transformation for after delays does not infer the width of the generated bank of delays. It uses a "variable" width, which means that the precise width needs to be inferred by the generator. However, this is not trivial and I don't know how to infer the width at runtime without providing additional information.

I talked to @edwardalee yesterday about this problem. He suggested to not use AST transformations, but support after delays directly in the C++ runtime. This is something I actually wanted to try for a long time, so I did that today. However, this turned out to be far from trivial. The main problem is that we can have various kinds of connections in a chain (input to contained input, output to input, contained output to output) and all those connections can have associated delays. I didn't find a straight forward way to deal with all the different corner-cases and I think that supporting delayed connections might require a significant redesign of the C++ runtime. So I abandoned that plan for now...

I actually found another solution to the problem which is probably a bit hacky. Essentially, I introduced the widthof keyword, which can be used to get the width of a port like so input[widthof(some.port)] in:int. I think such an operator could be quite useful, but for now the validator actually forbids its usage in LF code. However, the AST transformation uses this mechanism to define the width of the delay bank based on all right ports involved in the connection. This gives the C++ code generator all the information required for determining the width of the delay bank at runtime. It can simply sum the actual widths of all ports (multiplied by the bank width if the port is a contained port in a bank). This mechanism is currently only activated for C++, all other targets still use the variable length width.

@edwardalee @lhstrh What do you think of this approach? Do you think its a viable solution to the problem?

@edwardalee
Copy link
Collaborator

I'm not sure this will solve the problem in general. What if you have the following widths:

   a.out: 2
   b.out: 2
   c.in: 3
   d.in: 1

And you connect them as follows:

    a.out, b.out -> c.in, d.in after 10 msec;

Then I think there will be three inserted delay reactors with widths 2, 1, and 1.
Also, what ports would be scope for the widthof operator?

@cmnrd
Copy link
Collaborator Author

cmnrd commented Jul 5, 2021

For your example, the LF code generated by the AST transformation would look like this:
delay = new[widthof(c.in) + widthof(d.in)] Delay()

I am not sure why you think that three delay reactors would be inserted. As far as I can see, the AST transformation always generates one bank of delay reactors per after connection, where the width of the delay bank is the summed width of all ports (and banks) on the right side of the connection. If the referenced port is a multiport within a bank, widthof(bank.port) would return the bank width multiplied by the port width.

The scope of the widthof operator is the same as for all other references to ports in LF (like triggers or effects). Basically, it can reference all ports within the current reactor or within any contained reactors (one level deep). However, the widthof operator is currently only usable in AST transformations and not in user written code.

Base automatically changed from cpp-scoping to master July 6, 2021 20:03
@cmnrd
Copy link
Collaborator Author

cmnrd commented Jul 13, 2021

I would like to merge this soon if possible. @edwardalee @lhstrh Do you think the widthof solution is viable for now?

@edwardalee
Copy link
Collaborator

The widthof solution still feels a bit kludgey to me, but I'm not opposed to including it. I'm not sure how this will play out for federated execution, but I guess we can cross that bridge when we come to it.

@cmnrd cmnrd requested review from edwardalee and Soroosh129 July 19, 2021 09:16
@cmnrd
Copy link
Collaborator Author

cmnrd commented Jul 19, 2021

Ok, I am also not too sure about widthof, but it seems to solve the issue for now. We should keep this in mind however and might want to find better solutions in the future. @edwardalee Could you approve this PR? I need the approval to merge due to new merging rules.

@edwardalee
Copy link
Collaborator

Github does not offer me a button to approve this pull request, so I don't see how I could be blocking it.

Copy link
Collaborator

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

OK, I found the hidden button...

@cmnrd cmnrd merged commit aa6fbd2 into master Jul 20, 2021
@cmnrd cmnrd deleted the cpp-parameterized-multiports branch August 20, 2021 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler cpp Related to C++ target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support paramterized multiports and banks through generics Multi reactor instances and scoping rules
2 participants