Add transformation legality checks by svkorepanov · Pull Request #53 · specs-feup/metafor

svkorepanov · 2026-06-15T11:33:36Z

No description provided.

…x staging traversal bugs ## parenExpr factory (FortranNodeFactory → AstFactory → FortranJoinPoints) - FortranNodeFactory.java: add parenExpr(Expr) factory using existing ParenExpr node - AstFactory.java: expose parenExpr(AExpr) as a static LARA-accessible factory - FortranJoinPoints.ts: add parenExpr(Expr) TypeScript wrapper ## LoopUnroll.ts — two targeted fixes Fix A (cleanup loop bound — dynprog, lu): Wrap ctrl.lower in parenExpr() in the tripCount subtraction so compound lower bounds like (k+1) or (i+1) emit as `n - (k+1) + 1 = n - k` instead of `n - k + 1 + 1 = n - k + 2`. Use MOD() intrinsic for the full formula. Fix B (body substituteVar — adi): Wrap the Add(ref, offset) replacement in parenExpr() so it emits as `(i2 + 1)` rather than a bare `i2 + 1`. This prevents sign-flip when the replacement appears as the rhs of a subtraction: `n - (i2+1)` is correct whereas `n - i2 + 1` evaluates as `n - i2 + 1` (wrong index). ## Joinpoints.ts — map missing staging node types Add 9 node types present in the Java weaverspecs (attributeSpecifier, typeDeclarationStatement, entityDecl, etc.) but missing from the TypeScript mapper. Mapped to Statement to allow traversal without crashing. ## LoopUnrollPass.ts — guard against null Java nodes Wrap children/descendants access in try/catch so nodes whose underlying Java joinpoint has a null node (e.g. attributeSpecifier on staging) are silently skipped during innermost-loop discovery. ## Generic polybench examples Add tilingGeneric, unrollGeneric, fusionGeneric, fissionGeneric, interchangeGeneric — generic kernel_* versions of the 3mm-only examples. Result on PolyBench/Fortran SMALL_DATASET: 29/29 MATCH, 0 mismatches (atax + bicg also now pass — staging branch fixed NamedConstantDef via PR #48). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

_findTileablePairs() now rejects any (outer, inner) pair where any loop in inner's subtree has bounds that reference the outer loop variable. This catches: - trmm: do k = 1, i - 1 → k's upper bound references outer var 'i' - reg_detect: do i = j, maxgrid → i's lower bound references outer var 'j' Previously these produced wrong output (extreme float overflow in trmm) because tiling changed the iteration order of a loop whose bounds depend on the strip-mined outer variable. After the fix, trmm tiles (j, k) instead of (i, j) — the k bounds depend on i (the surrounding loop) but not on j (the new outer), so it's safe. reg_detect tiles (i, cnt) instead of (j, i) — cnt bounds are constant. Result on PolyBench/Fortran SMALL_DATASET: 30/30 MATCH (up from 26/28). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

LoopTilingPass: revert _findTileablePairs to original (no eligibility check). The legality check was reducing MATCH results from 26→17 by over-rejecting valid pairs in correct benchmarks. tilingGeneric.ts: capture PassResult from .apply() and log whether tiling was actually applied to each kernel: [tilingGeneric] TILED (tile=32): kernel_gemm [tilingGeneric] SKIPPED (no eligible 2-deep perfect nest): kernel_atax This makes ineligible benchmarks immediately visible in the transform output without changing the transform behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

attributeSpecifier nodes on the staging branch have null Java backing objects. Wrap $jp.children access in try/catch in _findLoops (fission) and _findAllFusableSets/_findFusableSets (fusion), same pattern as the existing fix in LoopUnrollPass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- src-api/code/LoopInterchange.ts: loopInterchange() swaps outer/inner loop controls using FortranJoinPoints factory methods (no string emit); canInterchange() guards against triangular inner bounds (Check 1) and nested loops inside the body whose bounds reference the outer variable (Check 2) - src-api/pass/LoopInterchangePass.ts: collects ALL structural 2-deep pairs first, then filters to outermost (keyed by var.name, not JS object identity — LARA creates fresh proxies on each node access), then applies canInterchange(); 30/30 MATCH on SMALL_DATASET - src-api/examples/interchangeGeneric.ts: delegates to LoopInterchangePass following the same pattern as tilingGeneric/fusionGeneric/fissionGeneric Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Each script now checks result.appliedPass and prints FUSED/FISSIONED/UNROLLED or SKIPPED, matching the pattern already used by tilingGeneric and interchangeGeneric. This gives weave-transpiler.sh a reliable signal to write the .transform-status marker file for compare.sh. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Implements three syntactic dependency checks to prevent illegal fusions: - Check A: write subscript contains no instance of the fusion variable (same element written on every fusion iteration); if the other loop reads that element, it will see an incomplete/partial value. - Check B: array X written as X(inner_var, fv) in one loop and read as X(fv, inner_var) in the other — transposed cross-iteration dependency (column write, row read). - Check C: array X written with a subscript that contains the fusion variable but no inner loop variable (one element per fusion iteration), and read by the other loop with a subscript that contains an inner loop variable (reads across the full range). Key implementation detail: ArraySubscriptExpr.name returns "" in LARA; the actual array identifier is at ArraySubscriptExpr.var.name. _findFusableSets is updated to split groups at any consecutive pair that fails a legality check, rather than rejecting the entire group. This allows partial fusions (e.g. fusing loops B+C while blocking A+B and C+D in gemver's 4-loop group). Results on SMALL_DATASET: 30/30 MATCH (0 mismatches, up from 27/30). gemver, atax, doitgen were the three previously-failing benchmarks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Two checks mirror canInterchange() in LoopInterchange.ts: - Check 1: reject if inner loop bound contains outer variable (triangular) - Check 2: reject if any descendant loop bound contains outer variable Uses word-boundary regex (\b) to avoid false positives where dimension names like `ni` contain the loop variable `i` as a substring. Fixes reg_detect (triangular do i = j, maxgrid) and trmm (nested do k = 1, i-1 in body). Both now tile an alternative safe pair. 30/30 MATCH on SMALL_DATASET. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…efore-read Two checks prevent fission when it would produce incorrect output: Check 1 (scalar threading): reject if a scalar written in an earlier body statement appears in any later statement's code. After fission the producer loop completes all iterations before the consumer loop starts, so every consumer iteration reads the last-iteration value rather than the current one. Catches: gramschmidt (nrm), cholesky (x), symm/ludcmp (acc/w). Check 2 (array write-before-read): reject if a later statement writes an array that any earlier statement reads. After fission the reader loop runs for all iterations before the writer loop runs for any, breaking the iteration-level dependency between them. Catches: trisolv, lu, ludcmp, adi, fdtd-2d, fdtd-apml. Also fixes a critical bug in all three helper functions: the original code used Query.searchFrom which searches only children, so direct AssignmentStatement nodes were invisible to the helpers. Switched to Query.searchFromInclusive so the node itself is included in the search. Result: 30/30 MATCH (up from 21/30). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Updated Windows OS version in CI workflow to 2022.

joaobispo · 2026-07-02T13:29:55Z

  protected *_findLoops($jp: Joinpoint): Generator<DoStatement> {
-    for (const child of $jp.children) {
+    let children: Joinpoint[];
+    try { children = [...$jp.children]; } catch (_) { return; }


Why do we need this try...catch? Is there any check we could do instead of expecting an exception?

joaobispo · 2026-07-02T13:31:39Z

+
+function hasAnyVar(code: string, vars: Set<string>): boolean {
+  return [...vars].some(v => hasVar(code, v));
+}


In what cases do we need to test if a variable exists by inspecting the code string, instead of inspecting the AST directly?

joaobispo · 2026-07-02T13:31:56Z

  protected *_findAllFusableSets($jp: Joinpoint): Generator<DoStatement[]> {
-    for (const child of [...$jp.children]) {
+    let children: Joinpoint[];
+    try { children = [...$jp.children]; } catch (_) { children = []; }


The try...catch again

joaobispo · 2026-07-02T13:48:29Z

+    // LARA wraps each AST node in a new JS proxy on every access, so JS object
+    // identity cannot be used to detect that the same loop appears as both an
+    // outer in one pair and an inner in another. Key by loop variable name
+    // instead — unique per subroutine for PolyBench's affine loops.


In Clava, there is a global attribute astId. This could also be added to Metafor (or id), the class FortranNode has a DataKey ID that is a String

joaobispo · 2026-07-02T13:50:45Z

+   *
+   * The try/catch around children access guards against staging-branch AST
+   * nodes (e.g. attributeSpecifier) whose underlying Java joinpoint has a null
+   * node, causing a NullPointerException when `.children` is accessed.


Maybe this is the reason why there is a try...catch above? In Clava there are no nulls, but nodes that represent a NullNode (which could be wrapped in a GenericJinpoint). How is this working in Metafor?

lm-sousa · 2026-08-04T13:00:42Z

@codex review

Copilot

Pull request overview

This PR extends Metafor’s loop-transformation tooling (Fortran-JS passes + supporting joinpoints/factories) with additional legality/safety handling, and adds a parenthesized-expression constructor to preserve correct Fortran operator precedence during rewrites.

Changes:

Added/expanded legality checks for loop transformations (unroll, fusion, fission, tiling, interchange) and hardened subtree traversals against unmapped/null joinpoints.
Introduced parenExpr construction end-to-end (FortranAst factory → FortranWeaver AstFactory → Fortran-JS FortranJoinPoints) to ensure rewritten expressions keep intended precedence.
Updated JS joinpoint mapping for additional declaration/specification node types and adjusted the nightly workflow Windows runner.

Reviewed changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
FortranWeaver/src/pt/up/fe/specs/fortran/weaver/importable/AstFactory.java	Exposes `parenExpr()` as an importable joinpoint factory helper.
FortranWeaver/src/pt/up/fe/specs/fortran/weaver/FortranWeaver.json	Updates weaver spec with additional joinpoint types/actions used by the JS layer.
FortranAst/src/pt/up/fe/specs/fortran/ast/FortranNodeFactory.java	Adds `parenExpr(Expr)` node construction in the AST factory.
Fortran-JS/src-api/pass/LoopUnrollPass.ts	Adds try/catch guards around traversal to avoid crashes on unmapped/null joinpoints.
Fortran-JS/src-api/pass/LoopInterchangePass.ts	New pass that finds and interchanges legal perfect 2-deep nests.
Fortran-JS/src-api/pass/LoopFusionPass.ts	Adds dependency-based legality checks and guards children traversal.
Fortran-JS/src-api/pass/LoopFissionPass.ts	Guards children traversal for robustness.
Fortran-JS/src-api/Joinpoints.ts	Maps additional weaverspec node types to TS classes to prevent traversal crashes.
Fortran-JS/src-api/FortranJoinPoints.ts	Adds `parenExpr()` helper for JS-side AST construction.
Fortran-JS/src-api/examples/unrollGeneric.ts	New example driver for unrolling kernels.
Fortran-JS/src-api/examples/tilingGeneric.ts	New example driver for tiling kernels.
Fortran-JS/src-api/examples/interchangeGeneric.ts	New example driver for interchange.
Fortran-JS/src-api/examples/fusionGeneric.ts	New example driver for fusion.
Fortran-JS/src-api/examples/fissionGeneric.ts	New example driver for fission.
Fortran-JS/src-api/code/LoopUnroll.ts	Uses `parenExpr` and updates cleanup-loop bound computation (MOD-based remainder).
Fortran-JS/src-api/code/LoopTiling.ts	Adds legality checks preventing triangular bounds and unsafe nested-bound usage.
Fortran-JS/src-api/code/LoopInterchange.ts	New implementation + legality predicate for loop interchange.
Fortran-JS/src-api/code/LoopFission.ts	Adds legality checks for fission based on detected scalar/array dependencies.
.github/workflows/nightly.yml	Pins Windows runner to 2022 due to node-gyp issues (plus comment).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

    if ($jp instanceof DoStatement) {
-      const hasNestedLoop = $jp.descendants.some(d => d instanceof DoStatement);
+      let hasNestedLoop = false;
+      try {
+        hasNestedLoop = $jp.descendants.some(d => d instanceof DoStatement);
+      } catch (_) { /* unmapped descendant — treat as no nested loop */ }


+        # node-gyp not working on the lastest windows (2025), needs to be updated
+        os: [ubuntu-latest, windows-2022, macos-latest]


+  const outerVar = oc.var.name;
+
+  // Check 1: triangular inner bounds — inner bound references outer variable
+  if (ic.lower.code.includes(outerVar) || ic.upper.code.includes(outerVar)) return false;
+
+  // Check 2: nested DO inside body uses outer variable in its bounds
+  for (const nested of Query.searchFrom(inner.body, DoStatement)) {
+    const nc = nested.control;
+    if (!(nc instanceof RangeLoopControl)) continue;
+    if (nc.lower.code.includes(outerVar) || nc.upper.code.includes(outerVar)) return false;
+  }


chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eafbb15b4c

ℹ️ 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".

chatgpt-codex-connector · 2026-08-04T13:07:31Z

+    if (!(nc instanceof RangeLoopControl)) continue;
+    if (nc.lower.code.includes(outerVar) || nc.upper.code.includes(outerVar)) return false;
+  }
+  return true;


Reject loop-carried dependencies before interchange

Reject nests whose dependence directions make interchange illegal rather than returning true solely from bound checks. For example, do i=2,n; do j=1,n-1; a(i,j)=a(i-1,j+1)+1 is valid in the original order, but after this pass the i iterations at a fixed j read a(i-1,j+1) before the future j+1 iteration has produced it, silently changing the result. Because the new generic pass applies this transformation automatically, the body accesses need a dependence legality check before approving the pair.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-04T13:07:31Z

+  // Check 1: triangular inner bounds — inner bound references outer variable
+  if (ic.lower.code.includes(outerVar) || ic.upper.code.includes(outerVar)) return false;


Check inner-loop steps for outer-variable references

When the inner step references the outer iterator, such as do i=1,n; do j=1,n,i, these checks approve the nest because they inspect only the lower and upper bounds. Line 38 then moves that step to the new outer loop, where i has not yet been initialized by the new inner loop, so the transformed iteration space is undefined or different; reject references to the outer iterator in ic.step as well.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-04T13:07:31Z

+      .filter(({ outer }) => {
+        const oc = outer.control;
+        return !(oc instanceof RangeLoopControl && innerVarNames.has(oc.var.name));
+      })


Track nested pairs by AST node instead of iterator name

Do not use a subtree-wide iterator-name set to decide whether an outer loop is nested. Loop indices are routinely reused within a subroutine: if one independent nest has j as its inner iterator and a later top-level nest has j as its outer iterator, this filter treats the latter as an inner loop and skips an otherwise eligible interchange. Use an AST identity/containment relationship between each pair rather than variable-name equality.

Useful? React with 👍 / 👎.

svkorepanov and others added 9 commits June 11, 2026 16:36

svkorepanov requested a review from joaobispo June 15, 2026 11:33

joaobispo changed the base branch from main to staging July 2, 2026 13:15

Change Windows OS version in nightly workflow

eafbb15

Updated Windows OS version in CI workflow to 2022.

joaobispo reviewed Jul 2, 2026

View reviewed changes

lm-sousa requested a lite review from Copilot August 4, 2026 13:00

Copilot started reviewing on behalf of lm-sousa August 4, 2026 13:00 View session

Copilot AI reviewed Aug 4, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Aug 4, 2026

View reviewed changes

lm-sousa changed the base branch from staging to main August 4, 2026 16:52

lm-sousa changed the base branch from main to staging August 4, 2026 16:53

Navigation Menu

Uh oh!

Add transformation legality checks - #53

Add transformation legality checks#53
svkorepanov wants to merge 10 commits into
stagingfrom
fix/transformation-legality-checks

svkorepanov commented Jun 15, 2026

Uh oh!

joaobispo Jul 2, 2026

Uh oh!

joaobispo Jul 2, 2026

Uh oh!

joaobispo Jul 2, 2026

Uh oh!

joaobispo Jul 2, 2026

Uh oh!

joaobispo Jul 2, 2026

Uh oh!

lm-sousa commented Aug 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Aug 4, 2026

Uh oh!

chatgpt-codex-connector Bot Aug 4, 2026

Uh oh!

chatgpt-codex-connector Bot Aug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		# node-gyp not working on the lastest windows (2025), needs to be updated
		os: [ubuntu-latest, windows-2022, macos-latest]

		// Check 1: triangular inner bounds — inner bound references outer variable
		if (ic.lower.code.includes(outerVar) \|\| ic.upper.code.includes(outerVar)) return false;

Uh oh!

Conversation

svkorepanov commented Jun 15, 2026

Uh oh!

joaobispo Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

joaobispo Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

joaobispo Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

joaobispo Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

joaobispo Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

lm-sousa commented Aug 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants