Skip to content

Commit 71d9601

Browse files
aartbikcommit-bot@chromium.org
aartbik
authored and
[email protected]
committed
[vm/compiler] Introduce 64-bit NEGATE - all archs.
Rationale: This improves JIT and AOT performance of unary minus and also improves constant folding and range analysis on negative constant (viz. x / -3 is often x / - (3)). The SHIFT operator needed some special treatment, since we have to avoid converting a NON-speculative shifts back into a deopt. #34072 Change-Id: I230c9cfda98297f683bbba53688e57c2cc659360 Reviewed-on: https://dart-review.googlesource.com/68434 Reviewed-by: Alexander Markov <[email protected]> Reviewed-by: Vyacheslav Egorov <[email protected]> Commit-Queue: Aart Bik <[email protected]>
1 parent 5013a2c commit 71d9601

File tree

7 files changed

+130
-19
lines changed

7 files changed

+130
-19
lines changed

‎runtime/vm/compiler/aot/aot_call_specializer.cc

+10
Original file line numberDiff line numberDiff line change
@@ -607,6 +607,16 @@ bool AotCallSpecializer::TryOptimizeStaticCallUsingStaticTypes(
607607
}
608608
break;
609609
}
610+
#ifndef TARGET_ARCH_DBC
611+
case Token::kNEGATE: {
612+
Value* left_value = instr->PushArgumentAt(receiver_index)->value();
613+
left_value = PrepareReceiverOfDevirtualizedCall(left_value, kMintCid);
614+
replacement = new (Z)
615+
UnaryInt64OpInstr(Token::kNEGATE, left_value, Thread::kNoDeoptId,
616+
Instruction::kNotSpeculative);
617+
break;
618+
}
619+
#endif
610620
case Token::kSHL:
611621
case Token::kSHR: {
612622
Value* left_value = instr->PushArgumentAt(receiver_index)->value();

‎runtime/vm/compiler/backend/il.cc

+16
Original file line numberDiff line numberDiff line change
@@ -2437,9 +2437,17 @@ Definition* BinaryIntegerOpInstr::Canonicalize(FlowGraph* flow_graph) {
24372437
if (rhs == 0) {
24382438
return left()->definition();
24392439
} else if (rhs < 0) {
2440+
// Instruction will always throw on negative rhs operand.
2441+
if (!CanDeoptimize()) {
2442+
// For non-speculative operations (no deopt), let
2443+
// the code generator deal with throw on slowpath.
2444+
break;
2445+
}
2446+
ASSERT(GetDeoptId() != Thread::kNoDeoptId);
24402447
DeoptimizeInstr* deopt =
24412448
new DeoptimizeInstr(ICData::kDeoptBinarySmiOp, GetDeoptId());
24422449
flow_graph->InsertBefore(this, deopt, env(), FlowGraph::kEffect);
2450+
// Replace with zero since it always throws.
24432451
return CreateConstantResult(flow_graph, Integer::Handle(Smi::New(0)));
24442452
}
24452453
break;
@@ -2450,10 +2458,18 @@ Definition* BinaryIntegerOpInstr::Canonicalize(FlowGraph* flow_graph) {
24502458
return left()->definition();
24512459
} else if ((rhs < 0) || (rhs >= kMaxShift)) {
24522460
if ((rhs < 0) || !is_truncating()) {
2461+
// Instruction will always throw on negative rhs operand.
2462+
if (!CanDeoptimize()) {
2463+
// For non-speculative operations (no deopt), let
2464+
// the code generator deal with throw on slowpath.
2465+
break;
2466+
}
2467+
ASSERT(GetDeoptId() != Thread::kNoDeoptId);
24532468
DeoptimizeInstr* deopt =
24542469
new DeoptimizeInstr(ICData::kDeoptBinarySmiOp, GetDeoptId());
24552470
flow_graph->InsertBefore(this, deopt, env(), FlowGraph::kEffect);
24562471
}
2472+
// Replace with zero since it overshifted or always throws.
24572473
return CreateConstantResult(flow_graph, Integer::Handle(Smi::New(0)));
24582474
}
24592475
break;

‎runtime/vm/compiler/backend/il_arm.cc

+21-6
Original file line numberDiff line numberDiff line change
@@ -6090,7 +6090,9 @@ LocationSummary* ShiftInt64OpInstr::MakeLocationSummary(Zone* zone,
60906090
zone, kNumInputs, kNumTemps, LocationSummary::kCallOnSlowPath);
60916091
summary->set_in(0, Location::Pair(Location::RequiresRegister(),
60926092
Location::RequiresRegister()));
6093-
if (ConstantInstr* constant = right()->definition()->AsConstant()) {
6093+
if (RangeUtils::IsPositive(shift_range()) &&
6094+
right()->definition()->IsConstant()) {
6095+
ConstantInstr* constant = right()->definition()->AsConstant();
60946096
summary->set_in(1, Location::Constant(constant));
60956097
} else {
60966098
summary->set_in(1, Location::Pair(Location::RequiresRegister(),
@@ -6114,7 +6116,7 @@ void ShiftInt64OpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
61146116
EmitShiftInt64ByConstant(compiler, op_kind(), out_lo, out_hi, left_lo,
61156117
left_hi, locs()->in(1).constant());
61166118
} else {
6117-
// Code for a variable shift amount.
6119+
// Code for a variable shift amount (or constant that throws).
61186120
PairLocation* right_pair = locs()->in(1).AsPairLocation();
61196121
Register right_lo = right_pair->At(0).reg();
61206122
Register right_hi = right_pair->At(1).reg();
@@ -6228,7 +6230,9 @@ LocationSummary* ShiftUint32OpInstr::MakeLocationSummary(Zone* zone,
62286230
LocationSummary* summary = new (zone) LocationSummary(
62296231
zone, kNumInputs, kNumTemps, LocationSummary::kCallOnSlowPath);
62306232
summary->set_in(0, Location::RequiresRegister());
6231-
if (ConstantInstr* constant = right()->definition()->AsConstant()) {
6233+
if (RangeUtils::IsPositive(shift_range()) &&
6234+
right()->definition()->IsConstant()) {
6235+
ConstantInstr* constant = right()->definition()->AsConstant();
62326236
summary->set_in(1, Location::Constant(constant));
62336237
} else {
62346238
summary->set_in(1, Location::Pair(Location::RequiresRegister(),
@@ -6248,6 +6252,7 @@ void ShiftUint32OpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
62486252
EmitShiftUint32ByConstant(compiler, op_kind(), out, left,
62496253
locs()->in(1).constant());
62506254
} else {
6255+
// Code for a variable shift amount (or constant that throws).
62516256
PairLocation* right_pair = locs()->in(1).AsPairLocation();
62526257
Register right_lo = right_pair->At(0).reg();
62536258
Register right_hi = right_pair->At(1).reg();
@@ -6338,16 +6343,26 @@ LocationSummary* UnaryInt64OpInstr::MakeLocationSummary(Zone* zone,
63386343
}
63396344

63406345
void UnaryInt64OpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
6341-
ASSERT(op_kind() == Token::kBIT_NOT);
63426346
PairLocation* left_pair = locs()->in(0).AsPairLocation();
63436347
Register left_lo = left_pair->At(0).reg();
63446348
Register left_hi = left_pair->At(1).reg();
63456349

63466350
PairLocation* out_pair = locs()->out(0).AsPairLocation();
63476351
Register out_lo = out_pair->At(0).reg();
63486352
Register out_hi = out_pair->At(1).reg();
6349-
__ mvn(out_lo, Operand(left_lo));
6350-
__ mvn(out_hi, Operand(left_hi));
6353+
6354+
switch (op_kind()) {
6355+
case Token::kBIT_NOT:
6356+
__ mvn(out_lo, Operand(left_lo));
6357+
__ mvn(out_hi, Operand(left_hi));
6358+
break;
6359+
case Token::kNEGATE:
6360+
__ rsbs(out_lo, left_lo, Operand(0));
6361+
__ sbc(out_hi, out_hi, Operand(out_hi));
6362+
__ sub(out_hi, out_hi, Operand(left_hi));
6363+
default:
6364+
UNREACHABLE();
6365+
}
63516366
}
63526367

63536368
CompileType BinaryUint32OpInstr::ComputeType() const {

‎runtime/vm/compiler/backend/il_arm64.cc

+8-3
Original file line numberDiff line numberDiff line change
@@ -5398,7 +5398,9 @@ LocationSummary* ShiftInt64OpInstr::MakeLocationSummary(Zone* zone,
53985398
LocationSummary* summary = new (zone) LocationSummary(
53995399
zone, kNumInputs, kNumTemps, LocationSummary::kCallOnSlowPath);
54005400
summary->set_in(0, Location::RequiresRegister());
5401-
summary->set_in(1, Location::RegisterOrConstant(right()));
5401+
summary->set_in(1, RangeUtils::IsPositive(shift_range())
5402+
? Location::RegisterOrConstant(right())
5403+
: Location::RequiresRegister());
54025404
summary->set_out(0, Location::RequiresRegister());
54035405
return summary;
54045406
}
@@ -5412,7 +5414,7 @@ void ShiftInt64OpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
54125414
EmitShiftInt64ByConstant(compiler, op_kind(), out, left,
54135415
locs()->in(1).constant());
54145416
} else {
5415-
// Code for a variable shift amount.
5417+
// Code for a variable shift amount (or constant that throws).
54165418
Register shift = locs()->in(1).reg();
54175419

54185420
// Jump to a slow path if shift is larger than 63 or less than 0.
@@ -5507,7 +5509,9 @@ LocationSummary* ShiftUint32OpInstr::MakeLocationSummary(Zone* zone,
55075509
LocationSummary* summary = new (zone) LocationSummary(
55085510
zone, kNumInputs, kNumTemps, LocationSummary::kCallOnSlowPath);
55095511
summary->set_in(0, Location::RequiresRegister());
5510-
summary->set_in(1, Location::RegisterOrConstant(right()));
5512+
summary->set_in(1, RangeUtils::IsPositive(shift_range())
5513+
? Location::RegisterOrConstant(right())
5514+
: Location::RequiresRegister());
55115515
summary->set_out(0, Location::RequiresRegister());
55125516
return summary;
55135517
}
@@ -5520,6 +5524,7 @@ void ShiftUint32OpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
55205524
EmitShiftUint32ByConstant(compiler, op_kind(), out, left,
55215525
locs()->in(1).constant());
55225526
} else {
5527+
// Code for a variable shift amount (or constant that throws).
55235528
const Register right = locs()->in(1).reg();
55245529
const bool shift_count_in_range =
55255530
IsShiftCountInRange(kUint32ShiftCountLimit);

‎runtime/vm/compiler/backend/il_ia32.cc

+21-7
Original file line numberDiff line numberDiff line change
@@ -5497,7 +5497,9 @@ LocationSummary* ShiftInt64OpInstr::MakeLocationSummary(Zone* zone,
54975497
zone, kNumInputs, kNumTemps, LocationSummary::kCallOnSlowPath);
54985498
summary->set_in(0, Location::Pair(Location::RequiresRegister(),
54995499
Location::RequiresRegister()));
5500-
if (ConstantInstr* constant = right()->definition()->AsConstant()) {
5500+
if (RangeUtils::IsPositive(shift_range()) &&
5501+
right()->definition()->IsConstant()) {
5502+
ConstantInstr* constant = right()->definition()->AsConstant();
55015503
summary->set_in(1, Location::Constant(constant));
55025504
} else {
55035505
summary->set_in(1, Location::Pair(Location::RegisterLocation(ECX),
@@ -5522,7 +5524,7 @@ void ShiftInt64OpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
55225524
EmitShiftInt64ByConstant(compiler, op_kind(), left_lo, left_hi,
55235525
locs()->in(1).constant());
55245526
} else {
5525-
// Code for a variable shift amount.
5527+
// Code for a variable shift amount (or constant that throws).
55265528
ASSERT(locs()->in(1).AsPairLocation()->At(0).reg() == ECX);
55275529
Register right_hi = locs()->in(1).AsPairLocation()->At(1).reg();
55285530

@@ -5638,7 +5640,9 @@ LocationSummary* ShiftUint32OpInstr::MakeLocationSummary(Zone* zone,
56385640
LocationSummary* summary = new (zone) LocationSummary(
56395641
zone, kNumInputs, kNumTemps, LocationSummary::kCallOnSlowPath);
56405642
summary->set_in(0, Location::RequiresRegister());
5641-
if (ConstantInstr* constant = right()->definition()->AsConstant()) {
5643+
if (RangeUtils::IsPositive(shift_range()) &&
5644+
right()->definition()->IsConstant()) {
5645+
ConstantInstr* constant = right()->definition()->AsConstant();
56425646
summary->set_in(1, Location::Constant(constant));
56435647
} else {
56445648
summary->set_in(1, Location::Pair(Location::RegisterLocation(ECX),
@@ -5657,7 +5661,7 @@ void ShiftUint32OpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
56575661
EmitShiftUint32ByConstant(compiler, op_kind(), left,
56585662
locs()->in(1).constant());
56595663
} else {
5660-
// Code for a variable shift amount.
5664+
// Code for a variable shift amount (or constant that throws).
56615665
ASSERT(locs()->in(1).AsPairLocation()->At(0).reg() == ECX);
56625666
Register right_hi = locs()->in(1).AsPairLocation()->At(1).reg();
56635667

@@ -5745,7 +5749,6 @@ LocationSummary* UnaryInt64OpInstr::MakeLocationSummary(Zone* zone,
57455749
}
57465750

57475751
void UnaryInt64OpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
5748-
ASSERT(op_kind() == Token::kBIT_NOT);
57495752
PairLocation* left_pair = locs()->in(0).AsPairLocation();
57505753
Register left_lo = left_pair->At(0).reg();
57515754
Register left_hi = left_pair->At(1).reg();
@@ -5754,8 +5757,19 @@ void UnaryInt64OpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
57545757
Register out_hi = out_pair->At(1).reg();
57555758
ASSERT(out_lo == left_lo);
57565759
ASSERT(out_hi == left_hi);
5757-
__ notl(left_lo);
5758-
__ notl(left_hi);
5760+
switch (op_kind()) {
5761+
case Token::kBIT_NOT:
5762+
__ notl(left_lo);
5763+
__ notl(left_hi);
5764+
break;
5765+
case Token::kNEGATE:
5766+
__ negl(left_lo);
5767+
__ adcl(left_hi, Immediate(0));
5768+
__ negl(left_hi);
5769+
break;
5770+
default:
5771+
UNREACHABLE();
5772+
}
57595773
}
57605774

57615775
CompileType BinaryUint32OpInstr::ComputeType() const {

‎runtime/vm/compiler/backend/il_x64.cc

+8-3
Original file line numberDiff line numberDiff line change
@@ -5587,7 +5587,9 @@ LocationSummary* ShiftInt64OpInstr::MakeLocationSummary(Zone* zone,
55875587
LocationSummary* summary = new (zone) LocationSummary(
55885588
zone, kNumInputs, kNumTemps, LocationSummary::kCallOnSlowPath);
55895589
summary->set_in(0, Location::RequiresRegister());
5590-
summary->set_in(1, Location::FixedRegisterOrConstant(right(), RCX));
5590+
summary->set_in(1, RangeUtils::IsPositive(shift_range())
5591+
? Location::FixedRegisterOrConstant(right(), RCX)
5592+
: Location::RegisterLocation(RCX));
55915593
summary->set_out(0, Location::SameAsFirstInput());
55925594
return summary;
55935595
}
@@ -5602,7 +5604,7 @@ void ShiftInt64OpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
56025604
EmitShiftInt64ByConstant(compiler, op_kind(), left,
56035605
locs()->in(1).constant());
56045606
} else {
5605-
// Code for a variable shift amount.
5607+
// Code for a variable shift amount (or constant that throws).
56065608
ASSERT(locs()->in(1).reg() == RCX);
56075609

56085610
// Jump to a slow path if shift count is > 63 or negative.
@@ -5705,7 +5707,9 @@ LocationSummary* ShiftUint32OpInstr::MakeLocationSummary(Zone* zone,
57055707
LocationSummary* summary = new (zone) LocationSummary(
57065708
zone, kNumInputs, kNumTemps, LocationSummary::kCallOnSlowPath);
57075709
summary->set_in(0, Location::RequiresRegister());
5708-
summary->set_in(1, Location::FixedRegisterOrConstant(right(), RCX));
5710+
summary->set_in(1, RangeUtils::IsPositive(shift_range())
5711+
? Location::FixedRegisterOrConstant(right(), RCX)
5712+
: Location::RegisterLocation(RCX));
57095713
summary->set_out(0, Location::SameAsFirstInput());
57105714
return summary;
57115715
}
@@ -5719,6 +5723,7 @@ void ShiftUint32OpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
57195723
EmitShiftUint32ByConstant(compiler, op_kind(), left,
57205724
locs()->in(1).constant());
57215725
} else {
5726+
// Code for a variable shift amount (or constant that throws).
57225727
ASSERT(locs()->in(1).reg() == RCX);
57235728

57245729
// Jump to a slow path if shift count is > 31 or negative.

‎tests/language_2/vm/negate_int_test.dart

+46
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// Copyright (c) 2018, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
// VMOptions=--no_background_compilation --optimization_counter_threshold=10
6+
7+
import "package:expect/expect.dart";
8+
9+
// Tests for long negations under
10+
// 64-bit arithmetic wrap-around semantics.
11+
12+
final int maxInt32 = 2147483647;
13+
final int minInt32 = -2147483648;
14+
final int maxInt64 = 0x7fffffffffffffff;
15+
final int minInt64 = 0x8000000000000000;
16+
17+
int negate(int x) {
18+
return -x;
19+
}
20+
21+
doConstant() {
22+
Expect.equals(1, negate(-1));
23+
Expect.equals(0, negate(0));
24+
Expect.equals(-1, negate(1));
25+
26+
Expect.equals(-maxInt32, negate(maxInt32));
27+
Expect.equals(-minInt32, negate(minInt32));
28+
Expect.equals(-maxInt64, negate(maxInt64));
29+
Expect.equals(minInt64, negate(minInt64)); // sic!
30+
}
31+
32+
doVar() {
33+
int d = 0;
34+
for (int i = -88; i < 10; i++) {
35+
d += negate(i);
36+
}
37+
Expect.equals(3871, d);
38+
}
39+
40+
main() {
41+
// Repeat tests to enter JIT (when applicable).
42+
for (int i = 0; i < 20; i++) {
43+
doConstant();
44+
doVar();
45+
}
46+
}

0 commit comments

Comments
 (0)