Skip to content

Commit 7bbeab7

Browse files
committed
improve: sharper Verrou dd_line attribution via source filtering and function scoping
Three changes to reduce spurious dd_line reports pointing at do-loop boundaries, #:endfor lines, or inlined-function call sites: 1. _CONTROL_FLOW_RE + _is_arithmetic_loc: post-filter dd_line results by reading the actual source line; skip pure control-flow delimiters (end do, end if, #:endfor, $:END_GPU_PARALLEL_LOOP, etc.) that share DWARF info with the preceding arithmetic due to template expansion or inlining. 2. restrict_syms in _write_dd_run_sh / _run_dd_line: when dd_sym has already identified responsible functions, dd_line_run.sh embeds --source-function=<sym> flags so Verrou only perturbs FP ops inside those functions, narrowing the bisection space. 3. FFLAGS=-fno-inline in fp-stability.yml: prevents gfortran from inlining small helpers into their callers, so DWARF correctly attributes arithmetic to the callee source line rather than the caller loop header.
1 parent b40c0bf commit 7bbeab7

2 files changed

Lines changed: 116 additions & 14 deletions

File tree

.github/workflows/fp-stability.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,12 @@ jobs:
101101
run: ~/.local/verrou/bin/valgrind --version
102102

103103
- name: Build MFC (debug, serial)
104+
# FFLAGS=-fno-inline prevents gfortran from inlining small functions into
105+
# their callers. Without it, DWARF debug info attributes inlined ops to
106+
# the caller's line (often a do-loop header), making Verrou dd_line point
107+
# to loop boundaries instead of the actual arithmetic.
108+
env:
109+
FFLAGS: "-fno-inline"
104110
run: ./mfc.sh build -t pre_process simulation --no-mpi --debug -j"$(nproc)"
105111

106112
- name: Run FP Stability Suite

toolchain/mfc/fp_stability.py

Lines changed: 110 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,55 @@
7979
# Matches the first "at" frame in a Valgrind stack trace: "(file.fpp:LINE)".
8080
_VGFRAME_RE = re.compile(r"\(([^):]+\.(?:fpp|f90|F90|c|cpp))\s*:(\d+)\)")
8181

82+
# Lines that are clearly control-flow delimiters rather than arithmetic.
83+
# dd_line sometimes reports these when the responsible arithmetic is on the
84+
# preceding line but shares DWARF debug info with the delimiter (e.g. loop
85+
# boundaries in #:for-expanded code, or inlined functions at call sites).
86+
_CONTROL_FLOW_RE = re.compile(
87+
r"^\s*("
88+
r"end\s+(do|if|select|where|forall|subroutine|function|module|program|block)\b"
89+
r"|do\s+\w+\s*=\s*[\w,\s]+" # naked do-loop header (no arithmetic)
90+
r"|\$:END_GPU\w+" # fypp GPU macro closers
91+
r"|#:end\w*" # fypp directive closers (#:endfor, #:enddef, etc.)
92+
r"|\s*!\s*$" # comment-only lines
93+
r"|\s*$" # blank lines
94+
r")",
95+
re.IGNORECASE,
96+
)
97+
98+
99+
def _read_source_line(fname: str, lineno: int) -> str:
100+
"""Return the raw source line at lineno (1-based), or '' if unavailable."""
101+
if os.path.isabs(fname) and os.path.isfile(fname):
102+
candidates = [fname]
103+
else:
104+
candidates = glob.glob(os.path.join(MFC_ROOT_DIR, "src", "**", os.path.basename(fname)), recursive=True)
105+
if not candidates:
106+
return ""
107+
try:
108+
with open(candidates[0]) as fh:
109+
lines = fh.readlines()
110+
return lines[lineno - 1] if 0 < lineno <= len(lines) else ""
111+
except OSError:
112+
return ""
113+
114+
115+
def _is_arithmetic_loc(fname: str, start: int, end: int) -> bool:
116+
"""Return True if any line in [start, end] contains non-trivial arithmetic.
117+
118+
Filters out loop delimiters and fypp directive lines that dd_line sometimes
119+
reports when the responsible arithmetic shares DWARF info with its enclosing
120+
control-flow boundary (inlining, #:for template expansion, etc.).
121+
Returns True (keep) when uncertain so we never silently drop real hotspots.
122+
"""
123+
for lineno in range(start, end + 1):
124+
line = _read_source_line(fname, lineno)
125+
if not line:
126+
return True # can't read — keep to be safe
127+
if not _CONTROL_FLOW_RE.match(line):
128+
return True
129+
return False
130+
82131

83132
def _get_source_context(fname: str, lineno: int, context: int = 2) -> str:
84133
"""Return a annotated source snippet around lineno, or '' if file not found.
@@ -649,7 +698,7 @@ def _run_vprec_sweep(case: dict, verrou_bin: str, sim_bin: str, work_dir: str, r
649698
return results
650699

651700

652-
def _write_dd_run_sh(path: str, verrou_bin: str, sim_bin: str, ic_dir: str):
701+
def _write_dd_run_sh(path: str, verrou_bin: str, sim_bin: str, ic_dir: str, restrict_syms: list = None):
653702
"""Generate dd_run.sh for verrou_dd_sym / verrou_dd_line.
654703
655704
verrou_dd_* calls: dd_run.sh RUNDIR and injects function/line exclusion via
@@ -659,7 +708,15 @@ def _write_dd_run_sh(path: str, verrou_bin: str, sim_bin: str, ic_dir: str):
659708
environment — we honour that so the reference is a stable nearest-rounding baseline
660709
to compare against. CLI --rounding-mode would override the env var and break the
661710
reference, so we pass the mode via ${VERROU_ROUNDING_MODE:-float} instead.
711+
712+
restrict_syms: when provided (a list of function names from dd_sym), the script
713+
adds --source-function flags so Verrou only perturbs FP ops inside those functions.
714+
This narrows dd_line's search and avoids spurious template-expansion attributions.
662715
"""
716+
sym_flags = ""
717+
if restrict_syms:
718+
sym_flags = " ".join(f"--source-function={s!r}" for s in restrict_syms)
719+
663720
content = textwrap.dedent(f"""\
664721
#!/usr/bin/env bash
665722
# Generated by mfc.sh fp-stability — do not edit by hand.
@@ -684,7 +741,7 @@ def _write_dd_run_sh(path: str, verrou_bin: str, sim_bin: str, ic_dir: str):
684741
ROUND="${{VERROU_ROUNDING_MODE:-float}}"
685742
686743
cd "$TMPDIR_RUN"
687-
"$VERROU_BIN" --tool=verrou --error-limit=no --rounding-mode="$ROUND" "$SIM_BIN"
744+
"$VERROU_BIN" --tool=verrou --error-limit=no --rounding-mode="$ROUND" {sym_flags} "$SIM_BIN"
688745
rc=$?
689746
690747
[ -d "$TMPDIR_RUN/D" ] && cp -a "$TMPDIR_RUN/D/." "$RUNDIR/"
@@ -741,10 +798,17 @@ def _dd_env(verrou_bin: str) -> dict:
741798

742799

743800
def _parse_rddmin_locs(summary_path: str) -> list:
744-
"""Extract [(rel_path, start_line, end_line)] from a dd_line rddmin_summary."""
801+
"""Extract [(rel_path, start_line, end_line)] from a dd_line rddmin_summary.
802+
803+
Filters out locations whose source lines are pure control-flow delimiters
804+
(loop boundaries, fypp directive closers, blank/comment lines). These can
805+
appear when the responsible arithmetic shares DWARF debug info with an
806+
enclosing boundary due to inlining or #:for template expansion.
807+
"""
745808
if not os.path.isfile(summary_path):
746809
return []
747810
locs = []
811+
skipped = []
748812
with open(summary_path) as fh:
749813
for line in fh:
750814
m = _LOC_RE.search(line)
@@ -759,7 +823,14 @@ def _parse_rddmin_locs(summary_path: str) -> list:
759823
rel = path
760824
except ValueError:
761825
rel = path
762-
locs.append((rel.replace("\\", "/"), start, end))
826+
rel = rel.replace("\\", "/")
827+
if _is_arithmetic_loc(path, start, end):
828+
locs.append((rel, start, end))
829+
else:
830+
skipped.append((rel, start, end))
831+
for rel, start, end in skipped:
832+
loc = f"{rel}:{start}" if start == end else f"{rel}:{start}-{end}"
833+
cons.print(f" [dim]dd_line: skipped control-flow boundary {loc}[/dim]")
763834
return locs
764835

765836

@@ -821,24 +892,37 @@ def _run_dd_sym(case: dict, verrou_bin: str, sim_bin: str, work_dir: str, log_di
821892
return _parse_rddmin_syms(os.path.join(dd_dir, "dd.sym", "rddmin_summary"))
822893

823894

824-
def _run_dd_line(case: dict, verrou_bin: str, sim_bin: str, work_dir: str, log_dir: str, threshold: float = None) -> list:
825-
"""Run verrou_dd_line; return list of (rel_path, start_line, end_line) tuples."""
895+
def _run_dd_line(
896+
case: dict,
897+
verrou_bin: str,
898+
sim_bin: str,
899+
work_dir: str,
900+
log_dir: str,
901+
threshold: float = None,
902+
restrict_syms: list = None,
903+
) -> list:
904+
"""Run verrou_dd_line; return list of (rel_path, start_line, end_line) tuples.
905+
906+
restrict_syms: function names from dd_sym. When provided, dd_run.sh passes
907+
--source-function=<sym> flags to Verrou so perturbation is restricted to those
908+
functions. This narrows the bisection space and avoids spurious attributions
909+
to template-shared or inlined code outside the responsible functions.
910+
"""
826911
dd_bin = _find_dd_line(verrou_bin)
827912
if not dd_bin:
828913
cons.print(" [dim]verrou_dd_line not found; skipping line-level debug[/dim]")
829914
return []
830915

831916
dd_dir = os.path.join(log_dir, case["name"])
832917
os.makedirs(dd_dir, exist_ok=True)
833-
dd_run_sh = os.path.join(dd_dir, "dd_run.sh")
918+
# Always write a fresh dd_run.sh for dd_line so --source-function flags are included.
919+
dd_run_sh = os.path.join(dd_dir, "dd_line_run.sh")
834920
dd_cmp_py = os.path.join(dd_dir, "dd_cmp.py")
835921
effective_threshold = threshold if threshold is not None else case["threshold"]
836-
if not os.path.isfile(dd_run_sh):
837-
_write_dd_run_sh(dd_run_sh, verrou_bin, sim_bin, work_dir)
838-
_write_dd_cmp_py(dd_cmp_py, case["compare"], effective_threshold)
839-
else:
840-
# dd_sym already wrote dd_cmp.py with its threshold; rewrite with ours if different
841-
_write_dd_cmp_py(dd_cmp_py, case["compare"], effective_threshold)
922+
_write_dd_run_sh(dd_run_sh, verrou_bin, sim_bin, work_dir, restrict_syms=restrict_syms)
923+
_write_dd_cmp_py(dd_cmp_py, case["compare"], effective_threshold)
924+
if restrict_syms:
925+
cons.print(f" [dim]dd_line restricted to {len(restrict_syms)} dd_sym function(s)[/dim]")
842926
_run_dd_tool(dd_bin, dd_dir, dd_run_sh, dd_cmp_py, _dd_env(verrou_bin), "dd_line.log", "dd.line", "verrou_dd_line")
843927
return _parse_rddmin_locs(os.path.join(dd_dir, "dd.line", "rddmin_summary"))
844928

@@ -954,7 +1038,19 @@ def _run_case(
9541038
cons.print(f" [bold yellow]dd_sym error[/bold yellow]: {exc}")
9551039
if dd_threshold > 0 and run_dd_line:
9561040
try:
957-
result["dd_line_locs"] = _run_dd_line(case, verrou_bin, sim_bin, work_dir, log_dir, threshold=dd_threshold)
1041+
# Seed dd_line with dd_sym results so perturbation is restricted
1042+
# to the responsible functions, avoiding spurious attributions from
1043+
# template-expansion or inlining outside those functions.
1044+
restrict = result["dd_sym_syms"] if result["dd_sym_syms"] else None
1045+
result["dd_line_locs"] = _run_dd_line(
1046+
case,
1047+
verrou_bin,
1048+
sim_bin,
1049+
work_dir,
1050+
log_dir,
1051+
threshold=dd_threshold,
1052+
restrict_syms=restrict,
1053+
)
9581054
except Exception as exc:
9591055
cons.print(f" [bold yellow]dd_line error[/bold yellow]: {exc}")
9601056

0 commit comments

Comments
 (0)