InsertSync: guard sync motion by branch predicate domain#515
InsertSync: guard sync motion by branch predicate domain#515TaoTao-real wants to merge 1 commit intohw-native-sys:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a branch-predicate guard to ensure that synchronization operations remain within the same execution predicate as their dependency anchors during motion optimizations. It adds logic to calculate predicate keys based on enclosing branch structures and uses these keys to validate whether a sync operation can be safely hoisted or sunk across branch or loop boundaries. A regression test case for nested-if sync motion is also included. The review feedback highlights potential performance issues due to push_front instead of push_back to maintain the original program order of sync operations when they are not moved.
| MoveSyncState::GetBranchPredicateKey(unsigned int syncIRIndex) const { | ||
| SmallVector<int, 8> key; | ||
| if (syncIRIndex >= syncIR_.size()) { | ||
| return key; | ||
| } | ||
|
|
||
| for (const auto &element : syncIR_) { | ||
| auto *branch = dyn_cast<BranchInstanceElement>(element.get()); | ||
| if (!branch || branch->getBranchKind() != KindOfBranch::IF_BEGIN) { | ||
| continue; | ||
| } | ||
| if (!(branch->beginId < branch->endId)) { | ||
| continue; | ||
| } | ||
| if (syncIRIndex <= branch->beginId || syncIRIndex >= branch->endId) { | ||
| continue; | ||
| } | ||
|
|
||
| // Encode each enclosing if's side into a stable token: | ||
| // token*4+0 => then-side | ||
| // token*4+1 => else-side | ||
| // token*4+2 => else-boundary marker | ||
| const int token = static_cast<int>(branch->beginId) + 1; | ||
| if (syncIRIndex < branch->branchId) { | ||
| key.push_back(token * 4 + 0); | ||
| } else if (syncIRIndex > branch->branchId) { | ||
| key.push_back(token * 4 + 1); | ||
| } else { | ||
| key.push_back(token * 4 + 2); | ||
| } | ||
| } | ||
| return key; | ||
| } |
There was a problem hiding this comment.
The current implementation of GetBranchPredicateKey iterates over the entire syncIR_ vector on every call to find enclosing branches. Since this function is called multiple times for every sync operation during the transformation (via HasSameBranchPredicate), it results in
Consider pre-calculating these predicate keys once at the beginning of the Run() method or implementing a cache to store the keys for each instruction index. This will significantly improve performance for large IRs.
| const unsigned depIndex = sync->GetDepSyncIRIndex(); | ||
| if (depIndex >= syncIR_.size()) { | ||
| return false; | ||
| } | ||
| return HasSameBranchPredicate(depIndex, targetIndex); |
There was a problem hiding this comment.
The depSyncIRIndex in the SyncOperation class defaults to 0. If a barrier-type sync operation has not had its dependency anchor explicitly set, this fallback logic will compare the target predicate against the predicate of the first instruction in the IR (index 0). While this is likely conservative (as the first instruction is usually in the root predicate domain), it is logically fragile. It would be safer to initialize depSyncIRIndex_ to an invalid value (e.g., ~0U) and check for that value here before proceeding.
| if ((waitSync->GetSyncIRIndex() >= pair.second) || | ||
| (waitSync->GetSyncIRIndex() <= pair.first)) { | ||
| if (!CanMoveToPredicateCompatibleAnchor(s, bound.second)) { | ||
| newPipeAfter.push_back(s); |
There was a problem hiding this comment.
Since the caller PlanMoveOutBranchSync iterates over e->pipeAfter in reverse order (line 157), using push_back here will cause the relative order of sync operations that are not moved to be reversed when they are reassigned to e->pipeAfter. To maintain the original program order for these syncs, you should use push_front instead.
| newPipeAfter.push_back(s); | |
| newPipeAfter.push_front(s); |
| if ((waitSync->GetSyncIRIndex() > pair.second) || | ||
| (waitSync->GetSyncIRIndex() < pair.first)) { | ||
| if (!CanMoveToPredicateCompatibleAnchor(s, pair.second)) { | ||
| newPipeAfter.push_back(s); |
There was a problem hiding this comment.
Similar to the logic in branch motion, the caller MoveOutSync iterates over e->pipeAfter in reverse order (line 270). Using push_back for syncs that stay in place will reverse their original relative order. Use push_front to preserve the order in the newPipeAfter deque.
| newPipeAfter.push_back(s); | |
| newPipeAfter.push_front(s); |
Codex Review该评论由 review 机器人自动更新。
Summary未检查到 PR #515 存在问题 FindingsNo issues found. |
2 participants
Summary
MoveSyncStateso sync motion (hoist/sink) only happens when target anchor is in the same effective branch predicate domain as the sync's paired peer (set/wait).issue_nested_if_predicate_sync_motion_guard.ptoto lock nested-if behavior.Background
wait/setacross the nearest enclosing control-flow predicate can cause mismatch risk (e.g. outer-if false path executes moved sync while original producer path does not).Design
MoveSyncState:GetBranchPredicateKey(syncIRIndex)HasSameBranchPredicate(lhs, rhs)CanMoveToPredicateCompatibleAnchor(sync, targetIndex)PlanMoveOutIfWaitSyncPlanMoveOutIfSetSyncPlanMoveOutWaitSyncPlanMoveOutSetSyncValidation
ptoaslocally on this branch.set_flag/wait_flag(PIPE_MTE1, PIPE_M, EVENT_ID0)remain inside outerifbefore innerif.issue454_loop_if_else_loop_carried_sync_regression.ptoissue454_nested_loop_same_pipe_pair_regression.ptoNotes
FileCheckbinary in this worktree, so validation used generated C++ structural inspection (rgon emitted output).