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

[HW] Enable parametric polymorphism for module parameters #8040

Merged
merged 8 commits into from
Jan 24, 2025

Conversation

bubblepipe42
Copy link
Contributor

The current implementation of parametric polymorphism in modules, as described in the CIRCT documentation, supports input and output types but does not extend to parameters.

For example, given a parameter <DATA_WIDTH: i32>, it is possible to define an argument like in %input : !hw.int<#hw.param.decl.ref<"DATA_WIDTH">> , but it is not possible to define a parameter such as <RESET_VALUE: !hw.int<#hw.param.decl.ref<"DATA_WIDTH">>>

Consider the following code snippet:

hw.module.extern @param_in_param< 
  RESET_VALUE: !hw.int<#hw.param.decl.ref<"DATA_WIDTH">>, 
  DATA_WIDTH: i32 = 32
>( 
  in %input : !hw.int<#hw.param.decl.ref<"DATA_WIDTH">>, 
  out output : !hw.int<#hw.param.decl.ref<"DATA_WIDTH">>
)

hw.module @param_in_param_inst(
  in %input : i96,
  in %clk : i1, 
  in %rst : i1, 
  out output : i96
) {
  %a.output = hw.instance "a" @param_in_param<
    RESET_VALUE: i96 = 0, 
    DATA_WIDTH: i32 = 96
  >(
    input: %input: i96
  ) -> (output: i96)
  hw.output %a.output : i96
}

In this example, with the parameter DATA_WIDTH: i32 = 96 , the argument in %input of type hw.int<#hw.param.decl.ref<"DATA_WIDTH">> is correctly matched with input: %input: i96 through a simple substitution. However, circt-opt fails to recognize that RESET_VALUE: i96 is the equivalent to RESET_VALUE: !hw.int<#hw.param.decl.ref<"DATA_WIDTH">> . Instead it emits the following error message:

<source>:4:15: error: 'hw.instance' op parameter "RESET_VALUE" should have type '!hw.int<#hw.param.decl.ref<"DATA_WIDTH">>' but has type 'i96'
  %a.output = hw.instance "a" @param_in_param<RESET_VALUE: i96 = 0, DATA_WIDTH: i32 = 96>(input: %input: i96) -> (output: i96)
              ^
<source>:4:15: note: see current operation: %0 = "hw.instance"(%arg0) <{argNames = ["input"], instanceName = "a", moduleName = @param_in_param, parameters = [#hw.param.decl<"RESET_VALUE": i96 = 0> : i96, #hw.param.decl<"DATA_WIDTH": i32 = 96> : i32], resultNames = ["output"]}> : (i96) -> i96
<source>:1:1: note: module declared here
hw.module.extern @param_in_param<RESET_VALUE: !hw.int<#hw.param.decl.ref<"DATA_WIDTH">>, DATA_WIDTH: i32 = 32>(in %input : !hw.int<#hw.param.decl.ref<"DATA_WIDTH">>, out output : !hw.int<#hw.param.decl.ref<"DATA_WIDTH">>)
^
Compiler returned: 1

This issue can be reproduced using the following demo: Godbolt Link.

My proposed PR addresses this limitation and enables parametric polymorphism for module parameters, ensuring that types like !hw.int<#hw.param.decl.ref<"DATA_WIDTH">> are correctly recognized and validated.

@bubblepipe42 bubblepipe42 changed the title Enable parametric polymorphism for module parameters [HW] Enable parametric polymorphism for module parameters Jan 8, 2025
Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

test/Dialect/HW/parameters.mlir Outdated Show resolved Hide resolved
lib/Dialect/HW/HWInstanceImplementation.cpp Outdated Show resolved Hide resolved
@uenoku
Copy link
Member

uenoku commented Jan 24, 2025

Thank you for the PR! Shall I merge the PR?

@bubblepipe42
Copy link
Contributor Author

Thank you for the PR! Shall I merge the PR?

Sure! Thank you!

@uenoku uenoku merged commit 65ae4bd into llvm:main Jan 24, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants