[HW] Add HWConstProp pass for inter-module constant propagation #9207
Conversation
uenoku
left a comment
uenoku
left a comment
There was a problem hiding this comment.
LGTM, do you think it makes sense to add to Firtool pipeline (with opt out flag) for better coverage? Also does this block constprop across ports with inner symbol?
| // CHECK-NEXT: %wire = hw.wire %c42_i8 : i8 | ||
| // CHECK-NEXT: hw.output %wire : i8 | ||
| %c42_i8 = hw.constant 42 : i8 | ||
| %wire = hw.wire %c42_i8 : i8 |
There was a problem hiding this comment.
Do we need this restriction? I think wire without inner symbols can be optimized?
| } | ||
|
|
||
| def HWConstProp : Pass< | ||
| "hw-constprop", "mlir::ModuleOp" |
There was a problem hiding this comment.
nit: can we name this as imconstprop for consistency with FIRRTL?
seldridge
left a comment
seldridge
left a comment
There was a problem hiding this comment.
Partial review. I'd like to give this another look!
| #include "mlir/Pass/Pass.h" | ||
| #include "llvm/Support/Debug.h" | ||
|
|
||
| #define DEBUG_TYPE "hw-constprop" |
There was a problem hiding this comment.
When changing to imconstprop, make that change here.
Also, I figured out a way to not have to hard-code this, but it requires reorganizing the file a little bit. See how this works here: https://github.com/llvm/circt/blob/main/lib/Dialect/FIRRTL/Transforms/LowerDomains.cpp#L69
| void markUnknownValuesOverdefined(hw::HWModuleOp module); | ||
| std::pair<unsigned, unsigned> fold(); | ||
|
|
||
| public: |
There was a problem hiding this comment.
This public is unnecessary as this is already in a public scope.
| /** | ||
| * Returns the lattice value associated with an SSA value. | ||
| * | ||
| * `std::nullopt` is unknown, `IntegerAttr{}` is overdefined. | ||
| */ |
There was a problem hiding this comment.
Nit: I think this needs doxygen-style comments not Javadoc style, i.e., ///.
| //===----------------------------------------------------------------------===// | ||
|
|
||
| namespace { | ||
| class ConstantPropagation { |
There was a problem hiding this comment.
Please include comments for this and for all member functions/members.
I'm still grokking why there are three queues here and this would help with that!
| auto module = dyn_cast_or_null<HWModuleOp>(referencedOp); | ||
| return !module; |
There was a problem hiding this comment.
I think this can be:
| auto module = dyn_cast_or_null<HWModuleOp>(referencedOp); | |
| return !module; | |
| return !isa_and_nonnull<HWModuleOp>(referencedOp); |
| // Mark wires as overdefined since they can be targeted by force. | ||
| markOverdefined(wire.getResult()); |
There was a problem hiding this comment.
Is there a less-strict check that can work here? Like, just if this has an inner symbol?
| if (attr) { | ||
| valueQueue.push_back(user); | ||
| } else { | ||
| overdefQueue.push_back(user); | ||
| } |
There was a problem hiding this comment.
Nit:
| if (attr) { | |
| valueQueue.push_back(user); | |
| } else { | |
| overdefQueue.push_back(user); | |
| } | |
| if (attr) | |
| valueQueue.push_back(user); | |
| else | |
| overdefQueue.push_back(user); |
| private: | ||
| hw::InstanceGraph &graph; | ||
| DenseMap<Value, IntegerAttr> values; | ||
| DenseSet<Operation *> inQueue; |
There was a problem hiding this comment.
Calling this a "queue" is a bit disingenuous.
| std::optional<IntegerAttr> ConstantPropagation::map(Value value) { | ||
| if (auto constant = value.getDefiningOp<hw::ConstantOp>()) | ||
| return constant.getValueAttr(); | ||
|
|
||
| auto it = values.find(value); | ||
| if (it == values.end()) | ||
| return std::nullopt; | ||
|
|
||
| return it->second; | ||
| } |
There was a problem hiding this comment.
Calling this map seems a bit odd given that it's "getMaybeConstant". (That is a terrible name!) Something describing what this is doing would be better.
This PR adds the HWConstProp pass that performs inter-module constant propagation across hardware module boundaries in the HW dialect.
The pass uses the Sparse Conditional Constant Propagation (SCCP) with lattice-based analysis
Key Steps: