[A5][Sync] remove identity tmov before insert-sync#419
[A5][Sync] remove identity tmov before insert-sync#419TaoTao-real wants to merge 1 commit intohw-native-sys:mainfrom
Conversation
|
Superseded by #420 (same fix, clean PR description and branch). |
There was a problem hiding this comment.
Code Review
This pull request introduces a new optimization pass, PTORemoveIdentityTMovPass, which removes identity pto.tmov operations (where source and destination are the same) specifically for the A5 architecture. This pass is integrated into the ptoas tool to run before synchronization insertion to prevent unnecessary sync edges. My feedback suggests simplifying the pass implementation by performing the operation erasure directly within the walk function, which avoids the need for an intermediate collection vector and improves code efficiency.
| SmallVector<TMovOp> toErase; | ||
| funcOp.walk([&](TMovOp op) { | ||
| if (canEraseIdentityTMov(op)) | ||
| toErase.push_back(op); | ||
| }); | ||
|
|
||
| for (TMovOp op : toErase) { | ||
| Value result = op.getResult(); | ||
| if (result && !result.use_empty()) | ||
| result.replaceAllUsesWith(op.getDst()); | ||
| op.erase(); | ||
| } |
There was a problem hiding this comment.
The current implementation iterates over the function twice: once to collect the TMovOps to erase, and a second time to perform the erasure. This can be simplified into a single walk. MLIR's walk function is safe to use with in-place erasure of the visited operation, making the intermediate toErase vector unnecessary. This improves both efficiency and readability.
funcOp.walk([&](TMovOp op) {
if (canEraseIdentityTMov(op)) {
Value result = op.getResult();
if (result && !result.use_empty())
result.replaceAllUsesWith(op.getDst());
op.erase();
}
});
Summary\n- Add an A5-only cleanup pass () that erases before auto-sync runs.\n- Wire the pass in directly before when is enabled.\n- Add regression test .\n\nMotivation\n- Fixes the A5 hang risk caused by identity being treated as a real producer/consumer by auto-sync, which can create spurious sync edges around a hardware no-op move.\n\nDesign\n- New pass: (func pass).\n- Gated by module attribute .\n- Removes only must-prove identity ( SSA value).\n- If optional result is used and type-compatible with , rewires uses to before erase.\n- Pipeline placement: .\n\nTesting\n- Built: ninja: Entering directory `build'\n- New targeted test checks:\n - A5: identity no longer emits or / sync edges.\n - A3: remains (scope constrained to A5).\n- Extra guard test: still passes.\n\nRisk / Rollback\n- Risk is low: behavior change is strictly A5-scoped and only for syntactic identity moves.\n- Rollback is straightforward: revert this PR.