[maglev] Keep forward property facts in MergeForLoop without backedge witness [v8/v8 : main]
0 views
Skip to first unread message
Victor Gomes (Gerrit)
Jul 24, 2026, 9:04:23 AM (20 hours ago) Jul 24
to Olivier Flückiger, chrom...@appspot.gserviceaccount.com, dmercadi...@chromium.org, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Olivier Flückiger
Victor Gomes added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Olivier Flückiger
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Jul 24, 2026, 9:10:34 AM (20 hours ago) Jul 24
to Victor Gomes, Olivier Flückiger, chrom...@appspot.gserviceaccount.com, dmercadi...@chromium.org, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Olivier Flückiger and Victor Gomes
Darius Mercadier added 1 comment![]( "Open in Gerrit")
File src/maglev/maglev-known-node-aspects.cc
Line 378, Patchset 3 (Latest):
// The forward predecessor dominates the loop body and the loop effects
// summarize every write in the body, so a forward fact provably untouched
// by the loop is kept even without a matching backedge entry.
for (auto key_it = loaded_properties_.begin();
key_it != loaded_properties_.end();) {
PropertyKey key = key_it->first;
const ZoneMap<ValueNode*, ValueNode*>* backedge_props = nullptr;
auto backedge_key_it = backedge.loaded_properties_.find(key);
if (backedge_key_it != backedge.loaded_properties_.end()) {
backedge_props = &backedge_key_it->second;
}
ZoneMap<ValueNode*, ValueNode*>& props = key_it->second;
for (auto it = props.begin(); it != props.end();) {
ValueNode* obj = it->first;
if (it->second) it->second = it->second->UnwrapIdentities();
ValueNode* l = it->second;
ValueNode* r = nullptr;
if (backedge_props) {
auto r_it = backedge_props->find(obj);
if (r_it != backedge_props->end() && r_it->second) {
r = r_it->second->UnwrapIdentities();
}
}
bool keep;
if (r) {
keep = l == r || (entry_invariant(key, obj) && same_load(l, r));
} else {
keep = l != nullptr && entry_invariant(key, obj);
}
if (keep) {
++it;
} else {
it = props.erase(it);
}
}
if (props.empty()) {
key_it = loaded_properties_.erase(key_it);
} else {
++key_it;
}Darius Mercadier . unresolved
Is there really no way to make this look nicer???
Related details
Attention is currently required from:
- Olivier Flückiger
- Victor Gomes
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Jul 24, 2026, 9:49:33 AM (19 hours ago) Jul 24
to Victor Gomes, Darius Mercadier, chrom...@appspot.gserviceaccount.com, dmercadi...@chromium.org, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Victor Gomes
Olivier Flückiger added 3 comments![]( "Open in Gerrit")
Patchset-level comments
Olivier Flückiger . resolved
nice!
File src/maglev/maglev-known-node-aspects.cc
Line 283, Patchset 3 (Latest):
// Loop-header variant of Merge. The forward (LHS) predecessor dominates theOlivier Flückiger . unresolved
#followup
ugh, depending on visitation order is so brittle here. it would really help to normalize the graph such that we always have one forward entry for loops.
Olivier Flückiger
next to the niceness, another question is if we could somehow unify it with CloneForLoopHeader to make sure they always change in lockstep?
Related details
Attention is currently required from:
- Victor Gomes
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Gomes (Gerrit)
Jul 24, 2026, 9:51:12 AM (19 hours ago) Jul 24
to Darius Mercadier, Olivier Flückiger, chrom...@appspot.gserviceaccount.com, dmercadi...@chromium.org, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Darius Mercadier and Olivier Flückiger
Victor Gomes added 2 comments![]( "Open in Gerrit")
File src/maglev/maglev-known-node-aspects.cc
Line 283, Patchset 3:
// Loop-header variant of Merge. The forward (LHS) predecessor dominates theOlivier Flückiger . resolved
#followup
ugh, depending on visitation order is so brittle here. it would really help to normalize the graph such that we always have one forward entry for loops.
Victor Gomes
Let's do that in a follow up. It was kind of in my plans already, since loop peeler only supports a single forward predecessor.
Line 378, Patchset 3:
// The forward predecessor dominates the loop body and the loop effectsVictor Gomes
It is a lock-step walk similar to https://source.chromium.org/chromium/chromium/src/+/main:v8/src/maglev/maglev-known-node-aspects.cc;drc=2c2e022b251a82064c60e67b17f9103162a501aa;l=79
I guess we can massage DestructivelyIntersect... but I guess that is also ugly in other aspects. But I can try. Wdyt?
Related details
Attention is currently required from:
- Darius Mercadier
- Olivier Flückiger
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages