[turbolev] Range value analysis [v8/v8 : main]
Victor Gomes (Gerrit)
Victor Gomes added 1 comment![]( "Open in Gerrit")
Related details
- Darius Mercadier
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 23 comments![]( "Open in Gerrit")
Overall looks good; cool stuff! :)
DEFINE_BOOL(maglev_range_analysis, false,Make experimental before landing.
auto r1 = GetRange(GetInputAt(0));Now that the Visit methods take the `node` as input, I think that you should remove those `GetInputAt` helpers and just use `node->input_node()` instead.
(not in this CL though)
if (r1 && r2 && Range::Uint32().contains(*r1) && *r1 < *r2) {`r1->IsUint32()` would feel less awkward imo
return ReplaceWith(
reducer_.AddNewNodeNoInputConversion<UnsafeSmiTagInt32>(
{GetInputAt(0)}));What about overwriting the node in place instead of creating a new node?
} else if (block->is_edge_split_block()) {
BasicBlock* next_block = block;
while (next_block->is_edge_split_block()) {
next_block = next_block->control_node()->Cast<Jump>()->target();
}
known_node_aspects_ = next_block->state()->CloneKnownNodeAspects(zone());
}
DCHECK_NOT_NULL(known_node_aspects_);Consider doing this in a separate CL.
BasicBlock* next_block = block;
while (next_block->is_edge_split_block()) {
next_block = next_block->control_node()->Cast<Jump>()->target();
}
known_node_aspects_ = next_block->state()->CloneKnownNodeAspects(zone());```suggestion
while (block->is_edge_split_block()) {
block = block->control_node()->Cast<Jump>()->target();
}
known_node_aspects_ = block->state()->CloneKnownNodeAspects(zone());
```
if (range != widened && !(range.is_empty() && widened.is_empty())) {What's the idea of this check exactly? Can't this be true in some cases where the loop is unreachable?
auto range = ranges_.Get(block, phi);
auto backedge = ranges_.Get(backedge_pred, phi->input_node(backedge_id));
auto widened = Range::Widen(range, backedge);```suggestion
Range range = ranges_.Get(block, phi);
Range backedge = ranges_.Get(backedge_pred, phi->input_node(backedge_id));
Range widened = Range::Widen(range, backedge);
```
DCHECK_EQ(backedge_pred, block->backedge_predecessor());move up
for (int i = block->predecessor_count() - 1; i >= 0; i--) {Why is this iterating backwards?
if (node->if_true() == succ) {
ranges_.NarrowUpdate(succ, lhs, lhs_range.ConstrainLessEqual(rhs_range));
ranges_.NarrowUpdate(succ, rhs, rhs_range.ConstraintGreater(lhs_range));
} else {
DCHECK_EQ(node->if_false(), succ);
ranges_.NarrowUpdate(succ, lhs, lhs_range.ConstraintGreater(rhs_range));
ranges_.NarrowUpdate(succ, rhs, rhs_range.ConstrainLessEqual(lhs_range));
}This should depend on `node->operation()`
} else {I would distinguish between Conditional and Unconditional control nodes to make things clearer. Thanks to edge splitting, you are guaranteed that Conditional control node go to blocks with a single predecessor. And only unconditional control nodes are allowed to go to Merges.
TRACE_RANGE("[range]: Narrow update: "```suggestion
TRACE_RANGE("[range]: update: "
```
void Update(BasicBlock* block, ValueNode* node, Range range) {`Union` would be a more descriptive name.
if (!IsSafeInteger(new_min)) new_min = kInfMin;Is that possible?
if (!IsSafeInteger(new_max)) new_max = kInfMax;Is that possible?
int64_t min = kInfMin;
if (r1.min_ != kInfMin) {
min = r1.min_ & r2.max_;
}
int64_t max = kInfMax;
if (r1.max_ != kInfMax) {
max = r1.max_ & r2.max_;
}
if (max < min) return Range::Empty();
return Range(min, max);This is wrong. A simple example for the maxes for instance: if r1.max_ is `2**31` and r2.max_ is `2**31-1`, then the bitwise and of the 2 will be `0` instead of `2**31-1`.
Cf OperationTyper::NumberBitwiseAnd in Turbofan for an implementation that should be more correct.
if (r2.is_constant()) {`if (r1.is_constant() && !r2.is_constant()) { std::swap(r1, r2); }`
if (!IsSafeInteger(min)) min = kInfMin;
if (!IsSafeInteger(max)) max = kInfMax;Is that possible?
if (!IsSafeInteger(min)) min = kInfMin;
if (!IsSafeInteger(max)) max = kInfMax;Is that possible?
void DestructivelyIntersect(ZoneMap<Key, Value>& lhs_map,Add a comment stating which input will be updated by the intersection (lhs_map here).
Related details
- Victor Gomes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |