Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Initial support for childref multiports #1228

Merged
merged 22 commits into from
Jan 9, 2023
Merged

Initial support for childref multiports #1228

merged 22 commits into from
Jan 9, 2023

Conversation

jhaye
Copy link
Collaborator

@jhaye jhaye commented Jun 13, 2022

This is an initial attempt to implement child references within a reactor being allowed to be multiports as explained in #806.

Some caveats:
Right now if the child reactors that are being referenced are a bank and then are further referenced to a multiport, this completely flattens this structure. So if the bank is of size 4 and the multiport of size 5, the childref would be unrolled into a PortBank of size 20. This is not ideal but was the easiest to implement right now.
Secondly, one of the provided unit tests called ReadOutputOfContainedBank fails right now due to a scheduling error, but it shows that it is able to access the data of the child reactor successfully.

Refs lf-lang/benchmarks-lingua-franca#38

@jhaye jhaye added the rust Related to the Rust target label Jun 13, 2022
@jhaye jhaye requested review from cmnrd and oowekyala June 13, 2022 13:23
@jhaye jhaye marked this pull request as ready for review July 25, 2022 09:23
@jhaye jhaye force-pushed the rust-childref-multi branch 2 times, most recently from e7d7379 to 971ceba Compare August 1, 2022 09:08
@jhaye jhaye marked this pull request as draft August 1, 2022 11:21
@jhaye
Copy link
Collaborator Author

jhaye commented Aug 2, 2022

Another caveat, if a reactor is initialised in another, and

  1. The contained reactor is used via childref
  2. The width of the bank used via childref is initialised via an argument to the respective reactor

it does not compile.

I have reduced this to a minimal example, that can be found here: https://gist.github.com/jhaye/ce4d92af09c37379529d3902bec1faab

I'm not sure how I would implement value propagation like this.

jhaye added a commit to lf-lang/benchmarks-lingua-franca that referenced this pull request Aug 2, 2022
@jhaye jhaye force-pushed the rust-childref-multi branch from 971ceba to 4900a7d Compare August 11, 2022 07:53
jhaye added a commit to lf-lang/benchmarks-lingua-franca that referenced this pull request Aug 11, 2022
@oowekyala
Copy link
Collaborator

So if the bank is of size 4 and the multiport of size 5, the childref would be unrolled into a PortBank of size 20. This is not ideal but was the easiest to implement right now.

I'm not sure how it's not ideal... IMHO it's a usability feature that there is a uniform API regardless of whether you're accessing a multiport on a child, a port on a bank of children, or a multiport on a bank. Are other targets doing this differently?

I have reduced this to a minimal example, that can be found here: gist.github.com/jhaye/ce4d92af09c37379529d3902bec1faab

This is now supported, thanks for the MWE.

@cmnrd
Copy link
Collaborator

cmnrd commented Sep 7, 2022

Are other targets doing this differently?

I am not sure if I understand how this works in Rust, but in C++ we do not unroll the multiport. In the case of multiports within a bank, the reaction body can say precisely which port in which reactor instance it wants to access. If it wants to read all values, it needs two nested loops:

target Cpp
reactor Source {
  output[5] out: int;
  reaction (startup) -> out {= /* ... */ =}
}
main reactor {
  source = new[4] Source()
  reaction (source.out) {=
    for (int i{0}; i < source.size(); i++) {
      for (int j{0}; j < source[i].out.size(); j++) {
         int value = *source[i].out[j].get();
         // ....
      }
    }
  =}
}

@edwardalee
Copy link
Collaborator

The C target does this similarly to Cpp.

@oowekyala
Copy link
Collaborator

Thanks for your feedback, so there is one view class generated per reaction to expose only the declared dependencies... We can probably also do that in Rust, I'll create a ticket

@cmnrd
Copy link
Collaborator

cmnrd commented Dec 22, 2022

@oowekyala What is the status of this PR? Should it be reviewed?

@oowekyala oowekyala marked this pull request as ready for review January 3, 2023 10:55
@oowekyala
Copy link
Collaborator

@cmnrd one of the new benchmarks in lf-lang/benchmarks-lingua-franca#38 shows a weird bug which I still have to investigate. I expect the fix will only touch the runtime crate so I think this PR can still be reviewed.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

This looks good to me, but it would be better if someone with a better understanding of the Rust target could have a look. Maybe @jhaye or @revol-xut?

@lhstrh lhstrh added the feature New feature label Jan 3, 2023
@jhaye
Copy link
Collaborator Author

jhaye commented Jan 4, 2023

I don't know much about the internals either, which is why I handed this PR off. All I can say from a cursory glance is that it looks good. I also like how the issue of value propagation was solved.

Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

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

Looks great!

@cmnrd cmnrd merged commit f031fd7 into master Jan 9, 2023
@cmnrd cmnrd deleted the rust-childref-multi branch January 9, 2023 16:11
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
feature New feature rust Related to the Rust target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants