Skip to content

Propagate to exact types in struct-utils.h #7889

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Sep 13, 2025
Merged

Propagate to exact types in struct-utils.h #7889

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
20e051a
Propate to exact types in struct-utils.h
tlively Sep 6, 2025
df76cfe
Merge branch 'main' into exact-cfp-fix
tlively Sep 8, 2025
f35f60d
Merge branch 'main' into exact-cfp-fix
tlively Sep 8, 2025
be904ae
[NFC] Add tests for missing exact CFP optimizations
tlively Sep 8, 2025
edb1d95
Merge branch 'cfp-missing-opts-tests' into exact-cfp-fix
tlively Sep 8, 2025
65eff0c
udpate test
tlively Sep 8, 2025
fe62381
Merge branch 'main' into exact-cfp-fix
tlively Sep 9, 2025
a310e7f
fix
tlively Sep 9, 2025
c567e37
comment
tlively Sep 9, 2025
7d0445e
Comment about convenience subscripting.
tlively Sep 9, 2025
89751b1
lambda
tlively Sep 9, 2025
49dcfc7
comment on exact propagation
tlively Sep 9, 2025
80a65e1
Merge branch 'main' into exact-cfp-fix
tlively Sep 9, 2025
56720bc
Merge branch 'main' into exact-cfp-fix
tlively Sep 10, 2025
6403073
update comment
tlively Sep 10, 2025
04fe19a
Merge branch 'main' into exact-cfp-fix
tlively Sep 11, 2025
13b7a0f
Merge branch 'main' into exact-cfp-fix
tlively Sep 12, 2025
1d61606
remove complexity of avoiding exactness
tlively Sep 12, 2025
fe63302
expand comment
tlively Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 74 additions & 41 deletions src/ir/struct-utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include "ir/properties.h"
#include "ir/subtypes.h"
#include "wasm-type.h"
#include "wasm.h"

namespace wasm {
Expand Down Expand Up @@ -88,19 +89,24 @@ template<typename T> struct StructValues : public std::vector<T> {
// Also provides a combineInto() helper that combines one map into another. This
// depends on the underlying T defining a combine() method.
template<typename T>
struct StructValuesMap : public std::unordered_map<HeapType, StructValues<T>> {
struct StructValuesMap
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth explaining in the comment what the meaning of the mapping is, that is, what do the exact and inexact entries mean.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

: public std::unordered_map<std::pair<HeapType, Exactness>, StructValues<T>> {
// When we access an item, if it does not already exist, create it with a
// vector of the right length for that type.
StructValues<T>& operator[](HeapType type) {
assert(type.isStruct());
StructValues<T>& operator[](std::pair<HeapType, Exactness> type) {
assert(type.first.isStruct());
auto inserted = this->insert({type, {}});
auto& values = inserted.first->second;
if (inserted.second) {
values.resize(type.getStruct().fields.size());
values.resize(type.first.getStruct().fields.size());
}
return values;
}

StructValues<T>& operator[](HeapType type) {
return (*this)[{type, Inexact}];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we disallow this operator? Or, are we not risking users getting the less-precise version by default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here was to allow other users of these utilities to keep using them as-is without changes. Otherwise there would be a lot of mechanical changes here just adding Inexact in a bunch of places in various passes.


void combineInto(StructValuesMap<T>& combinedInfos) const {
for (auto& [type, info] : *this) {
for (Index i = 0; i < info.size(); i++) {
Expand All @@ -113,7 +119,8 @@ struct StructValuesMap : public std::unordered_map<HeapType, StructValues<T>> {
void dump(std::ostream& o) {
o << "dump " << this << '\n';
for (auto& [type, vec] : (*this)) {
o << "dump " << type << " " << &vec << ' ';
o << "dump " << type.first << (type.second == Exact ? " exact " : " ")
<< &vec << ' ';
for (auto x : vec) {
x.dump(o);
o << " ";
Expand Down Expand Up @@ -203,7 +210,8 @@ struct StructScanner
// Note writes to all the fields of the struct.
auto heapType = type.getHeapType();
auto& fields = heapType.getStruct().fields;
auto& infos = functionNewInfos[this->getFunction()][heapType];
auto ht = std::make_pair(heapType, Exact);
auto& infos = functionNewInfos[this->getFunction()][ht];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can remove functionNewInfos entirely?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, probably. I can look at this in a follow-up.

for (Index i = 0; i < fields.size(); i++) {
if (curr->isWithDefault()) {
self().noteDefault(fields[i].type, heapType, i, infos[i]);
Expand All @@ -224,11 +232,12 @@ struct StructScanner
}

// Note a write to this field of the struct.
noteExpressionOrCopy(curr->value,
type.getHeapType(),
curr->index,
functionSetGetInfos[this->getFunction()]
[type.getHeapType()][curr->index]);
auto ht = std::make_pair(type.getHeapType(), type.getExactness());
noteExpressionOrCopy(
curr->value,
type.getHeapType(),
curr->index,
functionSetGetInfos[this->getFunction()][ht][curr->index]);
}

void visitStructGet(StructGet* curr) {
Expand All @@ -237,11 +246,11 @@ struct StructScanner
return;
}

auto heapType = type.getHeapType();
auto ht = std::make_pair(type.getHeapType(), type.getExactness());
auto index = curr->index;
self().noteRead(heapType,
self().noteRead(type.getHeapType(),
index,
functionSetGetInfos[this->getFunction()][heapType][index]);
functionSetGetInfos[this->getFunction()][ht][index]);
}

void visitStructRMW(StructRMW* curr) {
Expand All @@ -251,9 +260,9 @@ struct StructScanner
}

auto heapType = type.getHeapType();
auto ht = std::make_pair(heapType, type.getExactness());
auto index = curr->index;
auto& info =
functionSetGetInfos[this->getFunction()][type.getHeapType()][index];
auto& info = functionSetGetInfos[this->getFunction()][ht][index];

if (curr->op == RMWXchg) {
// An xchg is really like a read and write combined.
Expand All @@ -274,9 +283,9 @@ struct StructScanner
}

auto heapType = type.getHeapType();
auto ht = std::make_pair(heapType, type.getExactness());
auto index = curr->index;
auto& info =
functionSetGetInfos[this->getFunction()][type.getHeapType()][curr->index];
auto& info = functionSetGetInfos[this->getFunction()][ht][index];

// A cmpxchg is like a read and conditional write.
self().noteRead(heapType, index, info);
Expand Down Expand Up @@ -310,11 +319,12 @@ struct StructScanner
return;
}
auto heapType = type.getHeapType();
auto ht = std::make_pair(heapType, type.getExactness());
if (heapType.isStruct()) {
// Any subtype of the reference here may be read from.
self().noteRead(heapType,
DescriptorIndex,
functionSetGetInfos[this->getFunction()][heapType].desc);
functionSetGetInfos[this->getFunction()][ht].desc);
return;
}
}
Expand Down Expand Up @@ -372,13 +382,19 @@ template<typename T> class TypeHierarchyPropagator {
// Propagate given a StructValuesMap, which means we need to take into
// account fields.
void propagateToSuperTypes(StructValuesMap<T>& infos) {
propagate(infos, false, true);
propagate(infos, false, true, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this defaulting to exact while the others have new variants for that, and default to inexact?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Propagating up never propagates to exact types, so its behavior does not depend on the includeExact flag. Even though they would have identical behavior, I think true is a little more honest than false because it can still propagate from exact types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Propagating up never propagates to exact types,

But propagating down does? Why is there this asymmetry?

(What I feel might be happening is that this class left "reasoning" to the users, that is, it provided low-level "propagate here or there", and the user was responsible for picking which made sense for their use case. It's not obvious to me why this is asymmetrical so I wonder if this is anticipating certain use cases. If so that might be fine but could be documented perhaps.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because exact types are subtypes of their inexact counterparts, but have no subtypes themselves no matter how many subtypes their inexact counterparts have. This entirely due to the structure of the type relationships, not for any particular use cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see, so includeExact means literally "include Exact when looking at subtypes", not "take exactness into account when propagating"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, pretty much.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, perhaps a comment with that? The wrong meaning was my initial impression.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
void propagateToSubTypes(StructValuesMap<T>& infos) {
propagate(infos, true, false);
propagate(infos, true, false, false);
}
void propagateToSubTypesWithExact(StructValuesMap<T>& infos) {
propagate(infos, true, false, true);
}
void propagateToSuperAndSubTypes(StructValuesMap<T>& infos) {
propagate(infos, true, true);
propagate(infos, true, true, false);
}
void propagateToSuperAndSubTypesWithExact(StructValuesMap<T>& infos) {
propagate(infos, true, true, true);
}

// Propagate on a simpler map of structs and infos (that is, not using
Expand All @@ -398,46 +414,63 @@ template<typename T> class TypeHierarchyPropagator {
private:
void propagate(StructValuesMap<T>& combinedInfos,
bool toSubTypes,
bool toSuperTypes) {
UniqueDeferredQueue<HeapType> work;
for (auto& [type, _] : combinedInfos) {
work.push(type);
bool toSuperTypes,
bool includeExact) {
UniqueDeferredQueue<std::pair<HeapType, Exactness>> work;
for (auto& [ht, _] : combinedInfos) {
work.push(ht);
}
while (!work.empty()) {
auto type = work.pop();
auto& infos = combinedInfos[type];
auto [type, exactness] = work.pop();
auto& infos = combinedInfos[{type, exactness}];

if (toSuperTypes) {
// Propagate shared fields to the supertype.
if (auto superType = type.getDeclaredSuperType()) {
auto& superInfos = combinedInfos[*superType];
auto& superFields = superType->getStruct().fields;
for (Index i = 0; i < superFields.size(); i++) {
// Propagate shared fields to the supertype, which may be the inexact
// version of the same type.
std::optional<std::pair<HeapType, Exactness>> super;
if (exactness == Exact) {
super = {type, Inexact};
} else if (auto superType = type.getDeclaredSuperType()) {
super = {*superType, Inexact};
}
if (super) {
auto& superInfos = combinedInfos[*super];
const auto& superFields = &super->first.getStruct().fields;
for (Index i = 0; i < superFields->size(); i++) {
if (superInfos[i].combine(infos[i])) {
work.push(*superType);
work.push(*super);
}
}
// Propagate the descriptor to the super, if the super has one.
if (superType->getDescriptorType() &&
if (super->first.getDescriptorType() &&
superInfos.desc.combine(infos.desc)) {
work.push(*superType);
work.push(*super);
}
}
}

if (toSubTypes) {
// Propagate shared fields to the subtypes.
// Propagate shared fields to the subtypes, which may just be the exact
// version of the same type.
auto numFields = type.getStruct().fields.size();
for (auto subType : subTypes.getImmediateSubTypes(type)) {
auto& subInfos = combinedInfos[subType];
std::vector<std::pair<HeapType, Exactness>> subs;
if (includeExact && exactness == Inexact) {
subs = {{type, Exact}};
} else {
for (auto subType : subTypes.getImmediateSubTypes(type)) {
subs.emplace_back(subType, Inexact);
}
}
for (auto sub : subs) {
auto& subInfos = combinedInfos[sub];
for (Index i = 0; i < numFields; i++) {
if (subInfos[i].combine(infos[i])) {
work.push(subType);
work.push(sub);
}
}
// Propagate the descriptor.
if (subInfos.desc.combine(infos.desc)) {
work.push(subType);
work.push(sub);
}
}
}
Expand Down
19 changes: 11 additions & 8 deletions src/passes/ConstantFieldPropagation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@

#include "ir/bits.h"
#include "ir/gc-type-utils.h"
#include "ir/module-utils.h"
#include "ir/possible-constant.h"
#include "ir/struct-utils.h"
#include "ir/utils.h"
Expand Down Expand Up @@ -114,8 +113,10 @@ struct FunctionOptimizer : public WalkerPass<PostWalker<FunctionOptimizer>> {
return heapType;
}

PossibleConstantValues getInfo(HeapType type, Index index) {
if (auto it = propagatedInfos.find(type); it != propagatedInfos.end()) {
PossibleConstantValues
getInfo(HeapType type, Exactness exactness, Index index) {
if (auto it = propagatedInfos.find({type, exactness});
it != propagatedInfos.end()) {
// There is information on this type, fetch it.
return it->second[index];
}
Expand Down Expand Up @@ -177,7 +178,8 @@ struct FunctionOptimizer : public WalkerPass<PostWalker<FunctionOptimizer>> {
// Find the info for this field, and see if we can optimize. First, see if
// there is any information for this heap type at all. If there isn't, it is
// as if nothing was ever noted for that field.
PossibleConstantValues info = getInfo(heapType, index);
PossibleConstantValues info =
getInfo(heapType, ref->type.getExactness(), index);
if (!info.hasNoted()) {
// This field is never written at all. That means that we do not even
// construct any data of this type, and so it is a logic error to reach
Expand Down Expand Up @@ -282,7 +284,7 @@ struct FunctionOptimizer : public WalkerPass<PostWalker<FunctionOptimizer>> {
return;
}

auto iter = rawNewInfos.find(type);
auto iter = rawNewInfos.find({type, Exact});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rawNewInfos is always exact, though. This is what my last revision in the other PR uses directly.

They are always exact because they are info from struct.new, which always has an exact type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I think the idea in this PR is good! I'll read the code in detail monday.

But my point is that CFP already has exact info, in the form of rawNewInfos. We can probably remove that entirely, in this PR. And for the immediate fix, the latest version of my other PR is just a few lines, basically simply using rawNewInfos in a second place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have follow-up PRs removing rawNewInfos and making a few other improvements to CFP.

if (iter == rawNewInfos.end()) {
// This type has no struct.news, so we can ignore it: it is abstract.
return;
Expand Down Expand Up @@ -446,7 +448,8 @@ struct PCVScanner
void noteCopy(HeapType type, Index index, PossibleConstantValues& info) {
// Note copies, as they must be considered later. See the comment on the
// propagation of values below.
functionCopyInfos[getFunction()][type][index] = true;
// TODO: Take into account exactness here.
functionCopyInfos[getFunction()][{type, Inexact}][index] = true;
}

void noteRead(HeapType type, Index index, PossibleConstantValues& info) {
Expand Down Expand Up @@ -558,7 +561,7 @@ struct ConstantFieldPropagation : public Pass {
// a copy of A means it could be a copy of B or C).
StructUtils::TypeHierarchyPropagator<StructUtils::CombinableBool>
boolPropagator(subTypes);
boolPropagator.propagateToSubTypes(combinedCopyInfos);
boolPropagator.propagateToSubTypesWithExact(combinedCopyInfos);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above explains in detail why we propagate as we do, and should be updated for exactness.

Perhaps something like "when we read from an exact reference, the value at its field may be influenced by writes to non-exact super- and subtypes."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's address this in follow-on PRs that remove the false distinction between values written by news and values written by sets. The explanation of the analysis can be made much simpler and more rigorous once we simplify the analysis itself.

for (auto& [type, copied] : combinedCopyInfos) {
for (Index i = 0; i < copied.size(); i++) {
if (copied[i]) {
Expand All @@ -570,7 +573,7 @@ struct ConstantFieldPropagation : public Pass {
StructUtils::TypeHierarchyPropagator<PossibleConstantValues> propagator(
subTypes);
propagator.propagateToSuperTypes(combinedNewInfos);
propagator.propagateToSuperAndSubTypes(combinedSetInfos);
propagator.propagateToSuperAndSubTypesWithExact(combinedSetInfos);

// Combine both sources of information to the final information that gets
// care about.
Expand Down
10 changes: 5 additions & 5 deletions src/passes/GlobalTypeOptimization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
#include "ir/struct-utils.h"
#include "ir/subtypes.h"
#include "ir/type-updating.h"
#include "ir/utils.h"
#include "pass.h"
#include "support/permutations.h"
#include "wasm-builder.h"
Expand Down Expand Up @@ -205,9 +204,9 @@ struct GlobalTypeOptimization : public Pass {
SubTypes subTypes(*module);
StructUtils::TypeHierarchyPropagator<FieldInfo> propagator(subTypes);
auto dataFromSubsAndSupersMap = combinedSetGetInfos;
propagator.propagateToSuperAndSubTypes(dataFromSubsAndSupersMap);
propagator.propagateToSuperAndSubTypesWithExact(dataFromSubsAndSupersMap);
auto dataFromSupersMap = std::move(combinedSetGetInfos);
propagator.propagateToSubTypes(dataFromSupersMap);
propagator.propagateToSubTypesWithExact(dataFromSupersMap);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as before - why do we want exactness here? On the one hand it seems obvious we do, but it isn't the default - why not?

And, I see no test changes from this - if it doesn't help, why do it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the FieldInfoScanner now writes the info it discovers at a mix of exact and inexact keys, we now need to propagate information to the exact keys as well to ensure all the data is properly combined.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, but that sounds like it applies to all users of the utility? (why do it here and not in all others?)


// Find the public types, which we must not modify.
auto publicTypes = ModuleUtils::getPublicHeapTypes(*module);
Expand All @@ -224,8 +223,9 @@ struct GlobalTypeOptimization : public Pass {
continue;
}
auto& fields = type.getStruct().fields;
auto& dataFromSubsAndSupers = dataFromSubsAndSupersMap[type];
auto& dataFromSupers = dataFromSupersMap[type];
auto ht = std::make_pair(type, Exact);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment makes sense for dataFromSupersMap which was propagated using propagateToSubTypes - only down. But what about dataFromSubsAndSupersMap which was propagated both ways?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's propagated both ways, then the exact and inexact entries are the same. I'll add that to the comment.

auto& dataFromSubsAndSupers = dataFromSubsAndSupersMap[ht];
auto& dataFromSupers = dataFromSupersMap[ht];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must say, I find it confusing to see "use an Exact reference, but look in data that was propagated from subs and supers". Those feel in contradiction. What's the right way to look at this?

Concretely, I'm not sure why this Exact should not be InExact.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An (exact $Foo) is a subtype of $Foo and is propagated to and from just like any other subtype of $Foo. Exactness fits right into the existing super and subtype relationships, so there is no contradiction between talking about exactness and talking about super- and subtypes.

Concretely, this needs to be Exact because the inexact keys no longer have all the data in dataFromSupersMap. The field scanner now writes data to a mix of exact and inexact keys. This doesn't matter for dataFromSubsAndSupersMap because it propagates up and down simultaneously, ensuring the exact and inexact entries for a given heap type have the same data. But for dataFromSupersMap, the inexact entry has never been joined with the data in the exact entry since propagation has only gone down.

Copy link
Member

@kripken kripken Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks.

That makes sense, but this does seem more complicated than before. Continuing my comment from a moment ago, can we

  1. Make the propagation always go into exact types? As you say, they are just a normal part of subtyping. So there shouldn't be a need to oddly avoid propagating them.
  2. That the inexact keys don't have all the data, and that the solution is to query with an exact ref, seems surprising, in the sense that I would expect the point of "querying with an exact ref" to be "get exact results". Can we perhaps add an API to query a heap type without specifying exactness, and that will get all the data (internally, using an exact reference if that makes sense in the concrete data structure, but my point is that the query is just trying to get all the propagated info for the heap type).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another solution to limit the spread of this complexity would be to revert the shared changes and so something bespoke in CFP. I already have further changes to how copies are noted in the works, and that also only benefits CFP. I could also get some nice performance gains by doing a custom propagation algorithm for CFP, so maybe it makes sense not try to share any of that with other passes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, how would the custom propagation differ from the existing?

Though it seems CFP has fairly general needs, so I would hope we can keep sharing logic between it and other passes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed offline, I will remove the complexity of trying to sometimes include exactness and sometimes not. We will always make exactness explicit for all users. As we also discussed, the problem with suggestion (2) is that the fully collected information for a particular heap type might be in its exact or inexact entry (or both) depending on which direction the information was propagated in.


// Process immutability.
for (Index i = 0; i < fields.size(); i++) {
Expand Down
Loading
Loading