Add short forms of Load::make and Store::make - #9247
Open
abadams wants to merge 3 commits into
Open
Conversation
Load::make and Store::make take a lot of arguments, most of which are
empty or trivial for the many sites that construct an access to an
internal buffer rather than deriving one from an existing node. Add
three-argument overloads that supply the rest:
Load::make(type, name, index)
Store::make(name, value, index)
These use no image or parameter, an all-true predicate, a
default-constructed ModulusRemainder, and is_streaming = false, which is
exactly what the 57 converted call sites were already passing. The
predicate is guaranteed equivalent by the lane-count assertions already
in the long form.
Also removes the default argument for is_streaming from the long form of
both. A trailing defaulted argument silently turns "I forgot this field"
into "false", which is only safe as long as nobody uses make to rebuild
an existing node. That has already gone wrong twice: the padding argument
to Allocate::make was dropped when optimize_hexagon_instructions rebuilt
an allocation, and is_streaming was dropped when StageStridedLoads was
merged with the branch that introduced it. Neither was caught by a test.
Now that Load::with and Store::with cover the rebuilding cases and the
short forms cover the trivial ones, the long form only remains where the
caller is constructing something specific, and can afford to say so.
The 27 sites that relied on the default now pass false explicitly. This
preserves existing behavior everywhere, including the five places that
synthesize a Load from a Store to analyze an atomic read-modify-write,
which could propagate the Store's is_streaming but do not.
Co-Authored-By: Claude Opus 5 <[email protected]>
alexreinking
approved these changes
Jul 28, 2026
IRGraphCXXPrinter emits C++ that reconstructs an IR graph, and static asserts that the fields it extracts for each node match one of that node's make() signatures. It was not extracting is_streaming for Load and Store, which only compiled because make() defaulted that argument. Now that the default is gone the assert fires, so extract it. This also makes the printed reproducer faithful for streaming accesses, which previously came back as non-streaming. Co-Authored-By: Claude Opus 5 <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## abadams/remake_ir_nodes #9247 +/- ##
==========================================================
Coverage ? 70.35%
==========================================================
Files ? 255
Lines ? 78677
Branches ? 18833
==========================================================
Hits ? 55352
Misses ? 17769
Partials ? 5556 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
2 participants
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.
Load::make and Store::make take a lot of arguments, most of which are empty or trivial for the many sites that construct an access to an internal buffer rather than deriving one from an existing node. Add three-argument overloads that supply the rest:
These use no image or parameter, an all-true predicate, a default-constructed ModulusRemainder, and is_streaming = false, which is exactly what the 57 converted call sites were already passing. The predicate is guaranteed equivalent by the lane-count assertions already in the long form.
Also removes the default argument for is_streaming from the long form of both. A trailing defaulted argument silently turns "I forgot this field" into "false", which is only safe as long as nobody uses make to rebuild an existing node. That has already gone wrong twice: the padding argument to Allocate::make was dropped when optimize_hexagon_instructions rebuilt an allocation, and is_streaming was dropped when StageStridedLoads was merged with the branch that introduced it. Neither was caught by a test. Now that Load::with and Store::with cover the rebuilding cases and the short forms cover the trivial ones, the long form only remains where the caller is constructing something specific, and can afford to say so.
The 27 sites that relied on the default now pass false explicitly. This preserves existing behavior everywhere, including the five places that synthesize a Load from a Store to analyze an atomic read-modify-write, which could propagate the Store's is_streaming but do not.