Skip to content

coll: read algorithm CVARs through a pointer table - #8039

Merged
hzhou merged 2 commits into
pmodels:mainfrom
hzhou-muse:coll_cvar_ptr
Oct 21, 2026
Merged

hzhou merged 2 commits into
pmodels:mainfrom
hzhou-muse:coll_cvar_ptr

Conversation

@hzhou-muse

Copy link
Copy Markdown
Contributor

Pull Request Description

MPIR_Coll_cvar_table copied the algorithm CVAR values once during init. As a result, writing an algorithm CVAR such as MPIR_CVAR_BCAST_INTRA_ALGORITHM with MPI_T_cvar_write had no effect, even though the algorithm's parameter CVARs (TREE_KVAL, etc.) are read on every call. Also, an invalid value reached MPIR_Assert(0) in MPIR_Coll_cvar_to_algo_id, so a release build silently ran algorithm id 0.

The table now holds pointers to the CVARs, set by a constant initializer from gen_coll.py. A value written through MPI_T takes effect in the next collective call.

  • MPIR_Coll_cvar_to_algo_id returns the END id for a value that is not one of the collective's algorithms, and MPIR_Coll_json returns an error (**collcvarval) for it.
  • The malloc/free of the table and its init code are removed.
  • doc/wiki/design/Collective_Selection.md is updated.

Author Checklist

  • Provide Description
  • Commits Follow Good Practice
  • Passes All Tests
  • Contribution Agreement

🤖 Generated with Claude Code

@hzhou-muse

hzhou-muse commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

Tested locally with a debug build (--enable-g=dbg, ch4:ofi). There were no compiler warnings in mpir_coll.c or coll_impl.c.

The test program changes MPIR_CVAR_BCAST_INTRA_ALGORITHM with MPI_T_cvar_write between bcasts, run on 4 ranks with MPIR_CVAR_DUMP_COLL_ALGO_COUNTERS=0 (rank 0 prints the counters):

Step Result
Default (auto), 1 bcast MPIR_Bcast_intra_binomial
Write scatter_ring_allgather, 3 bcasts 3 × MPIR_Bcast_intra_scatter_ring_allgather
Write back to auto, 1 bcast MPIR_Bcast_intra_binomial (2 total)
Write 9999, 1 bcast Invalid bcast-intra algorithm CVAR value (9999) returned as an error, instead of MPIR_Assert(0)

Side note, not changed by this PR: MPICH doesn't register an MPI_T_enum for enum CVARs, so MPI_T_cvar_get_info returns no enum handle for the algorithm CVARs, and a tool can set them only by raw number (6 for scatter_ring_allgather here). Registering the enums in extractcvars would make runtime tuning usable from tools; that would be a separate PR.

@hzhou-muse

hzhou-muse commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

test:mpich/ch3/tcp
test:mpich/ch4/ofi

✔️

@pmodels pmodels deleted a comment from hzhou-muse Oct 20, 2026
hzhou-muse and others added 2 commits October 20, 2026 19:44
The scan smp and iscan sched_smp algorithms are restricted to commutative
ops, but MPIR_Csel_op_is_commutative had no case for scan, so forcing either
algorithm through its CVAR hit MPIR_Assert(0).

Co-Authored-By: Claude Opus 5.5 <[email protected]>
MPIR_Coll_cvar_table now holds pointers to the algorithm CVARs, set by a
generated constant initializer, so a value written with MPI_T_cvar_write
takes effect in the next collective call. MPIR_Coll_cvar_to_algo_id
returns the END id for a value that is not one of the collective's
algorithms, and MPIR_Coll_json returns an error for it.

Previously the table copied the CVAR values at init, so MPI_T writes to
algorithm CVARs were ignored, and an invalid value hit MPIR_Assert(0),
which in a release build silently ran algorithm id 0.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@hzhou
hzhou merged commit cbf31df into pmodels:main Oct 21, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants