Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8346664: C2: Optimize mask check with constant offset #22856

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
+251 −170
Open

8346664: C2: Optimize mask check with constant offset #22856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
bd77da8
C2: optimize constant addends in masked sums
mernst-github Dec 21, 2024
2b8da62
review:
mernst-github Dec 24, 2024
bc3c4f0
undo method reordering
mernst-github Dec 24, 2024
ba2f82b
T != X
mernst-github Dec 24, 2024
cabc0d8
counter
mernst-github Dec 24, 2024
df79a96
--more null checks
mernst-github Dec 24, 2024
2a79c24
consistently use bt
mernst-github Dec 24, 2024
1555846
comment readability
mernst-github Jan 2, 2025
e0f44b4
naming, comment
mernst-github Jan 8, 2025
6df13ff
randomize, test "(j + const) & nonconst_mask".
mernst-github Jan 8, 2025
9d3ca06
addConstNonConstMaskLong
mernst-github Jan 8, 2025
cbe6165
avoid redundant comment
mernst-github Jan 8, 2025
35afeb0
"element".
mernst-github Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 66 additions & 96 deletions src/hotspot/share/opto/mulnode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,9 +670,11 @@ const Type *AndINode::mul_ring( const Type *t0, const Type *t1 ) const {
return and_value<TypeInt>(r0, r1);
}

static bool AndIL_is_zero_element_under_mask(const PhaseGVN* phase, const Node* expr, const Node* mask, BasicType bt);

const Type* AndINode::Value(PhaseGVN* phase) const {
// patterns similar to (v << 2) & 3
if (AndIL_shift_and_mask_is_always_zero(phase, in(1), in(2), T_INT, true)) {
if (AndIL_is_zero_element_under_mask(phase, in(1), in(2), T_INT) ||
AndIL_is_zero_element_under_mask(phase, in(2), in(1), T_INT)) {
return TypeInt::ZERO;
}

Expand Down Expand Up @@ -718,8 +720,8 @@ Node* AndINode::Identity(PhaseGVN* phase) {

//------------------------------Ideal------------------------------------------
Node *AndINode::Ideal(PhaseGVN *phase, bool can_reshape) {
// pattern similar to (v1 + (v2 << 2)) & 3 transformed to v1 & 3
Node* progress = AndIL_add_shift_and_mask(phase, T_INT);
// Simplify (v1 + v2) & mask to v1 & mask or v2 & mask when possible.
Node* progress = AndIL_sum_and_mask(phase, T_INT);
if (progress != nullptr) {
return progress;
}
Expand Down Expand Up @@ -802,8 +804,8 @@ const Type *AndLNode::mul_ring( const Type *t0, const Type *t1 ) const {
}

const Type* AndLNode::Value(PhaseGVN* phase) const {
// patterns similar to (v << 2) & 3
if (AndIL_shift_and_mask_is_always_zero(phase, in(1), in(2), T_LONG, true)) {
if (AndIL_is_zero_element_under_mask(phase, in(1), in(2), T_LONG) ||
AndIL_is_zero_element_under_mask(phase, in(2), in(1), T_LONG)) {
return TypeLong::ZERO;
}

Expand Down Expand Up @@ -850,8 +852,8 @@ Node* AndLNode::Identity(PhaseGVN* phase) {

//------------------------------Ideal------------------------------------------
Node *AndLNode::Ideal(PhaseGVN *phase, bool can_reshape) {
// pattern similar to (v1 + (v2 << 2)) & 3 transformed to v1 & 3
Node* progress = AndIL_add_shift_and_mask(phase, T_LONG);
// Simplify (v1 + v2) & mask to v1 & mask or v2 & mask when possible.
Node* progress = AndIL_sum_and_mask(phase, T_LONG);
if (progress != nullptr) {
return progress;
}
Expand Down Expand Up @@ -2052,99 +2054,69 @@ const Type* RotateRightNode::Value(PhaseGVN* phase) const {
}
}

// Given an expression (AndX shift mask) or (AndX mask shift),
// determine if the AndX must always produce zero, because the
// the shift (x<<N) is bitwise disjoint from the mask #M.
// The X in AndX must be I or L, depending on bt.
// Specifically, the following cases fold to zero,
// when the shift value N is large enough to zero out
// all the set positions of the and-mask M.
// (AndI (LShiftI _ #N) #M) => #0
// (AndL (LShiftL _ #N) #M) => #0
// (AndL (ConvI2L (LShiftI _ #N)) #M) => #0
// The M and N values must satisfy ((-1 << N) & M) == 0.
// Because the optimization might work for a non-constant
// mask M, we check the AndX for both operand orders.
bool MulNode::AndIL_shift_and_mask_is_always_zero(PhaseGVN* phase, Node* shift, Node* mask, BasicType bt, bool check_reverse) {
if (mask == nullptr || shift == nullptr) {
return false;
}
const TypeInteger* mask_t = phase->type(mask)->isa_integer(bt);
if (mask_t == nullptr || phase->type(shift)->isa_integer(bt) == nullptr) {
return false;
}
shift = shift->uncast();
if (shift == nullptr) {
return false;
//------------------------------ Sum & Mask ------------------------------

// Returns a lower bound on the number of trailing zeros in expr.
static jint AndIL_min_trailing_zeros(const PhaseGVN* phase, const Node* expr, BasicType bt) {
Comment on lines +2059 to +2060
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method restricted to use in AndIL? Because it looks like it is doing something more generic: trying to figure out a lower bound on the trailing zeros of an expression.

If that is the case: Why not put it in Node::get_trailing_zeros_lower_bound(phase, bt), so it can be used elsewhere too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that while this might be incidentally reusable outside of the scope of "And" nodes, as long as there is no actual demand to reuse this, I would rather not add it to the rather prominent Node class to avoid api bloat.

Iff the notion of "is known to be a multiple of a certain power of two" is really of general interest, I would expect it to become part of TypeInteger.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, just leave it where it is for now. I'm ok with it.

expr = expr->uncast();
const TypeInteger* type = phase->type(expr)->isa_integer(bt);
if (type == nullptr) {
return 0;
}
if (phase->type(shift)->isa_integer(bt) == nullptr) {
return false;

if (type->is_con()) {
long con = type->get_con_as_long(bt);
return con == 0L ? (type2aelembytes(bt) * BitsPerByte) : count_trailing_zeros(con);
}
BasicType shift_bt = bt;
if (bt == T_LONG && shift->Opcode() == Op_ConvI2L) {

if (expr->Opcode() == Op_ConvI2L) {
expr = expr->in(1)->uncast();
bt = T_INT;
Node* val = shift->in(1);
if (val == nullptr) {
return false;
}
val = val->uncast();
if (val == nullptr) {
return false;
}
if (val->Opcode() == Op_LShiftI) {
shift_bt = T_INT;
shift = val;
if (phase->type(shift)->isa_integer(bt) == nullptr) {
return false;
}
}
}
if (shift->Opcode() != Op_LShift(shift_bt)) {
if (check_reverse &&
(mask->Opcode() == Op_LShift(bt) ||
(bt == T_LONG && mask->Opcode() == Op_ConvI2L))) {
// try it the other way around
return AndIL_shift_and_mask_is_always_zero(phase, mask, shift, bt, false);
}
return false;
}
Node* shift2 = shift->in(2);
if (shift2 == nullptr) {
return false;
}
const Type* shift2_t = phase->type(shift2);
if (!shift2_t->isa_int() || !shift2_t->is_int()->is_con()) {
return false;
type = phase->type(expr)->isa_int();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are trying to look through a ConvI2L, I think for the sake of consistency, you can reassign bt to T_INT at this point.

Copy link
Author

@mernst-github mernst-github Dec 24, 2024 •
edited by bridgekeeper bot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

jint shift_con = shift2_t->is_int()->get_con() & ((shift_bt == T_INT ? BitsPerJavaInteger : BitsPerJavaLong) - 1);
if ((((jlong)1) << shift_con) > mask_t->hi_as_long() && mask_t->lo_as_long() >= 0) {
return true;
if (expr->Opcode() == Op_LShift(bt)) {
const TypeInt* rhs_t = phase->type(expr->in(2))->isa_int();
if (rhs_t == nullptr || !rhs_t->is_con()) {
return 0;
}
return rhs_t->get_con() % (type2aelembytes(bt) * BitsPerByte);
}

return false;
return 0;
}

// Given an expression (AndX (AddX v1 (LShiftX v2 #N)) #M)
// determine if the AndX must always produce (AndX v1 #M),
// because the shift (v2<<N) is bitwise disjoint from the mask #M.
// The X in AndX will be I or L, depending on bt.
// Specifically, the following cases fold,
// Checks whether expr is neutral wrt addition under mask, i.e.
// an expression of the form (AndX (T+expr) mask) can be simplified to (AndX T mask).
// The X in AndX must be I or L, depending on bt.
// Specifically, this holds for the following cases,
// when the shift value N is large enough to zero out
// all the set positions of the and-mask M.
// (AndI (AddI v1 (LShiftI _ #N)) #M) => (AndI v1 #M)
// (AndL (AddI v1 (LShiftL _ #N)) #M) => (AndL v1 #M)
// (AndL (AddL v1 (ConvI2L (LShiftI _ #N))) #M) => (AndL v1 #M)
// all the set positions of the and-mask M:
// (AndI (LShiftI _ #N) #M)
// (AndL (LShiftL _ #N) #M)
// (AndL (ConvI2L (LShiftI _ #N)) #M)
// including equivalent constant operands:
// (AndI (ConI (_ << #N)) #M)
// (AndL (ConL (_ << #N)) #M)
// The M and N values must satisfy ((-1 << N) & M) == 0.
// Because the optimization might work for a non-constant
// mask M, and because the AddX operands can come in either
// order, we check for every operand order.
Node* MulNode::AndIL_add_shift_and_mask(PhaseGVN* phase, BasicType bt) {
static bool AndIL_is_zero_element_under_mask(const PhaseGVN* phase, const Node* expr, const Node* mask, BasicType bt) {
const TypeInteger* mask_t = phase->type(mask)->isa_integer(bt);
if (mask_t == nullptr) {
return false;
}

jint zeros = AndIL_min_trailing_zeros(phase, expr, bt);
return zeros > 0 && ((((jlong)1) << zeros) > mask_t->hi_as_long() && mask_t->lo_as_long() >= 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line indicates that the mask could be a variable. You should make sure to add some tests for that in your patterns. You can create a variable in a specific range like this Math.min(5, Math.max(1, x)), should get you x clamped into the region 1..5.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a variant for adding consts using the same pattern as the other "NonConstMask" tests.

}

// Given an expression (AndX (AddX v1 v2) mask)
// determine if the AndX must always produce (AndX v1 mask),
// because v2 is zero wrt addition under mask.
// Because the AddX operands can come in either
// order, we check for both orders.
Node* MulNode::AndIL_sum_and_mask(PhaseGVN* phase, BasicType bt) {
Node* add = in(1);
Node* mask = in(2);
if (add == nullptr || mask == nullptr) {
return nullptr;
}
int addidx = 0;
if (add->Opcode() == Op_Add(bt)) {
addidx = 1;
Expand All @@ -2156,14 +2128,12 @@ Node* MulNode::AndIL_add_shift_and_mask(PhaseGVN* phase, BasicType bt) {
if (addidx > 0) {
Node* add1 = add->in(1);
Node* add2 = add->in(2);
if (add1 != nullptr && add2 != nullptr) {
if (AndIL_shift_and_mask_is_always_zero(phase, add1, mask, bt, false)) {
set_req_X(addidx, add2, phase);
return this;
} else if (AndIL_shift_and_mask_is_always_zero(phase, add2, mask, bt, false)) {
set_req_X(addidx, add1, phase);
return this;
}
if (AndIL_is_zero_element_under_mask(phase, add1, mask, bt)) {
set_req_X(addidx, add2, phase);
return this;
} else if (AndIL_is_zero_element_under_mask(phase, add2, mask, bt)) {
set_req_X(addidx, add1, phase);
return this;
}
}
return nullptr;
Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/opto/mulnode.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ class MulNode : public Node {

static MulNode* make(Node* in1, Node* in2, BasicType bt);

static bool AndIL_shift_and_mask_is_always_zero(PhaseGVN* phase, Node* shift, Node* mask, BasicType bt, bool check_reverse);
Node* AndIL_add_shift_and_mask(PhaseGVN* phase, BasicType bt);
protected:
Node* AndIL_sum_and_mask(PhaseGVN* phase, BasicType bt);
};

//------------------------------MulINode---------------------------------------
Expand Down
Loading