infer pipe init nosplit from split-0 users#452
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a nosplit attribute to pipe initialization operations and implements inference logic within the PTOResolveReservedBuffers pass to automatically set this attribute when downstream pipe users have a split value of 0. The EmitC backend is updated to include this boolean in the TPipe template parameters. Feedback suggests refactoring the pipe user inference loop for better readability and deduplicating the logic for resolving standalone nosplit attributes using a templated helper function.
| for (Operation *user : pipe.getUsers()) { | ||
| if (auto pushOp = dyn_cast<TPushOp>(user)) { | ||
| if (pushOp.getSplit() == 0) | ||
| return true; | ||
| continue; | ||
| } | ||
| if (auto popOp = dyn_cast<TPopOp>(user)) { | ||
| if (popOp.getSplit() == 0) | ||
| return true; | ||
| continue; | ||
| } | ||
| if (auto freeOp = dyn_cast<TFreeOp>(user)) { | ||
| if (freeOp.getSplit() == 0) | ||
| return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
The series of if statements with continue can be refactored into a more concise if-else if chain. This improves readability and is slightly more efficient as it avoids redundant checks. The continue statements are also unnecessary since the if blocks return immediately.
for (Operation *user : pipe.getUsers()) {
if (auto pushOp = dyn_cast<TPushOp>(user)) {
if (pushOp.getSplit() == 0)
return true;
} else if (auto popOp = dyn_cast<TPopOp>(user)) {
if (popOp.getSplit() == 0)
return true;
} else if (auto freeOp = dyn_cast<TFreeOp>(user)) {
if (freeOp.getSplit() == 0)
return true;
}
}
return false;| moduleOp.walk([&](InitializeL2LPipeOp initOp) { | ||
| if (groupedNoSplitResolved.count(initOp.getOperation())) | ||
| return; | ||
| if (inferNoSplitFromPipeUsers(initOp.getPipe())) | ||
| setNoSplitAttr(initOp, builder.getBoolAttr(true)); | ||
| }); | ||
| moduleOp.walk([&](InitializeL2G2LPipeOp initOp) { | ||
| if (groupedNoSplitResolved.count(initOp.getOperation())) | ||
| return; | ||
| if (inferNoSplitFromPipeUsers(initOp.getPipe())) | ||
| setNoSplitAttr(initOp, builder.getBoolAttr(true)); | ||
| }); |
There was a problem hiding this comment.
The logic inside the two moduleOp.walk calls for InitializeL2LPipeOp and InitializeL2G2LPipeOp is identical. This code duplication can be avoided by extracting the logic into a templated helper function, which would improve maintainability.
For example, you could define a helper function:
template <typename InitOpT>
void resolveStandaloneNoSplit(InitOpT initOp,
const std::set<Operation *> &resolvedOps,
OpBuilder &builder) {
if (resolvedOps.count(initOp.getOperation())) {
return;
}
if (inferNoSplitFromPipeUsers(getPipeResult(initOp))) {
setNoSplitAttr(initOp, builder.getBoolAttr(true));
}
}And then call it from the walks:
moduleOp.walk([&](InitializeL2LPipeOp initOp) {
resolveStandaloneNoSplit(initOp, groupedNoSplitResolved, builder);
});
moduleOp.walk([&](InitializeL2G2LPipeOp initOp) {
resolveStandaloneNoSplit(initOp, groupedNoSplitResolved, builder);
});
What changed
Infer implicit
nosplit = trueonly on internal pipe init ops when any same-logical-pipepto.tpush,pto.tpop, orpto.tfreein the module usessplit = 0.Why
The init-side
nosplitsetting should be derived from pipe usage semantics instead of being carried as an explicit push/pop frontend parameter.Impact
Generated
TPipe<...>init templates now append the finaltrueorfalseflag based on split-0 usage on the same pipe, while frontend and internal push/pop ops continue to use only the existingsplitattribute.Root cause
The previous implementation direction risked spreading
nosplitonto data ops, but the intended behavior is to keep it implicit and init-scoped.Validation
tpush/tpoppipe init templatestest/basic/tpush_tpop_frontend_nosplit_a5.ptoptoaslocally because no usable local binary/build was available in this environment