Skip to content

Commit dbeaa2e

Browse files
marjakhV8 LUCI CQ
marjakh
authored and
V8 LUCI CQ
committed
[maglev] Introduce AddNewNodeNoInputConversion
This will make it possible to make some code immune to the upcoming "AddNewNode returns a ReduceResult" refactoring. If the node doesn't need input conversions, AddNewNode cannot abort. Bug: 428667907 Change-Id: I21a90cb58443c65dbde1d4c0a4de9a0a952092dd Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6701574 Reviewed-by: Victor Gomes <[email protected]> Commit-Queue: Marja Hölttä <[email protected]> Cr-Commit-Position: refs/heads/main@{#101249}
1 parent 9c3a6b3 commit dbeaa2e

File tree

6 files changed

+117
-37
lines changed

6 files changed

+117
-37
lines changed

‎src/maglev/maglev-graph-builder.cc‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1592,24 +1592,31 @@ ReduceResult MaglevGraphBuilder::GetSmiValue(
15921592
switch (representation) {
15931593
case ValueRepresentation::kInt32: {
15941594
if (NodeTypeIsSmi(node_info->type())) {
1595-
return alternative.set_tagged(AddNewNode<UnsafeSmiTagInt32>({value}));
1595+
return alternative.set_tagged(
1596+
AddNewNodeNoInputConversion<UnsafeSmiTagInt32>({value}));
15961597
}
1597-
return alternative.set_tagged(AddNewNode<CheckedSmiTagInt32>({value}));
1598+
return alternative.set_tagged(
1599+
AddNewNodeNoInputConversion<CheckedSmiTagInt32>({value}));
15981600
}
15991601
case ValueRepresentation::kUint32: {
16001602
if (NodeTypeIsSmi(node_info->type())) {
1601-
return alternative.set_tagged(AddNewNode<UnsafeSmiTagUint32>({value}));
1603+
return alternative.set_tagged(
1604+
AddNewNodeNoInputConversion<UnsafeSmiTagUint32>({value}));
16021605
}
1603-
return alternative.set_tagged(AddNewNode<CheckedSmiTagUint32>({value}));
1606+
return alternative.set_tagged(
1607+
AddNewNodeNoInputConversion<CheckedSmiTagUint32>({value}));
16041608
}
16051609
case ValueRepresentation::kFloat64: {
1606-
return alternative.set_tagged(AddNewNode<CheckedSmiTagFloat64>({value}));
1610+
return alternative.set_tagged(
1611+
AddNewNodeNoInputConversion<CheckedSmiTagFloat64>({value}));
16071612
}
16081613
case ValueRepresentation::kHoleyFloat64: {
1609-
return alternative.set_tagged(AddNewNode<CheckedSmiTagFloat64>({value}));
1614+
return alternative.set_tagged(
1615+
AddNewNodeNoInputConversion<CheckedSmiTagFloat64>({value}));
16101616
}
16111617
case ValueRepresentation::kIntPtr:
1612-
return alternative.set_tagged(AddNewNode<CheckedSmiTagIntPtr>({value}));
1618+
return alternative.set_tagged(
1619+
AddNewNodeNoInputConversion<CheckedSmiTagIntPtr>({value}));
16131620
case ValueRepresentation::kTagged:
16141621
UNREACHABLE();
16151622
}
@@ -15668,6 +15675,13 @@ NodeT* MaglevGraphBuilder::AddNewNode(std::initializer_list<ValueNode*> inputs,
1566815675
return reducer_.AddNewNode<NodeT>(inputs, std::forward<Args>(args)...);
1566915676
}
1567015677

15678+
template <typename NodeT, typename... Args>
15679+
NodeT* MaglevGraphBuilder::AddNewNodeNoInputConversion(
15680+
std::initializer_list<ValueNode*> inputs, Args&&... args) {
15681+
return reducer_.AddNewNodeNoInputConversion<NodeT>(
15682+
inputs, std::forward<Args>(args)...);
15683+
}
15684+
1567115685
template <typename NodeT>
1567215686
void MaglevGraphBuilder::AttachDeoptCheckpoint(NodeT* node) {
1567315687
if constexpr (NodeT::kProperties.is_deopt_checkpoint()) {

‎src/maglev/maglev-graph-builder.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -442,6 +442,9 @@ class MaglevGraphBuilder {
442442
// Add a new node with a static set of inputs.
443443
template <typename NodeT, typename... Args>
444444
NodeT* AddNewNode(std::initializer_list<ValueNode*> inputs, Args&&... args);
445+
template <typename NodeT, typename... Args>
446+
NodeT* AddNewNodeNoInputConversion(std::initializer_list<ValueNode*> inputs,
447+
Args&&... args);
445448
template <typename NodeT>
446449
void AttachDeoptCheckpoint(NodeT* node);
447450
template <typename NodeT>

‎src/maglev/maglev-ir.cc‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -867,10 +867,7 @@ void CheckValueInputIs(const NodeBase* node, int i,
867867
ValueNode* input = node->input(i).node();
868868
DCHECK(!input->Is<Identity>());
869869
ValueRepresentation got = input->properties().value_representation();
870-
// Allow Float64 values to be inputs when HoleyFloat64 is expected.
871-
bool valid =
872-
(got == expected) || (got == ValueRepresentation::kFloat64 &&
873-
expected == ValueRepresentation::kHoleyFloat64);
870+
bool valid = ValueRepresentationIs(got, expected);
874871
if (!valid) {
875872
std::ostringstream str;
876873
str << "Type representation error: node ";

‎src/maglev/maglev-ir.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -690,6 +690,13 @@ inline constexpr bool IsZeroExtendedRepresentation(ValueRepresentation repr) {
690690
#endif
691691
}
692692

693+
inline bool ValueRepresentationIs(ValueRepresentation got,
694+
ValueRepresentation expected) {
695+
// Allow Float64 values to be inputs when HoleyFloat64 is expected.
696+
return (got == expected) || (got == ValueRepresentation::kFloat64 &&
697+
expected == ValueRepresentation::kHoleyFloat64);
698+
}
699+
693700
// ValueRepresentation doesn't distinguish between Int32 and TruncatedInt32:
694701
// both are Int32. For Phi untagging however, it's interesting to have a
695702
// difference between the 2, as a TruncatedInt32 would allow untagging to

‎src/maglev/maglev-reducer-inl.h‎

Lines changed: 69 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ NodeT* MaglevReducer<BaseT>::AddNewNode(
8989
if constexpr (Node::participate_in_cse(Node::opcode_of<NodeT>) &&
9090
ReducerBaseWithKNA<BaseT>) {
9191
if (v8_flags.maglev_cse) {
92-
return AddNewNodeOrGetEquivalent<NodeT>(inputs,
92+
return AddNewNodeOrGetEquivalent<NodeT>(true, inputs,
9393
std::forward<Args>(args)...);
9494
}
9595
}
@@ -99,6 +99,24 @@ NodeT* MaglevReducer<BaseT>::AddNewNode(
9999
return AttachExtraInfoAndAddToGraph(node);
100100
}
101101

102+
template <typename BaseT>
103+
template <typename NodeT, typename... Args>
104+
NodeT* MaglevReducer<BaseT>::AddNewNodeNoInputConversion(
105+
std::initializer_list<ValueNode*> inputs, Args&&... args) {
106+
static_assert(IsFixedInputNode<NodeT>());
107+
if constexpr (Node::participate_in_cse(Node::opcode_of<NodeT>) &&
108+
ReducerBaseWithKNA<BaseT>) {
109+
if (v8_flags.maglev_cse) {
110+
return AddNewNodeOrGetEquivalent<NodeT>(false, inputs,
111+
std::forward<Args>(args)...);
112+
}
113+
}
114+
NodeT* node =
115+
NodeBase::New<NodeT>(zone(), inputs.size(), std::forward<Args>(args)...);
116+
SetNodeInputsNoConversion(node, inputs);
117+
return AttachExtraInfoAndAddToGraph(node);
118+
}
119+
102120
template <typename BaseT>
103121
template <typename NodeT, typename... Args>
104122
NodeT* MaglevReducer<BaseT>::AddUnbufferedNewNode(
@@ -115,7 +133,8 @@ NodeT* MaglevReducer<BaseT>::AddUnbufferedNewNode(
115133
template <typename BaseT>
116134
template <typename NodeT, typename... Args>
117135
NodeT* MaglevReducer<BaseT>::AddNewNodeOrGetEquivalent(
118-
std::initializer_list<ValueNode*> raw_inputs, Args&&... args) {
136+
bool convert_inputs, std::initializer_list<ValueNode*> raw_inputs,
137+
Args&&... args) {
119138
DCHECK(v8_flags.maglev_cse);
120139
static constexpr Opcode op = Node::opcode_of<NodeT>;
121140
static_assert(Node::participate_in_cse(op));
@@ -133,9 +152,16 @@ NodeT* MaglevReducer<BaseT>::AddNewNodeOrGetEquivalent(
133152
int i = 0;
134153
constexpr UseReprHintRecording hint = ShouldRecordUseReprHint<NodeT>();
135154
for (ValueNode* raw_input : raw_inputs) {
136-
// TODO(marja): Here we might already have the empty type for the
137-
// node. Generate a deopt and make callers handle it.
138-
inputs[i] = ConvertInputTo<hint>(raw_input, NodeT::kInputTypes[i]);
155+
if (convert_inputs) {
156+
// TODO(marja): Here we might already have the empty type for the
157+
// node. Generate a deopt and make callers handle it.
158+
inputs[i] = ConvertInputTo<hint>(raw_input, NodeT::kInputTypes[i]);
159+
} else {
160+
CHECK(ValueRepresentationIs(
161+
raw_input->properties().value_representation(),
162+
NodeT::kInputTypes[i]));
163+
inputs[i] = raw_input;
164+
}
139165
i++;
140166
}
141167
if constexpr (IsCommutativeNode(Node::opcode_of<NodeT>)) {
@@ -196,11 +222,7 @@ NodeT* MaglevReducer<BaseT>::AddNewNodeOrGetEquivalent(
196222
}
197223
NodeT* node =
198224
NodeBase::New<NodeT>(zone(), inputs.size(), std::forward<Args>(args)...);
199-
int i = 0;
200-
for (ValueNode* input : inputs) {
201-
DCHECK_NOT_NULL(input);
202-
node->set_input(i++, input);
203-
}
225+
SetNodeInputsNoConversion(node, inputs);
204226
DCHECK_EQ(node->options(), std::tuple{std::forward<Args>(args)...});
205227
uint32_t epoch = Node::needs_epoch_check(op)
206228
? known_node_aspects().effect_epoch()
@@ -268,9 +290,8 @@ ValueNode* MaglevReducer<BaseT>::ConvertInputTo(ValueNode* input,
268290
}
269291

270292
template <typename BaseT>
271-
template <typename NodeT>
272-
void MaglevReducer<BaseT>::SetNodeInputs(
273-
NodeT* node, std::initializer_list<ValueNode*> inputs) {
293+
template <typename NodeT, typename InputsT>
294+
void MaglevReducer<BaseT>::SetNodeInputs(NodeT* node, InputsT inputs) {
274295
// Nodes with zero input count don't have kInputTypes defined.
275296
if constexpr (NodeT::kInputCount > 0) {
276297
constexpr UseReprHintRecording hint = ShouldRecordUseReprHint<NodeT>();
@@ -283,6 +304,23 @@ void MaglevReducer<BaseT>::SetNodeInputs(
283304
}
284305
}
285306

307+
template <typename BaseT>
308+
template <typename NodeT, typename InputsT>
309+
void MaglevReducer<BaseT>::SetNodeInputsNoConversion(NodeT* node,
310+
InputsT inputs) {
311+
// Nodes with zero input count don't have kInputTypes defined.
312+
if constexpr (NodeT::kInputCount > 0) {
313+
int i = 0;
314+
for (ValueNode* input : inputs) {
315+
DCHECK_NOT_NULL(input);
316+
CHECK(ValueRepresentationIs(input->properties().value_representation(),
317+
NodeT::kInputTypes[i]));
318+
node->set_input(i, input);
319+
i++;
320+
}
321+
}
322+
}
323+
286324
template <typename BaseT>
287325
template <typename NodeT>
288326
NodeT* MaglevReducer<BaseT>::AttachExtraInfoAndAddToGraph(NodeT* node) {
@@ -402,32 +440,40 @@ ValueNode* MaglevReducer<BaseT>::GetTaggedValue(
402440
case ValueRepresentation::kInt32: {
403441
if (!IsEmptyNodeType(node_info->type()) &&
404442
NodeTypeIsSmi(node_info->type())) {
405-
return alternative.set_tagged(AddNewNode<UnsafeSmiTagInt32>({value}));
443+
return alternative.set_tagged(
444+
AddNewNodeNoInputConversion<UnsafeSmiTagInt32>({value}));
406445
}
407-
return alternative.set_tagged(AddNewNode<Int32ToNumber>({value}));
446+
return alternative.set_tagged(
447+
AddNewNodeNoInputConversion<Int32ToNumber>({value}));
408448
}
409449
case ValueRepresentation::kUint32: {
410450
if (!IsEmptyNodeType(node_info->type()) &&
411451
NodeTypeIsSmi(node_info->type())) {
412-
return alternative.set_tagged(AddNewNode<UnsafeSmiTagUint32>({value}));
452+
return alternative.set_tagged(
453+
AddNewNodeNoInputConversion<UnsafeSmiTagUint32>({value}));
413454
}
414-
return alternative.set_tagged(AddNewNode<Uint32ToNumber>({value}));
455+
return alternative.set_tagged(
456+
AddNewNodeNoInputConversion<Uint32ToNumber>({value}));
415457
}
416458
case ValueRepresentation::kFloat64: {
417-
return alternative.set_tagged(AddNewNode<Float64ToTagged>(
418-
{value}, Float64ToTagged::ConversionMode::kCanonicalizeSmi));
459+
return alternative.set_tagged(
460+
AddNewNodeNoInputConversion<Float64ToTagged>(
461+
{value}, Float64ToTagged::ConversionMode::kCanonicalizeSmi));
419462
}
420463
case ValueRepresentation::kHoleyFloat64: {
421-
return alternative.set_tagged(AddNewNode<HoleyFloat64ToTagged>(
422-
{value}, HoleyFloat64ToTagged::ConversionMode::kForceHeapNumber));
464+
return alternative.set_tagged(
465+
AddNewNodeNoInputConversion<HoleyFloat64ToTagged>(
466+
{value}, HoleyFloat64ToTagged::ConversionMode::kForceHeapNumber));
423467
}
424468

425469
case ValueRepresentation::kIntPtr:
426470
if (!IsEmptyNodeType(node_info->type()) &&
427471
NodeTypeIsSmi(node_info->type())) {
428-
return alternative.set_tagged(AddNewNode<UnsafeSmiTagIntPtr>({value}));
472+
return alternative.set_tagged(
473+
AddNewNodeNoInputConversion<UnsafeSmiTagIntPtr>({value}));
429474
}
430-
return alternative.set_tagged(AddNewNode<IntPtrToNumber>({value}));
475+
return alternative.set_tagged(
476+
AddNewNodeNoInputConversion<IntPtrToNumber>({value}));
431477

432478
case ValueRepresentation::kTagged:
433479
UNREACHABLE();

‎src/maglev/maglev-reducer.h‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,11 @@ class MaglevReducer {
245245
std::initializer_list<ValueNode*> inputs,
246246
Args&&... args);
247247

248+
// Add a new node with a static set of inputs.
249+
template <typename NodeT, typename... Args>
250+
NodeT* AddNewNodeNoInputConversion(std::initializer_list<ValueNode*> inputs,
251+
Args&&... args);
252+
248253
void AddInitializedNodeToGraph(Node* node);
249254

250255
std::optional<int32_t> TryGetInt32Constant(ValueNode* value);
@@ -419,7 +424,8 @@ class MaglevReducer {
419424
};
420425

421426
template <typename NodeT, typename... Args>
422-
NodeT* AddNewNodeOrGetEquivalent(std::initializer_list<ValueNode*> raw_inputs,
427+
NodeT* AddNewNodeOrGetEquivalent(bool convert_inputs,
428+
std::initializer_list<ValueNode*> raw_inputs,
423429
Args&&... args);
424430

425431
template <UseReprHintRecording hint = UseReprHintRecording::kRecord>
@@ -438,8 +444,15 @@ class MaglevReducer {
438444
}
439445
}
440446

441-
template <typename NodeT>
442-
void SetNodeInputs(NodeT* node, std::initializer_list<ValueNode*> inputs);
447+
// TODO(marja): When we have C++26, `inputs` can be std::span<ValueNode*>,
448+
// since std::intializer_list can be converted to std::span.
449+
template <typename NodeT, typename InputsT>
450+
void SetNodeInputs(NodeT* node, InputsT inputs);
451+
452+
// TODO(marja): When we have C++26, `inputs` can be std::span<ValueNode*>,
453+
// since std::intializer_list can be converted to std::span.
454+
template <typename NodeT, typename InputsT>
455+
void SetNodeInputsNoConversion(NodeT* node, InputsT inputs);
443456
template <typename NodeT>
444457
NodeT* AttachExtraInfoAndAddToGraph(NodeT* node);
445458
template <typename NodeT>

0 commit comments

Comments
 (0)