Skip to content

Commit

Permalink
[vm/compiler] Cache rhs range for all binary integer instructions
Browse files Browse the repository at this point in the history 
Also, use the same rhs operand predicates for all binary integer
instructions.

TEST=ci

Change-Id: I7e0d3d87f0f4e74066a5be16f3aeed533e0095d8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/396621
Reviewed-by: Slava Egorov <[email protected]>
Commit-Queue: Alexander Markov <[email protected]>
  • Loading branch information
@alexmarkov
alexmarkov authored and Commit Queue committed Dec 2, 2024
1 parent 6c4c1cb commit 110c00d
Show file tree
Hide file tree
Showing 9 changed files with 174 additions and 200 deletions.
7 changes: 4 additions & 3 deletions runtime/vm/compiler/backend/flow_graph.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2762,9 +2762,10 @@ void FlowGraph::TryMergeTruncDivMod(
kSmiCid);

// Replace with TruncDivMod.
TruncDivModInstr* div_mod = new (Z) TruncDivModInstr(
curr_instr->left()->CopyWithType(),
curr_instr->right()->CopyWithType(), curr_instr->deopt_id());
TruncDivModInstr* div_mod =
new (Z) TruncDivModInstr(curr_instr->left()->CopyWithType(),
curr_instr->right()->CopyWithType(),
curr_instr->DeoptimizationTarget());
curr_instr->ReplaceWith(div_mod, nullptr);
other_binop->ReplaceUsesWith(div_mod);
other_binop->RemoveFromGraph();
Expand Down
62 changes: 31 additions & 31 deletions runtime/vm/compiler/backend/il.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2069,43 +2069,60 @@ bool BinarySmiOpInstr::ComputeCanDeoptimize() const {
return false;

case Token::kSHR:
return !RangeUtils::IsPositive(right_range());
return !RightOperandIsPositive();

case Token::kUSHR:
case Token::kSHL:
return can_overflow() || !RangeUtils::IsPositive(right_range());
return can_overflow() || !RightOperandIsPositive();

case Token::kMOD:
return RangeUtils::CanBeZero(right_range());
return RightOperandCanBeZero();

case Token::kTRUNCDIV:
return RangeUtils::CanBeZero(right_range()) ||
RangeUtils::Overlaps(right_range(), -1, -1);
return RightOperandCanBeZero() || RightOperandCanBeMinusOne();

default:
return can_overflow();
}
}

bool BinaryIntegerOpInstr::RightIsNonZero() const {
bool BinaryIntegerOpInstr::RightOperandCanBeZero() const {
if (right()->BindsToConstant()) {
const auto& constant = right()->BoundConstant();
if (!constant.IsInteger()) return false;
return Integer::Cast(constant).Value() != 0;
if (!constant.IsInteger()) return true;
return Integer::Cast(constant).Value() == 0;
}
return RangeUtils::CanBeZero(right_range());
}

bool BinaryIntegerOpInstr::RightOperandCanBeMinusOne() const {
if (right()->BindsToConstant()) {
const auto& constant = right()->BoundConstant();
if (!constant.IsInteger()) return true;
return Integer::Cast(constant).Value() == -1;
}
return !RangeUtils::CanBeZero(right()->definition()->range());
return RangeUtils::Overlaps(right_range(), -1, -1);
}

bool BinaryIntegerOpInstr::RightIsPositive() const {
bool BinaryIntegerOpInstr::RightOperandIsPositive() const {
if (right()->BindsToConstant()) {
const auto& constant = right()->BoundConstant();
if (!constant.IsInteger()) return false;
return Integer::Cast(constant).Value() > 0;
}
return RangeUtils::IsPositive(right()->definition()->range());
return RangeUtils::IsPositive(right_range());
}

bool BinaryIntegerOpInstr::RightOperandIsNegative() const {
if (right()->BindsToConstant()) {
const auto& constant = right()->BoundConstant();
if (!constant.IsInteger()) return false;
return Integer::Cast(constant).Value() < 0;
}
return RangeUtils::IsNegative(right_range());
}

bool BinaryIntegerOpInstr::RightIsPowerOfTwoConstant() const {
bool BinaryIntegerOpInstr::RightOperandIsPowerOfTwoConstant() const {
if (!right()->BindsToConstant()) return false;
const Object& constant = right()->BoundConstant();
if (!constant.IsSmi()) return false;
Expand All @@ -2121,7 +2138,7 @@ bool BinaryIntegerOpInstr::IsShiftCountInRange(int64_t max) const {
const int64_t value = Integer::Cast(constant).Value();
return (0 <= value) && (value <= max);
}
return RangeUtils::IsWithin(right()->definition()->range(), 0, max);
return RangeUtils::IsWithin(right_range(), 0, max);
}

static intptr_t RepresentationBits(Representation r) {
Expand Down Expand Up @@ -2279,26 +2296,9 @@ BinaryIntegerOpInstr* BinaryIntegerOpInstr::Make(Representation representation,
Value* right,
intptr_t deopt_id) {
BinaryIntegerOpInstr* op = nullptr;
Range* right_range = nullptr;
switch (op_kind) {
case Token::kMOD:
case Token::kTRUNCDIV:
if (representation != kTagged) break;
FALL_THROUGH;
case Token::kSHL:
case Token::kSHR:
case Token::kUSHR:
if (auto const const_def = right->definition()->AsConstant()) {
right_range = new Range();
const_def->InferRange(nullptr, right_range);
}
break;
default:
break;
}
switch (representation) {
case kTagged:
op = new BinarySmiOpInstr(op_kind, left, right, deopt_id, right_range);
op = new BinarySmiOpInstr(op_kind, left, right, deopt_id);
break;
case kUnboxedInt32:
if (!BinaryInt32OpInstr::IsSupported(op_kind, left, right)) {
Expand Down
58 changes: 28 additions & 30 deletions runtime/vm/compiler/backend/il.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9173,14 +9173,17 @@ class UnaryInt64OpInstr : public UnaryIntegerOpInstr {

class BinaryIntegerOpInstr : public TemplateDefinition<2, NoThrow, Pure> {
public:
static constexpr intptr_t kShiftCountLimit = 63;

BinaryIntegerOpInstr(Token::Kind op_kind,
Value* left,
Value* right,
intptr_t deopt_id)
: TemplateDefinition(deopt_id),
op_kind_(op_kind),
can_overflow_(true),
is_truncating_(false) {
is_truncating_(false),
right_range_(nullptr) {
SetInputAt(0, left);
SetInputAt(1, right);
}
Expand Down Expand Up @@ -9216,16 +9219,25 @@ class BinaryIntegerOpInstr : public TemplateDefinition<2, NoThrow, Pure> {
set_can_overflow(false);
}

// Returns true if right is either a non-zero Integer constant or has a range
// that does not include the possibility of being zero.
bool RightIsNonZero() const;
// Returns true if compiler cannot prove that rhs operand is not zero.
bool RightOperandCanBeZero() const;

// Returns true if compiler cannot prove that rhs operand is not -1.
bool RightOperandCanBeMinusOne() const;

// Returns true if rhs operand is positive.
bool RightIsPositive() const;
bool RightOperandIsPositive() const;

// Returns true if right is a non-zero Smi constant which absolute value is
// a power of two.
bool RightIsPowerOfTwoConstant() const;
// Returns true if rhs operand is negative.
bool RightOperandIsNegative() const;

// Returns true if rhs opernad is a non-zero Smi constant which
// absolute value is a power of two.
bool RightOperandIsPowerOfTwoConstant() const;

// Returns true if the shift amount (right operand) is guaranteed to be in
// [0..max] range.
bool IsShiftCountInRange(int64_t max = kShiftCountLimit) const;

virtual Definition* Canonicalize(FlowGraph* flow_graph);

Expand All @@ -9244,24 +9256,21 @@ class BinaryIntegerOpInstr : public TemplateDefinition<2, NoThrow, Pure> {
#define FIELD_LIST(F) \
F(const Token::Kind, op_kind_) \
F(bool, can_overflow_) \
F(bool, is_truncating_)
F(bool, is_truncating_) \
F(Range*, right_range_)

DECLARE_INSTRUCTION_SERIALIZABLE_FIELDS(BinaryIntegerOpInstr,
TemplateDefinition,
FIELD_LIST)
#undef FIELD_LIST

protected:
Range* right_range() const { return right_range_; }

void InferRangeHelper(const Range* left_range,
const Range* right_range,
Range* range);

static constexpr intptr_t kShiftCountLimit = 63;

// Returns true if the shift amount (right operand) is guaranteed to be in
// [0..max] range.
bool IsShiftCountInRange(int64_t max = kShiftCountLimit) const;

private:
Definition* CreateConstantResult(FlowGraph* graph, const Integer& result);

Expand All @@ -9273,27 +9282,16 @@ class BinarySmiOpInstr : public BinaryIntegerOpInstr {
BinarySmiOpInstr(Token::Kind op_kind,
Value* left,
Value* right,
intptr_t deopt_id,
// Provided by BinaryIntegerOpInstr::Make for constant RHS.
Range* right_range = nullptr)
: BinaryIntegerOpInstr(op_kind, left, right, deopt_id),
right_range_(right_range) {}
intptr_t deopt_id)
: BinaryIntegerOpInstr(op_kind, left, right, deopt_id) {}

virtual bool ComputeCanDeoptimize() const;

virtual void InferRange(RangeAnalysis* analysis, Range* range);
virtual CompileType ComputeType() const;

DECLARE_INSTRUCTION(BinarySmiOp)

Range* right_range() const { return right_range_; }

#define FIELD_LIST(F) F(Range*, right_range_)

DECLARE_INSTRUCTION_SERIALIZABLE_FIELDS(BinarySmiOpInstr,
BinaryIntegerOpInstr,
FIELD_LIST)
#undef FIELD_LIST
DECLARE_EMPTY_SERIALIZATION(BinarySmiOpInstr, BinaryIntegerOpInstr)

private:
DISALLOW_COPY_AND_ASSIGN(BinarySmiOpInstr);
Expand Down Expand Up @@ -9433,7 +9431,7 @@ class BinaryInt64OpInstr : public BinaryIntegerOpInstr {
return !IsShiftCountInRange();
case Token::kMOD:
case Token::kTRUNCDIV:
return !RightIsNonZero();
return RightOperandCanBeZero();
default:
return false;
}
Expand Down
30 changes: 13 additions & 17 deletions runtime/vm/compiler/backend/il_arm.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3399,7 +3399,6 @@ static void EmitSmiShiftLeft(FlowGraphCompiler* compiler,

// Right (locs.in(1)) is not constant.
const Register right = locs.in(1).reg();
Range* right_range = shift_left->right_range();
if (shift_left->left()->BindsToConstant() && shift_left->can_overflow()) {
// TODO(srdjan): Implement code below for is_truncating().
// If left is constant, we know the maximal allowed size for right.
Expand All @@ -3415,7 +3414,7 @@ static void EmitSmiShiftLeft(FlowGraphCompiler* compiler,
const intptr_t max_right =
compiler::target::kSmiBits - Utils::HighestBit(left_int);
const bool right_needs_check =
!RangeUtils::IsWithin(right_range, 0, max_right - 1);
!shift_left->IsShiftCountInRange(max_right - 1);
if (right_needs_check) {
__ cmp(right, compiler::Operand(compiler::target::ToRawSmi(max_right)));
__ b(deopt, CS);
Expand All @@ -3427,10 +3426,10 @@ static void EmitSmiShiftLeft(FlowGraphCompiler* compiler,
}

const bool right_needs_check =
!RangeUtils::IsWithin(right_range, 0, (compiler::target::kSmiBits - 1));
!shift_left->IsShiftCountInRange(compiler::target::kSmiBits - 1);
if (!shift_left->can_overflow()) {
if (right_needs_check) {
if (!RangeUtils::IsPositive(right_range)) {
if (!shift_left->RightOperandIsPositive()) {
ASSERT(shift_left->CanDeoptimize());
__ cmp(right, compiler::Operand(0));
__ b(deopt, MI);
Expand Down Expand Up @@ -3471,7 +3470,7 @@ LocationSummary* BinarySmiOpInstr::MakeLocationSummary(Zone* zone,
// Calculate number of temporaries.
intptr_t num_temps = 0;
if (op_kind() == Token::kTRUNCDIV) {
if (RightIsPowerOfTwoConstant()) {
if (RightOperandIsPowerOfTwoConstant()) {
num_temps = 1;
} else {
num_temps = 2;
Expand All @@ -3486,7 +3485,7 @@ LocationSummary* BinarySmiOpInstr::MakeLocationSummary(Zone* zone,
LocationSummary(zone, kNumInputs, num_temps, LocationSummary::kNoCall);
if (op_kind() == Token::kTRUNCDIV) {
summary->set_in(0, Location::RequiresRegister());
if (RightIsPowerOfTwoConstant()) {
if (RightOperandIsPowerOfTwoConstant()) {
ConstantInstr* right_constant = right()->definition()->AsConstant();
summary->set_in(1, Location::Constant(right_constant));
summary->set_temp(0, Location::RequiresRegister());
Expand Down Expand Up @@ -3739,7 +3738,7 @@ void BinarySmiOpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
break;
}
case Token::kTRUNCDIV: {
if (RangeUtils::CanBeZero(right_range())) {
if (RightOperandCanBeZero()) {
// Handle divide by zero in runtime.
__ cmp(right, compiler::Operand(0));
__ b(deopt, EQ);
Expand All @@ -3750,7 +3749,7 @@ void BinarySmiOpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
__ SmiUntag(IP, right);
__ IntegerDivide(result, temp, IP, dtemp, DTMP);

if (RangeUtils::Overlaps(right_range(), -1, -1)) {
if (RightOperandCanBeMinusOne()) {
// Check the corner case of dividing the 'MIN_SMI' with -1, in which
// case we cannot tag the result.
__ CompareImmediate(result, 0x40000000);
Expand All @@ -3760,7 +3759,7 @@ void BinarySmiOpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
break;
}
case Token::kMOD: {
if (RangeUtils::CanBeZero(right_range())) {
if (RightOperandCanBeZero()) {
// Handle divide by zero in runtime.
__ cmp(right, compiler::Operand(0));
__ b(deopt, EQ);
Expand Down Expand Up @@ -3799,7 +3798,7 @@ void BinarySmiOpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
__ SmiUntag(IP, right);
// sarl operation masks the count to 5 bits.
const intptr_t kCountLimit = 0x1F;
if (!RangeUtils::OnlyLessThanOrEqualTo(right_range(), kCountLimit)) {
if (!IsShiftCountInRange(kCountLimit)) {
__ CompareImmediate(IP, kCountLimit);
__ LoadImmediate(IP, kCountLimit, GT);
}
Expand Down Expand Up @@ -3827,10 +3826,8 @@ void BinarySmiOpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
// If left operand is non-negative, the result always
// fits into Smi range.
//
if (!RangeUtils::OnlyLessThanOrEqualTo(
right_range(), 64 - compiler::target::kSmiBits - 1)) {
if (!RangeUtils::OnlyLessThanOrEqualTo(right_range(),
kBitsPerInt64 - 1)) {
if (!IsShiftCountInRange(64 - compiler::target::kSmiBits - 1)) {
if (!IsShiftCountInRange(kBitsPerInt64 - 1)) {
__ CompareImmediate(IP, kBitsPerInt64);
// If shift amount >= 64, then result is 0.
__ LoadImmediate(result, 0, GE);
Expand Down Expand Up @@ -3859,8 +3856,7 @@ void BinarySmiOpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
}
// At this point left operand is non-negative, so unsigned shift
// can't overflow.
if (!RangeUtils::OnlyLessThanOrEqualTo(right_range(),
compiler::target::kSmiBits - 1)) {
if (!IsShiftCountInRange(compiler::target::kSmiBits - 1)) {
__ CompareImmediate(IP, compiler::target::kSmiBits);
// Left operand >= 0, shift amount >= kSmiBits. Result is 0.
__ LoadImmediate(result, 0, GE);
Expand Down Expand Up @@ -6301,7 +6297,7 @@ LocationSummary* BinaryInt64OpInstr::MakeLocationSummary(Zone* zone,
zone, kNumInputs, kNumTemps, LocationSummary::kCallOnSlowPath);
summary->set_in(0, Location::Pair(Location::RequiresRegister(),
Location::RequiresRegister()));
if (RightIsPositive() && right()->definition()->IsConstant()) {
if (RightOperandIsPositive() && right()->definition()->IsConstant()) {
ConstantInstr* constant = right()->definition()->AsConstant();
summary->set_in(1, Location::Constant(constant));
} else {
Expand Down
Loading

0 comments on commit 110c00d

Please sign in to comment.