[Sync] add syncFinder escape evidence tests#603
[Sync] add syncFinder escape evidence tests#603TaoTao-real wants to merge 1 commit intohw-native-sys:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several LIT test cases to reproduce synchronization issues in the syncFinder analysis, specifically focusing on zero-trip nested and single loops, as well as branch analysis involving virtual else blocks. The review feedback correctly identifies that the CHECK-NOT directives in the loop-related tests are misplaced. Due to the reverse-scan nature of the InsertSyncAnalysis, a pipe_barrier is processed before a wait_flag, meaning the barrier appears first in the debug output. The current placement of CHECK-NOT before the barrier check would fail to catch the presence of the wait_flag if the bug were fixed, rendering the test ineffective for regression testing.
| // CHECK: [ 7] COMPOUND pto.tstore [PIPE_MTE3] | ||
| // CHECK-NOT: wait_flag <PIPE_MTE2 -> PIPE_MTE3> | ||
| // CHECK: PRE : pipe_barrier <PIPE_MTE3 -> PIPE_MTE3> |
There was a problem hiding this comment.
The CHECK-NOT directive is misplaced and will not correctly validate the absence of the synchronization flag if the bug is fixed.
In the InsertSyncAnalysis implementation, synchronization operations are added to the pipeBefore list in the order they are discovered during the reverse scan. The loop-carried dependency (which generates the pipe_barrier) is processed before the top-level dependencies (which would generate the wait_flag). Consequently, the pipe_barrier appears before any wait_flag in the debug output.
FileCheck's CHECK-NOT only ensures the pattern does not occur between the previous CHECK and the next CHECK. Currently, it only checks the gap between the COMPOUND line and the PRE : pipe_barrier line. If the bug were fixed and the wait_flag were correctly inserted, it would appear after the barrier, and this test would still pass (failing to catch the regression). Additionally, note that wait_flag <PIPE_V -> PIPE_MTE3> is also missing for this node due to the same escape issue.
// CHECK: [ 7] COMPOUND pto.tstore [PIPE_MTE3]
// CHECK: PRE : pipe_barrier <PIPE_MTE3 -> PIPE_MTE3>
// CHECK-NOT: wait_flag <PIPE_MTE2 -> PIPE_MTE3>
| // CHECK: [ 5] COMPOUND pto.tstore [PIPE_MTE3] | ||
| // CHECK-NOT: wait_flag <PIPE_MTE2 -> PIPE_MTE3> | ||
| // CHECK: PRE : pipe_barrier <PIPE_MTE3 -> PIPE_MTE3> |
There was a problem hiding this comment.
The CHECK-NOT directive is misplaced here as well. Since the pipe_barrier is discovered and inserted into the pipeBefore list before the wait_flag during the reverse analysis, it will appear first in the debug output. Moving the CHECK-NOT after the barrier check ensures that the test correctly fails if the wait_flag is present anywhere in the PRE block.
// CHECK: [ 5] COMPOUND pto.tstore [PIPE_MTE3]
// CHECK: PRE : pipe_barrier <PIPE_MTE3 -> PIPE_MTE3>
// CHECK-NOT: wait_flag <PIPE_MTE2 -> PIPE_MTE3>
Codex Review该评论由 review 机器人自动更新。
SummaryPR #603 adds two passing lit tests that codify the known zero-trip sync bug, so a real fix will look like a regression in CI. Findings
The comment above this check says the correct zero-trip behavior is to keep a direct
This nested-loop variant has the same contract problem: the file documents that the post-loop |
Summary
Add focused lit tests that document why
syncFindermust not escape may-path control-flow regions, and what evidence we have today from currentmainbehavior.This PR is intentionally explanation-only:
Why this matters
InsertSyncAnalysisuses two pieces of state during reverse scan:alreadySync[pipe]syncFinder[syncIndex]For loop regions, current
maincarriessyncFinderout of the loop body while intentionally not carryingalreadySync.That is enough to be dangerous: an old wait inside a may-execute loop can still reactivate a matching top-level set later, rebuild
alreadySyncoutside the loop, and then incorrectly eliminate a direct post-loop sync that is required when the loop zero-trips.For
scf.ifwithout an explicit else, current frontend lowering materializes a virtual else region, so today we do not exercise theInsertBranchSyncpath that would propagatesyncFinderfrom a true no-else region. The new branch test documents that evidence explicitly.Added tests
1.
issue_syncfinder_zero_trip_single_loop_debug.ptoEvidence for the single-loop zero-trip failure mode.
Shape:
MTE2 -> VV -> MTE3MTE3consumer still directly depends on the originalMTE2Expected zero-trip-safe behavior:
MTE2 -> MTE3sync for the post-loop consumerObserved on current
main:After AnalysiscontainsMTE2 -> VandV -> MTE3MTE2 -> MTE3is missingThis is the minimal positive reproducer that shows loop
syncFinderescape is functionally unsafe.2.
issue_syncfinder_zero_trip_nested_loop_debug.ptoEvidence that the same problem composes across nested loops.
Shape:
MTE2 -> VV -> MTE3MTE3consumer still directly depends on the originalMTE2Expected zero-trip-safe behavior:
MTE2 -> MTE3sync for the post-loop consumerObserved on current
main:After Analysisstill drops the directMTE2 -> MTE3This shows the problem is not limited to a single loop layer.
3.
syncfinder_if_virtual_else_safe_debug.ptoBranch-side contrast case.
Shape:
MTE2 -> Vscf.ifthen-branch consumes the V result withMTE3MTE3consumer directly depends on the originalMTE2Observed on current
main:IF_BEGIN,ELSE_BEGIN, and avirtualElseplaceholder even when source IR has no explicitelseAfter Analysisstill keeps the directMTE2 -> MTE3This is important because it documents the current evidence for the branch case:
scf.ifwithoutelsedoes not become a true “then-only region” in SyncIRsyncFinderoutward, that would need the same scrutiny as loop propagationValidation
Ran these checks with current
ptoasplus LLVMFileCheck:issue_syncfinder_zero_trip_single_loop_debug.ptoissue_syncfinder_zero_trip_nested_loop_debug.ptosyncfinder_if_virtual_else_safe_debug.ptoAll checks passed.