[HW] Add pass to parameterize constant ports on private modules.#9266
Conversation
This pass converts input ports on private modules into parameters when all instances pass constant values (or parameter values) to those ports. By converting constant ports to parameters, synthesis pipelines can recognize these values as compile-time constants through local analysis alone, without requiring inter-module analysis. This enables more precise timing information and better optimization of each instance independently, since the constant values are immediately visible at the module interface.
cowardsa
left a comment
cowardsa
left a comment
There was a problem hiding this comment.
LGTM - nice change to simplify the analyses!!
I didn't see a test for if a port is used as both constant and variable, which might be nice to check for as you wouldn't want that to be parameterized?
| // Build new port names for instances (excluding removed ports) | ||
| DenseSet<unsigned> portsToRemoveSet(portsToParameterize.begin(), | ||
| portsToParameterize.end()); |
There was a problem hiding this comment.
Non-Blocking - an extension could check whether all the ports are unique and fuse ports that are always identical - but perhaps this is handled by a different pass already
There was a problem hiding this comment.
Yes, I agree such pass is very beneficial. #4638 is similar issue.
| %c1_i4 = hw.constant 1 : i4 | ||
| // CHECK: %inst1.out = hw.instance "inst1" @MultipleConstantPorts<id: i8 = 1, mode: i4 = 1> | ||
| // CHECK-SAME: (data: %data: i32) -> (out: i32) | ||
| %inst1.out = hw.instance "inst1" @MultipleConstantPorts(id: %c1_i8: i8, mode: %c1_i4: i4, data: %data: i32) -> (out: i32) |
There was a problem hiding this comment.
To incorporate a test that modules with ports that have both constant and variable instances - could make the mode port variable in this second instance?
There was a problem hiding this comment.
Great point! I changed mode to a variable (and added another port mode2 to test multiple parameters) 👍
dtzSiFive
left a comment
dtzSiFive
left a comment
There was a problem hiding this comment.
Fantastic work @uenoku !! 💯 🥳
(Thank you!)
Pass is very clear and clean, tests LGTM as well. Didn't fall over on various special cases, yet pass isn't cluttered with a bunch of one-offs. Awesome.
Left some comments, least "nit" was fixing bottom-up vs top-down in a comment 😉 .
| module.setParametersAttr(builder.getArrayAttr(newParameters)); | ||
|
|
||
| // Remove the parameterized ports from the module signature | ||
| module.modifyPorts({}, {}, portsToParameterize, {}); |
There was a problem hiding this comment.
Is this the only port mutation method we have? Not for this PR, but it uses a in-theory "deprecated" method modifyModulePorts as its implementation, so I'm surprised we aren't discouraging its use?
Not for this PR, looks like? 👍
There was a problem hiding this comment.
Yes, this is not great. I think there is no alternative method for port mutations now...
| static Attribute getAttributeValue(Value value) { | ||
| if (auto constOp = value.getDefiningOp<hw::ConstantOp>()) | ||
| return IntegerAttr::get(value.getType(), constOp.getValue()); | ||
| if (auto paramOp = value.getDefiningOp<hw::ParamValueOp>()) |
There was a problem hiding this comment.
Hyper-nit and honestly it's great as-is!
Per your judgement (!), but I'm looking at these methods (getAttributeValue and getConstantOrParamValue) and wondering if there's a slight organizational change that better captures / ensures that they both consider the same cases?
I'm less worried about the redundant "work" specifically but tiny possible clarity / maintenance improvement. And submitting the idea for your consideration 😉 .
As-is, the number of cases is low (!) and unlikely to change (especially with any frequency), and the methods as written are clear and concise 👍 . So I'm happy with them as-is for sure!
WDYT of grabbing the defining operation (well, its single result as a Value anyway) AND the value attribute in one go? Would save the llvm_unreachable which is in part where this thinking came from: What ensures that case is unreachable? --> The other method only covers those same cases too.
Would be neat if ConstantLike meant could easily grab the Attribute it would fold to...
There was a problem hiding this comment.
Great point! I agree ConstantLike trait provides some interface method :) I unified two methods to getAttributeAndDefiningOp (actually it was very straightforward) so I think this is ok now. Thank you for the suggetion!
|
|
||
| // Delete the constant or param.value op if it's only used by this | ||
| // instance. | ||
| if (constValue.hasOneUse()) { |
There was a problem hiding this comment.
Appreciate keeping the IR tidy. If this pass is ever hooked up to something that's more of the result of an analysis this might not be practical (or safe) to clean up this way naively but that's not what this pass is about! 👍
uenoku
force-pushed
the
dev/hidetou/constant
branch
from
0e01cda to
13bf08d
Compare
3 participants
This pass converts input ports on private modules into parameters when all instances pass constant values (or parameter values) to those ports.
By converting constant ports to parameters, synthesis pipelines can recognize these values as compile-time constants through local analysis alone, without requiring inter-module analysis. This enables more precise timing information and better optimization of each instance independently, since the constant values are immediately visible at the module interface.