EPFL-LAP / dynamatic

DHLS (Dynamic High-Level Synthesis) compiler based on MLIR
Other
65 stars 19 forks source link

[External functions] Updated the external function feature #144

Closed Carmine50 closed 2 months ago

Carmine50 commented 2 months ago

In this new pull request there are 3 important modifications to the external function feature:

Jiahui17 commented 2 months ago

Hi Carmine thank you for fixing this! I have two questions about the design choices (also see the review above):

Carmine50 commented 2 months ago

Hello @Jiahui17 , here are my answers to your questions:

  1. An external function represents a specific set of ports. The instantiation of a function represents the presence of this set of ports. By definition, you cannot have 2 sets of ports with the same name. Since this feature enables the user to specify the set of ports, the user should also decide different names for 2 sets of ports.

  2. The backend job would be unchanged. Removing the unused results only means removing unused ports. The job of the backend would not change.

lucas-rami commented 2 months ago

I am ok with the policy of only being able to invoke an external function once because it is inconvenient to derive unique port names for all invocations.

However, I do not think that dynamically removing ports for unused results of the internal function call is a good design decision. It ties the interface of the circuit we generate with the details of the internal circuit architecture, while the two should be as independent as possible. Additionally, this creates an inconsistency with how we handle "normal function arguments"; even if they are not used inside the function, we still instantiate ports for them as if they were used. That way, when one looks at a function's declaration, one can know for sure what the interface will look like for "normal function arguments", irrespective of the function's definition. Finally, if we start removing ports when they are not used, it makes it much harder for someone iterating on a kernel's implementation to interface programmatically with the circuit we generate; they may make a temporary change in the kernel implementation that renders an argument unused, and suddenly their wrapper circuit no longer works because it references ports of the Dynamatic circuits that are no longer generated.

Carmine50 commented 2 months ago

Hey @lucas-rami ,

Related to your second point, yes it makes sense. I will revert those changes accordingly.

Carmine50 commented 2 months ago

Yes sorry, I pushed the wrong file. I made the correct changes. Anyway, I changed the SmallVector sizes because the compiler would give me an error for inconsistent return type. I pushed it by mistake. Thank you :)