-
Notifications
You must be signed in to change notification settings - Fork 283
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
[FIRRTL] Inliner: Support for ops with regions. #7398
base: main
Are you sure you want to change the base?
[FIRRTL] Inliner: Support for ops with regions. #7398
Conversation
inlineInstances/flattenInstances: * Walk entire body, not only top-level operations. Fixes missing instances and allows inlining them when conservatively legal. * Reject inlining instances under when/match. inlineInto/flattenInto: Walk entire body using new `inliningWalk` method that drives the per-operations handling but also handles cloning "structure" operations that have regions (when/match/layer) and managing what should be cloned where. This allows inlining modules that contain these operations. Inliner now may produce errors, thread throughout. This allows the inliner to run earlier in the pipeline, particularly before LowerLayers.
Pull this into helper method, add error test.
This especially helps when the location points to a construct we've lowered to something else (e.g., layerblock -> sv.ifdef).
e7ce11d
to
5d819f4
Compare
There's some follow-up commits too, but may be useful to look through this before the the "format" commit, most of the changes are here: f6e254b . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Some nits/comments throughout.
/// Check not inlining into anything other than layer or module. | ||
/// In the future, could check this per-inlined-operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
/// Check not inlining into anything other than layer or module. | |
/// In the future, could check this per-inlined-operation. | |
/// Check not inlining into anything other than a layerblock or module. | |
/// In the future, could check this per-inlined-operation. |
continue; | ||
} | ||
return inliningWalk( | ||
il.mic.b, target.getBodyBlock(), mapper, [&](Operation *op) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this lambda is pretty massive. This may be clearer broken out into named lambda expression.
return success(); | ||
|
||
inliningStack.push_back(IPs{builder.saveInsertionPoint(), block->begin()}); | ||
OpBuilder::InsertionGuard x(builder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
OpBuilder::InsertionGuard x(builder); | |
OpBuilder::InsertionGuard guard(builder); |
Alternatively:
OpBuilder::InsertionGuard x(builder); | |
OpBuilder::InsertionGuard _(builder); |
// Clone source into insertion point 'target'. | ||
auto &source = *ips.source; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mega-nit: When reading this comment, this makes me think that the next line is doing the cloning when it clearly isn't. Could the PR reorganize this comment to avoid the confusion if this seems confusing to you on inspection?
TypeSwitch<Operation *, LogicalResult>(&sourceWithRegions) | ||
.Case<LayerBlockOp, WhenOp, MatchOp>([&](auto op) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love TypeSwitch
. However, this can avoid one level of nesting with an isa
given that this is only two cases: (1) layer block/when/match and (2) error.
currentPath.pop_back(); | ||
activeHierpaths = parentActivePaths; | ||
return success(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't review this part in detail as this all appeared to be code motion. Echoing an earlier comment, this diff may be less heinous without the large anonymous lambda.
// Erase the replaced instance. | ||
instance.erase(); | ||
} | ||
return failure(result.wasInterrupted()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not review this in detail as it appears to also be code motion.
for (auto targetNLA : instOpHierPaths[innerRef]) { | ||
nlaMap[targetNLA].flattenModule(target); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
for (auto targetNLA : instOpHierPaths[innerRef]) { | |
nlaMap[targetNLA].flattenModule(target); | |
} | |
for (auto targetNLA : instOpHierPaths[innerRef]) | |
nlaMap[targetNLA].flattenModule(target); |
firrtl.matchingconnect %c_i, %i : !firrtl.uint<8> | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rejection of layerblock inlining is fine for now. 👍 There are some situations where it could be legal or could be fixed up like you've suggested before.
firrtl.matchingconnect %c_cond, %cond : !firrtl.uint<1> | ||
firrtl.ref.define %o, %c_p : !firrtl.probe<uint<8>, @I::@J> | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are a tad large. It may be better to factor this into narrower tests of features. E.g., just inlining into a layerblock of a trivial module. There are also a lot of operations which could be pruned, e.g., invalid connections.
inlineInstances/flattenInstances:
Fixes missing instances and allows inlining them
when conservatively legal.
inlineInto/flattenInto:
Walk entire body using new
inliningWalk
methodthat drives the per-operations handling but also
handles cloning "structure" operations that have
regions (when/match/layer) and managing what
should be cloned where.
This allows inlining modules that contain these
operations.
Inliner now may produce errors, thread throughout.
This allows the inliner to run earlier in the pipeline,
particularly before LowerLayers.