Remove a false performance claim in addRowMultiple, and the dead macros behind it - #77
Merged
Merged
Conversation
The comment on it claimed that indexing mEntries directly instead would cost 14% of the whole matrix reduction, measured on MSVC 2012 in 2013 and never since. Its own author wrote "That does not make sense to me, but it is a fact none-the-less." It is not a fact on any compiler in use. Measured in 2026 with the loop built both ways and timed in interleaved runs against hyclic8-101-trimmed, yang1 and hilbertkunz1: GCC 11.4 -O2 on x86-64 emits different code with and without the local but times the two within -0.9% to +1.4%, inside the run-to-run noise on all three inputs. clang 21 on arm64 emits byte-identical object files either way, so there is nothing there to time at all. MSVC cannot build this code regardless -- stdinc.h has required C++17 since PR #60, and MSVC 2012 does not do complete C++11 -- so there is no compiler left for which the claim could still hold. Dropping it takes the assert with it. MATHICGB_ASSERT(entries + it.index() == &mEntries[it.index()]) only checked that vector::data() points where operator[] does, which is a statement about std::vector rather than about this code. Verified: 246/246 tests pass, hyclic8-101-trimmed.gb is unchanged, and the build with the local dropped times at -0.1% against the build with it, sd 0.6-1.0% over 15 interleaved reps. The manual unrolling below is a separate question and is left alone. It was measured in the same pass and does earn its keep: replacing it with a plain loop costs 8-12% on hyclic8-101-trimmed, though nothing on yang1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The restrict local in DenseRow::addRowMultiple, removed in the previous commit, was the only place it was ever used. All three definitions go, not just the one this build takes. The macro is defined once per compiler branch -- __restrict for MSVC, __restrict for GCC/clang, and empty for the fallback -- and removing only one branch would leave a macro that exists on some compilers and not others, which is worse than either keeping or removing it. stdinc.h is installed by both build systems, so this is nominally an API removal, as PR #60's FlattenNamespace change was. There is no ABI effect: a macro emits no symbol. Nothing outside the project has reason to use a MATHICGB_-prefixed compiler-portability macro, and anything that did would already have had to cope with it expanding to nothing on unrecognized compilers. Verified: 246/246 tests pass. Several neighbouring macros in the same blocks look equally unused, and some are visibly broken -- __attribute__(pure) is missing its inner parentheses and would not compile if anything expanded it. Auditing those is a separate pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MATHICGB_RESTRICT, removed in the previous commit, turned out not to be alone.
Auditing its neighbours in the same three compiler blocks found five more with
no uses anywhere in the project:
MATHICGB_ASSUME_AND_MAY_EVALUATE
MATHICGB_MUST_CHECK_RETURN_VALUE
MATHICGB_NOTHROW
MATHICGB_PURE
MATHICGB_RETURN_NO_ALIAS
What settles it is that in the GCC/clang branch none of the five would
compile if anything did use them. Four are written __attribute__(x) where
the attribute syntax needs __attribute__((x)):
#define MATHICGB_RETURN_NO_ALIAS __attribute__(malloc)
#define MATHICGB_NOTHROW __attribute__(nothrow)
#define MATHICGB_PURE __attribute__(pure)
#define MATHICGB_MUST_CHECK_RETURN_VALUE __attribute__(warn_unused_result)
and the fifth has its while(0) inside the do block rather than closing it.
Checked by expanding each one in a one-line translation unit against GCC
11.4: MATHICGB_PURE gives "error: expected '(' before 'pure'", its three
siblings give the same shape, and MATHICGB_ASSUME_AND_MAY_EVALUATE gives
"error: expected primary-expression before '}' token". So the first use of
any of them, on the compiler every current build uses, is a build failure.
They have been that way since 2013 and nothing noticed, because nothing uses
them. A facility nobody can adopt is not worth keeping; adding one back
correctly is two lines.
The MSVC spellings look right and the fallback branch defines them empty, but
all three copies go together -- leaving a macro that exists on some compilers
and not others is worse than either keeping or removing it. The five ///
comments in the MSVC block go too, since they document macros that no longer
exist.
stdinc.h is installed, so this is nominally an API removal, with no ABI
effect. Macaulay2 is the only known consumer and uses none of them: grepping
its tree at 169ac29181 for all six macro names returns nothing outside our own
submodule, and the only MATHICGB_* identifiers it references anywhere are
MATHICGB_LIBRARIES, MATHICGB_INCLUDE_DIR, MATHICGB_FOUND, MATHICGB_VERSION_
STRING, MATHICGB_DEBUG and MATHICGB_NO_TBB -- things it sets or probes for,
not attributes it consumes. It also cannot reach these definitions: it
includes only mathicgb.h and mathicgb/mtbb.hpp, neither of which includes
stdinc.h.
Two near neighbours are deliberately left alone, because the obvious grep
calls them dead and they are not. MATHICGB_ASSUME has no uses outside
stdinc.h but is the non-debug definition of MATHICGB_ASSERT further down the
file, so removing it would break all 898 assertions in release builds -- it
survives immediately above the first hunk here, while the macro whose comment
begins "As MATHICGB_ASSUME, but..." does not, which is correct but reads
asymmetrically. MATHICGB_CONCATENATE is used by
MATHICGB_CONCATENATE_AFTER_EXPANSION, which MATHICGB_UNIQUE needs.
Verified: 246/246 tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The comment on the
entrieslocal inDenseRow::addRowMultipleclaimed thatindexing
mEntriesdirectly instead would cost 14% of the whole matrixreduction. That was measured on MSVC 2012 in 2013 and never since; its own
author wrote "That does not make sense to me, but it is a fact none-the-less."
It is not a fact on any compiler in use. Measured with the loop built both ways
and timed in interleaved runs against
hyclic8-101-trimmed,yang1andhilbertkunz1:-O2, x86-64 emits different code with and without the local,and times the two within −0.9% to +1.4% — inside the run-to-run noise on all
three inputs.
nothing there to time at all.
MSVC cannot build this code regardless:
stdinc.hhas required C++17 sincePR #60, and MSVC 2012 does not do complete C++11. There is no compiler left for
which the claim could hold.
Removing the local leaves
MATHICGB_RESTRICTwith no users, and auditing itsneighbours in the same three compiler blocks found five more unused macros —
four of which have never been syntactically valid on GCC or clang
(
__attribute__(x)where the syntax needs__attribute__((x))), plus one withits
while(0)inside thedoblock instead of closing it. The first use of anyof them would be a build failure. They have been that way since 2013 and nothing
noticed, because nothing uses them.
Commits
Each builds standalone and passes the full suite, so the series is bisectable.
0190b09— drop the restrict local and the assert that only checkedvector::data()points whereoperator[]does6084a28— removeMATHICGB_RESTRICT, now unused555dec3— remove the five dead macrosOn the API surface
stdinc.his installed, so removing public macros is nominally an API removal,with no ABI effect. Macaulay2 is the only known consumer and uses none of them:
grepping its tree for all six names returns nothing outside the mathicgb
submodule, and the only
MATHICGB_*identifiers it references anywhere areMATHICGB_LIBRARIES,MATHICGB_INCLUDE_DIR,MATHICGB_FOUND,MATHICGB_VERSION_STRING,MATHICGB_DEBUGandMATHICGB_NO_TBB— things itsets or probes for, not attributes it consumes. It also cannot reach these
definitions: it includes only
mathicgb.handmathicgb/mtbb.hpp, neither ofwhich includes
stdinc.h.Deliberately left alone
MATHICGB_ASSUMEandMATHICGB_CONCATENATEalso have no uses outsidestdinc.h, and both are load-bearing.MATHICGB_ASSUMEis the non-debugdefinition of
MATHICGB_ASSERT, so removing it would break all 898 assertionsin release builds.
MATHICGB_CONCATENATEbacksMATHICGB_CONCATENATE_AFTER_EXPANSION, whichMATHICGB_UNIQUEneeds. Theasymmetry in commit 3 is intentional: the macro whose comment begins "As
MATHICGB_ASSUME, but…" goes while
MATHICGB_ASSUMEitself stays.The manual unrolling below the removed comment is a separate question and is
untouched. It was measured in the same pass and does earn its keep — replacing
it with a plain loop costs 8–12% on
hyclic8-101-trimmed, though nothing onyang1.Verification
246/246 tests pass at each of the three commits.
hyclic8-101-trimmed.gbisbyte-identical to before. The build with the local dropped times at −0.1%
against the build with it, sd 0.6–1.0% over 15 interleaved reps.
🤖 Generated with Claude Code