Repair transformation legality checks - #58
Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Integrate comments in the woven code
…to fix/update-push-mirror
Updates repository name of push-mirror
…rmation-legality-checks # Conflicts: # .github/workflows/nightly.yml
lm-sousa
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48cff8e5db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const reads = arrayNames(summary.arrayReads); | ||
| const writes = arrayNames(summary.arrayWrites); | ||
| return intersects(reads, writes); |
There was a problem hiding this comment.
Reject repeated writes to the same array when changing loop order, not only read/write intersections. For example, interchanging do i; do j; A(i+j)=i changes which (i,j) pair writes each overlapping element last, but this summary has array writes and no array reads, so both canInterchange and canTile currently accept it and can silently change results.
Useful? React with 👍 / 👎.
| if (variable instanceof ArraySubscriptExpr) { | ||
| result.arrayWrites.push({ name: variable.var.name }); | ||
| for (const name of scalarReferences(variable)) { | ||
| result.scalarReads.add(name); | ||
| } |
There was a problem hiding this comment.
Include nested array accesses on the left-hand side in arrayReads. In a fusion candidate where the first loop assigns A(B(i)) and the second writes B(i+1), the original first loop reads every old B value before any writes, whereas the fused loop can read a value written by the preceding iteration; because scalarReferences(variable) excludes array bases and only RHS arrays are added to arrayReads, this dependency is missed and the loops are incorrectly fused.
Useful? React with 👍 / 👎.
| if (reference.var.name === name) return true; | ||
| } else if (reference.name === name) { | ||
| return true; |
There was a problem hiding this comment.
Normalize identifier case before testing references because Fortran identifiers are case-insensitive and the parser preserves source spelling. If the outer iterator is written as i while an inner bound refers to I, this exact comparison misses the reference, allowing interchange or tiling of a triangular iteration space and changing the executed iterations; the same issue affects dependency summaries containing mixed-case uses.
Useful? React with 👍 / 👎.
| function freshName(preferred: string, usedNames: Set<string>): string { | ||
| let name = preferred; | ||
| let suffix = 2; | ||
| while (usedNames.has(name)) { | ||
| name = `${preferred}_${suffix}`; |
There was a problem hiding this comment.
Check usedNames using normalized Fortran identifiers. With a loop variable i and an existing declaration such as I_TILE, the set does not consider generated i_tile occupied, so tiling reuses the existing variable as its tile counter and can overwrite user state or alter loop bounds.
Useful? React with 👍 / 👎.
The stacked parent PR exposed several fundamental correctness problems: traversal errors were swallowed, legality checks inspected rendered source, loop relationships were inferred from iterator names or proxy identity, and several joinpoints did not carry their backing AST nodes. That made transformations silently skip nodes or approve unsafe rewrites.
This layer repairs those invariants with AST-based, conservative legality checks and adds focused regressions for dependencies, explicit loop steps, nested bounds, reused iterator names, and real 3mm transformations.
Validation:
npm run buildnpm run lintModel: OpenAI Codex (gpt-5.6-luna), operating as the implementation agent.