JIT: Refactor morph cmp optimizations

[BoyBaykiller]

Unifying the compare opts to naturally handle canonicalization of constant to the right for GT_EQ, GT_NE. This now also calls fgOptimizeCmpWithCasts for GT_EQ, GT_NE which was missing too.

I also added 2 canonicalizations:

1. '(A & pow2) == pow2' -> '(A & pow2) != 0'
2. '(A & pow2) != pow2' -> '(A & pow2) == 0'

These will help optimizeBools and consequentenly #126852 and probably other stuff. I'd also like this for #125899 if I add e.g SELECT(x & 16 == 16, y | 16, y) -> y | (x & 16)

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[EgorBo]

I see that there is already a logic that moves constants to the right, why do we repeat it here?

[BoyBaykiller]

I totally agreee. But the fact that its written in a switch makes it hard/ugly to freely share code. I noticed the exact thing when I was adding fgOptimizeDistributiveArithemtic from my other PR. Which personally motivates me to rewrite this whole thing to using classic if stmts without switch

[EgorBo]

I'm not sure I follow, for me it sounds like it repeats work that should be done elsewhere. It's already done in fgMorphSmpOp for all comparisons, we probably might want to have a some shared way to do that for all commutative opers

[BoyBaykiller]

I though you are talking about this right down below from where I stole it which only does it for some:

runtime/src/coreclr/jit/morph.cpp

Lines 7792 to 7809 in e19c810

	caseGT_LT:
	caseGT_LE:
	caseGT_GE:
	caseGT_GT:
	// Change "CNS relop op2" to "op2 relop* CNS"
	if (op1->IsIntegralConst() && tree->OperIsCompare() && gtCanSwapOrder(op1, op2))
	{
	std::swap(tree->AsOp()->gtOp1, tree->AsOp()->gtOp2);
	tree->gtOper = GenTree::SwapRelop(tree->OperGet());
	oper = tree->OperGet();
	op1 = tree->gtGetOp1();
	op2 = tree->gtGetOp2();
	}
	if (op1->OperIs(GT_CAST) || op2->OperIs(GT_CAST))
	{

Not sure what place you are refering to

[BoyBaykiller]

we probably might want to have a some shared way to do that for all commutative opers

That'd be nice

[tannergooding]

The check just below it is also notably too lenient, it is valid for floating-point as well and we handle that for SIMD nodes.

You can likely just extract it out to a small helper function if you don't want to duplicate the logic between EQ/NE and the other compares, although notably EQ/NE are simpler since they are commutative.

[tannergooding]

If you happen to know where this code is that should already be handling comparisons, please let me know. So I can try to improve it. I will try to find it now.

Looks like it might indeed be missing, outside of costing.

[tannergooding]

I'd recommend doing that one in it's own PR (it's much more broadly impacting and not isolated to the ispow2 work) and ideally fixing it to not only do it for integrals, we can do it for all types just fine and just check SwapRelop

[BoyBaykiller]

and ideally fixing it to not only do it for integrals, we can do it for all types

are there optimizations on relops with floats? if yes where

[tannergooding]

In some cases, yes, and we do folding and other transforms based on them. There are more for vectors than there are for scalars right now, but ideally they are mirrored. In general we want to push constants right for other reasons as well, such as CSE, containment, VN, etc.

[tannergooding]

nit: This is not correct as it checks for GT_CNS_INT where you must instead check for TypeIs(TYP_INT)

[BoyBaykiller]

if (IsIntegralConst())
{
if (TypeIs(TYP_INT))
{
returnisPow2((uint32_t)AsIntCon()->IconValue());
}
returnisPow2((uint64_t)AsLngCon()->LngValue());
}

Input:

boolTest1(longA,longB){return8==(A&8);}

Gives me: Assertion failed 'OperIs(GT_CNS_LNG)'. I guess I should use AsIntConCommon()->IntegralValue().

[tannergooding]

What platform are you getting the assertion on? and for which line?

There shouldn't be any GT_CNS_LNG being produced on 64-bit, they all get routed to GT_CNS_INT. Then you shouldn't be producing a GT_CNS_LNG for TYP_INT on 32-bit either, correspondingly.

inlinessize_tGenTreeIntConCommon::IconValue() const
{
assert(OperIs(GT_CNS_INT)); // We should never see a GT_CNS_LNG for a 64-bit target!returnAsIntCon()->gtIconVal;
}

and

inlineINT64GenTreeIntConCommon::LngValue() const
{
#ifndef TARGET_64BIT
assert(OperIs(GT_CNS_LNG));
returnAsLngCon()->gtLconVal;
#elsereturnIconValue();
#endif
}

and

GenTreeLngCon(INT64 val)
: GenTreeIntConCommon(GT_CNS_NATIVELONG, TYP_LONG)
{
SetLngValue(val);
}

and

GenTreeIntCon(var_types type, ssize_t value DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(nullptr)
{
}
GenTreeIntCon(var_types type, ssize_t value, FieldSeq* fields DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(fields)
{
}

[BoyBaykiller]

I will be able to properly answer you tomorrow but you should be able to reproduce on win-x64 main.

[BoyBaykiller]

Split into
Canon (cns to right): #127661 (more general, better version)
Canon (bit-test): #128533
Pow2: #127615
Unify: #127883

[BoyBaykiller]

Unifying the compare opts to naturally handle canonicalization of constant to the right for GT_EQ, GT_NE. This now also calls fgOptimizeCmpWithCasts for GT_EQ, GT_NE which was missing too.

I also added 2 canonicalizations:

1. '(A & pow2) == pow2' -> '(A & pow2) != 0'
2. '(A & pow2) != pow2' -> '(A & pow2) == 0'

These will help optimizeBools and consequentenly #126852 and probably other stuff. I'd also like this for #125899 if I add e.g SELECT(x & 16 == 16, y | 16, y) -> y | (x & 16)

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[EgorBo]

I see that there is already a logic that moves constants to the right, why do we repeat it here?

[BoyBaykiller]

I totally agreee. But the fact that its written in a switch makes it hard/ugly to freely share code. I noticed the exact thing when I was adding fgOptimizeDistributiveArithemtic from my other PR. Which personally motivates me to rewrite this whole thing to using classic if stmts without switch

[EgorBo]

I'm not sure I follow, for me it sounds like it repeats work that should be done elsewhere. It's already done in fgMorphSmpOp for all comparisons, we probably might want to have a some shared way to do that for all commutative opers

[BoyBaykiller]

I though you are talking about this right down below from where I stole it which only does it for some:

runtime/src/coreclr/jit/morph.cpp

Lines 7792 to 7809 in e19c810

	caseGT_LT:
	caseGT_LE:
	caseGT_GE:
	caseGT_GT:
	// Change "CNS relop op2" to "op2 relop* CNS"
	if (op1->IsIntegralConst() && tree->OperIsCompare() && gtCanSwapOrder(op1, op2))
	{
	std::swap(tree->AsOp()->gtOp1, tree->AsOp()->gtOp2);
	tree->gtOper = GenTree::SwapRelop(tree->OperGet());
	oper = tree->OperGet();
	op1 = tree->gtGetOp1();
	op2 = tree->gtGetOp2();
	}
	if (op1->OperIs(GT_CAST) || op2->OperIs(GT_CAST))
	{

Not sure what place you are refering to

[BoyBaykiller]

we probably might want to have a some shared way to do that for all commutative opers

That'd be nice

[tannergooding]

The check just below it is also notably too lenient, it is valid for floating-point as well and we handle that for SIMD nodes.

You can likely just extract it out to a small helper function if you don't want to duplicate the logic between EQ/NE and the other compares, although notably EQ/NE are simpler since they are commutative.

[tannergooding]

If you happen to know where this code is that should already be handling comparisons, please let me know. So I can try to improve it. I will try to find it now.

Looks like it might indeed be missing, outside of costing.

[tannergooding]

I'd recommend doing that one in it's own PR (it's much more broadly impacting and not isolated to the ispow2 work) and ideally fixing it to not only do it for integrals, we can do it for all types just fine and just check SwapRelop

[BoyBaykiller]

and ideally fixing it to not only do it for integrals, we can do it for all types

are there optimizations on relops with floats? if yes where

[tannergooding]

In some cases, yes, and we do folding and other transforms based on them. There are more for vectors than there are for scalars right now, but ideally they are mirrored. In general we want to push constants right for other reasons as well, such as CSE, containment, VN, etc.

[tannergooding]

nit: This is not correct as it checks for GT_CNS_INT where you must instead check for TypeIs(TYP_INT)

[BoyBaykiller]

if (IsIntegralConst())
{
if (TypeIs(TYP_INT))
{
returnisPow2((uint32_t)AsIntCon()->IconValue());
}
returnisPow2((uint64_t)AsLngCon()->LngValue());
}

Input:

boolTest1(longA,longB){return8==(A&8);}

Gives me: Assertion failed 'OperIs(GT_CNS_LNG)'. I guess I should use AsIntConCommon()->IntegralValue().

[tannergooding]

What platform are you getting the assertion on? and for which line?

There shouldn't be any GT_CNS_LNG being produced on 64-bit, they all get routed to GT_CNS_INT. Then you shouldn't be producing a GT_CNS_LNG for TYP_INT on 32-bit either, correspondingly.

inlinessize_tGenTreeIntConCommon::IconValue() const
{
assert(OperIs(GT_CNS_INT)); // We should never see a GT_CNS_LNG for a 64-bit target!returnAsIntCon()->gtIconVal;
}

and

inlineINT64GenTreeIntConCommon::LngValue() const
{
#ifndef TARGET_64BIT
assert(OperIs(GT_CNS_LNG));
returnAsLngCon()->gtLconVal;
#elsereturnIconValue();
#endif
}

and

GenTreeLngCon(INT64 val)
: GenTreeIntConCommon(GT_CNS_NATIVELONG, TYP_LONG)
{
SetLngValue(val);
}

and

GenTreeIntCon(var_types type, ssize_t value DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(nullptr)
{
}
GenTreeIntCon(var_types type, ssize_t value, FieldSeq* fields DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(fields)
{
}

[BoyBaykiller]

I will be able to properly answer you tomorrow but you should be able to reproduce on win-x64 main.

[BoyBaykiller]

Split into
Canon (cns to right): #127661 (more general, better version)
Canon (bit-test): #128533
Pow2: #127615
Unify: #127883

[BoyBaykiller]

Unifying the compare opts to naturally handle canonicalization of constant to the right for GT_EQ, GT_NE. This now also calls fgOptimizeCmpWithCasts for GT_EQ, GT_NE which was missing too.

I also added 2 canonicalizations:

1. '(A & pow2) == pow2' -> '(A & pow2) != 0'
2. '(A & pow2) != pow2' -> '(A & pow2) == 0'

These will help optimizeBools and consequentenly #126852 and probably other stuff. I'd also like this for #125899 if I add e.g SELECT(x & 16 == 16, y | 16, y) -> y | (x & 16)

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[EgorBo]

I see that there is already a logic that moves constants to the right, why do we repeat it here?

[BoyBaykiller]

I totally agreee. But the fact that its written in a switch makes it hard/ugly to freely share code. I noticed the exact thing when I was adding fgOptimizeDistributiveArithemtic from my other PR. Which personally motivates me to rewrite this whole thing to using classic if stmts without switch

[EgorBo]

I'm not sure I follow, for me it sounds like it repeats work that should be done elsewhere. It's already done in fgMorphSmpOp for all comparisons, we probably might want to have a some shared way to do that for all commutative opers

[BoyBaykiller]

I though you are talking about this right down below from where I stole it which only does it for some:

runtime/src/coreclr/jit/morph.cpp

Lines 7792 to 7809 in e19c810

	caseGT_LT:
	caseGT_LE:
	caseGT_GE:
	caseGT_GT:
	// Change "CNS relop op2" to "op2 relop* CNS"
	if (op1->IsIntegralConst() && tree->OperIsCompare() && gtCanSwapOrder(op1, op2))
	{
	std::swap(tree->AsOp()->gtOp1, tree->AsOp()->gtOp2);
	tree->gtOper = GenTree::SwapRelop(tree->OperGet());
	oper = tree->OperGet();
	op1 = tree->gtGetOp1();
	op2 = tree->gtGetOp2();
	}
	if (op1->OperIs(GT_CAST) || op2->OperIs(GT_CAST))
	{

Not sure what place you are refering to

[BoyBaykiller]

we probably might want to have a some shared way to do that for all commutative opers

That'd be nice

[tannergooding]

The check just below it is also notably too lenient, it is valid for floating-point as well and we handle that for SIMD nodes.

You can likely just extract it out to a small helper function if you don't want to duplicate the logic between EQ/NE and the other compares, although notably EQ/NE are simpler since they are commutative.

[tannergooding]

If you happen to know where this code is that should already be handling comparisons, please let me know. So I can try to improve it. I will try to find it now.

Looks like it might indeed be missing, outside of costing.

[tannergooding]

I'd recommend doing that one in it's own PR (it's much more broadly impacting and not isolated to the ispow2 work) and ideally fixing it to not only do it for integrals, we can do it for all types just fine and just check SwapRelop

[BoyBaykiller]

and ideally fixing it to not only do it for integrals, we can do it for all types

are there optimizations on relops with floats? if yes where

[tannergooding]

In some cases, yes, and we do folding and other transforms based on them. There are more for vectors than there are for scalars right now, but ideally they are mirrored. In general we want to push constants right for other reasons as well, such as CSE, containment, VN, etc.

[tannergooding]

nit: This is not correct as it checks for GT_CNS_INT where you must instead check for TypeIs(TYP_INT)

[BoyBaykiller]

if (IsIntegralConst())
{
if (TypeIs(TYP_INT))
{
returnisPow2((uint32_t)AsIntCon()->IconValue());
}
returnisPow2((uint64_t)AsLngCon()->LngValue());
}

Input:

boolTest1(longA,longB){return8==(A&8);}

Gives me: Assertion failed 'OperIs(GT_CNS_LNG)'. I guess I should use AsIntConCommon()->IntegralValue().

[tannergooding]

What platform are you getting the assertion on? and for which line?

There shouldn't be any GT_CNS_LNG being produced on 64-bit, they all get routed to GT_CNS_INT. Then you shouldn't be producing a GT_CNS_LNG for TYP_INT on 32-bit either, correspondingly.

inlinessize_tGenTreeIntConCommon::IconValue() const
{
assert(OperIs(GT_CNS_INT)); // We should never see a GT_CNS_LNG for a 64-bit target!returnAsIntCon()->gtIconVal;
}

and

inlineINT64GenTreeIntConCommon::LngValue() const
{
#ifndef TARGET_64BIT
assert(OperIs(GT_CNS_LNG));
returnAsLngCon()->gtLconVal;
#elsereturnIconValue();
#endif
}

and

GenTreeLngCon(INT64 val)
: GenTreeIntConCommon(GT_CNS_NATIVELONG, TYP_LONG)
{
SetLngValue(val);
}

and

GenTreeIntCon(var_types type, ssize_t value DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(nullptr)
{
}
GenTreeIntCon(var_types type, ssize_t value, FieldSeq* fields DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(fields)
{
}

[BoyBaykiller]

I will be able to properly answer you tomorrow but you should be able to reproduce on win-x64 main.

[BoyBaykiller]

Split into
Canon (cns to right): #127661 (more general, better version)
Canon (bit-test): #128533
Pow2: #127615
Unify: #127883

[BoyBaykiller]

Unifying the compare opts to naturally handle canonicalization of constant to the right for GT_EQ, GT_NE. This now also calls fgOptimizeCmpWithCasts for GT_EQ, GT_NE which was missing too.

I also added 2 canonicalizations:

1. '(A & pow2) == pow2' -> '(A & pow2) != 0'
2. '(A & pow2) != pow2' -> '(A & pow2) == 0'

These will help optimizeBools and consequentenly #126852 and probably other stuff. I'd also like this for #125899 if I add e.g SELECT(x & 16 == 16, y | 16, y) -> y | (x & 16)

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[EgorBo]

I see that there is already a logic that moves constants to the right, why do we repeat it here?

[BoyBaykiller]

I totally agreee. But the fact that its written in a switch makes it hard/ugly to freely share code. I noticed the exact thing when I was adding fgOptimizeDistributiveArithemtic from my other PR. Which personally motivates me to rewrite this whole thing to using classic if stmts without switch

[EgorBo]

I'm not sure I follow, for me it sounds like it repeats work that should be done elsewhere. It's already done in fgMorphSmpOp for all comparisons, we probably might want to have a some shared way to do that for all commutative opers

[BoyBaykiller]

I though you are talking about this right down below from where I stole it which only does it for some:

runtime/src/coreclr/jit/morph.cpp

Lines 7792 to 7809 in e19c810

	caseGT_LT:
	caseGT_LE:
	caseGT_GE:
	caseGT_GT:
	// Change "CNS relop op2" to "op2 relop* CNS"
	if (op1->IsIntegralConst() && tree->OperIsCompare() && gtCanSwapOrder(op1, op2))
	{
	std::swap(tree->AsOp()->gtOp1, tree->AsOp()->gtOp2);
	tree->gtOper = GenTree::SwapRelop(tree->OperGet());
	oper = tree->OperGet();
	op1 = tree->gtGetOp1();
	op2 = tree->gtGetOp2();
	}
	if (op1->OperIs(GT_CAST) || op2->OperIs(GT_CAST))
	{

Not sure what place you are refering to

[BoyBaykiller]

we probably might want to have a some shared way to do that for all commutative opers

That'd be nice

[tannergooding]

The check just below it is also notably too lenient, it is valid for floating-point as well and we handle that for SIMD nodes.

You can likely just extract it out to a small helper function if you don't want to duplicate the logic between EQ/NE and the other compares, although notably EQ/NE are simpler since they are commutative.

[tannergooding]

If you happen to know where this code is that should already be handling comparisons, please let me know. So I can try to improve it. I will try to find it now.

Looks like it might indeed be missing, outside of costing.

[tannergooding]

I'd recommend doing that one in it's own PR (it's much more broadly impacting and not isolated to the ispow2 work) and ideally fixing it to not only do it for integrals, we can do it for all types just fine and just check SwapRelop

[BoyBaykiller]

and ideally fixing it to not only do it for integrals, we can do it for all types

are there optimizations on relops with floats? if yes where

[tannergooding]

In some cases, yes, and we do folding and other transforms based on them. There are more for vectors than there are for scalars right now, but ideally they are mirrored. In general we want to push constants right for other reasons as well, such as CSE, containment, VN, etc.

[tannergooding]

nit: This is not correct as it checks for GT_CNS_INT where you must instead check for TypeIs(TYP_INT)

[BoyBaykiller]

if (IsIntegralConst())
{
if (TypeIs(TYP_INT))
{
returnisPow2((uint32_t)AsIntCon()->IconValue());
}
returnisPow2((uint64_t)AsLngCon()->LngValue());
}

Input:

boolTest1(longA,longB){return8==(A&8);}

Gives me: Assertion failed 'OperIs(GT_CNS_LNG)'. I guess I should use AsIntConCommon()->IntegralValue().

[tannergooding]

What platform are you getting the assertion on? and for which line?

There shouldn't be any GT_CNS_LNG being produced on 64-bit, they all get routed to GT_CNS_INT. Then you shouldn't be producing a GT_CNS_LNG for TYP_INT on 32-bit either, correspondingly.

inlinessize_tGenTreeIntConCommon::IconValue() const
{
assert(OperIs(GT_CNS_INT)); // We should never see a GT_CNS_LNG for a 64-bit target!returnAsIntCon()->gtIconVal;
}

and

inlineINT64GenTreeIntConCommon::LngValue() const
{
#ifndef TARGET_64BIT
assert(OperIs(GT_CNS_LNG));
returnAsLngCon()->gtLconVal;
#elsereturnIconValue();
#endif
}

and

GenTreeLngCon(INT64 val)
: GenTreeIntConCommon(GT_CNS_NATIVELONG, TYP_LONG)
{
SetLngValue(val);
}

and

GenTreeIntCon(var_types type, ssize_t value DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(nullptr)
{
}
GenTreeIntCon(var_types type, ssize_t value, FieldSeq* fields DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(fields)
{
}

[BoyBaykiller]

I will be able to properly answer you tomorrow but you should be able to reproduce on win-x64 main.

[BoyBaykiller]

Split into
Canon (cns to right): #127661 (more general, better version)
Canon (bit-test): #128533
Pow2: #127615
Unify: #127883

[BoyBaykiller]

Unifying the compare opts to naturally handle canonicalization of constant to the right for GT_EQ, GT_NE. This now also calls fgOptimizeCmpWithCasts for GT_EQ, GT_NE which was missing too.

I also added 2 canonicalizations:

1. '(A & pow2) == pow2' -> '(A & pow2) != 0'
2. '(A & pow2) != pow2' -> '(A & pow2) == 0'

These will help optimizeBools and consequentenly #126852 and probably other stuff. I'd also like this for #125899 if I add e.g SELECT(x & 16 == 16, y | 16, y) -> y | (x & 16)

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[EgorBo]

I see that there is already a logic that moves constants to the right, why do we repeat it here?

[BoyBaykiller]

I totally agreee. But the fact that its written in a switch makes it hard/ugly to freely share code. I noticed the exact thing when I was adding fgOptimizeDistributiveArithemtic from my other PR. Which personally motivates me to rewrite this whole thing to using classic if stmts without switch

[EgorBo]

I'm not sure I follow, for me it sounds like it repeats work that should be done elsewhere. It's already done in fgMorphSmpOp for all comparisons, we probably might want to have a some shared way to do that for all commutative opers

[BoyBaykiller]

I though you are talking about this right down below from where I stole it which only does it for some:

runtime/src/coreclr/jit/morph.cpp

Lines 7792 to 7809 in e19c810

	caseGT_LT:
	caseGT_LE:
	caseGT_GE:
	caseGT_GT:
	// Change "CNS relop op2" to "op2 relop* CNS"
	if (op1->IsIntegralConst() && tree->OperIsCompare() && gtCanSwapOrder(op1, op2))
	{
	std::swap(tree->AsOp()->gtOp1, tree->AsOp()->gtOp2);
	tree->gtOper = GenTree::SwapRelop(tree->OperGet());
	oper = tree->OperGet();
	op1 = tree->gtGetOp1();
	op2 = tree->gtGetOp2();
	}
	if (op1->OperIs(GT_CAST) || op2->OperIs(GT_CAST))
	{

Not sure what place you are refering to

[BoyBaykiller]

we probably might want to have a some shared way to do that for all commutative opers

That'd be nice

[tannergooding]

The check just below it is also notably too lenient, it is valid for floating-point as well and we handle that for SIMD nodes.

You can likely just extract it out to a small helper function if you don't want to duplicate the logic between EQ/NE and the other compares, although notably EQ/NE are simpler since they are commutative.

[tannergooding]

If you happen to know where this code is that should already be handling comparisons, please let me know. So I can try to improve it. I will try to find it now.

Looks like it might indeed be missing, outside of costing.

[tannergooding]

I'd recommend doing that one in it's own PR (it's much more broadly impacting and not isolated to the ispow2 work) and ideally fixing it to not only do it for integrals, we can do it for all types just fine and just check SwapRelop

[BoyBaykiller]

and ideally fixing it to not only do it for integrals, we can do it for all types

are there optimizations on relops with floats? if yes where

[tannergooding]

In some cases, yes, and we do folding and other transforms based on them. There are more for vectors than there are for scalars right now, but ideally they are mirrored. In general we want to push constants right for other reasons as well, such as CSE, containment, VN, etc.

[tannergooding]

nit: This is not correct as it checks for GT_CNS_INT where you must instead check for TypeIs(TYP_INT)

[BoyBaykiller]

if (IsIntegralConst())
{
if (TypeIs(TYP_INT))
{
returnisPow2((uint32_t)AsIntCon()->IconValue());
}
returnisPow2((uint64_t)AsLngCon()->LngValue());
}

Input:

boolTest1(longA,longB){return8==(A&8);}

Gives me: Assertion failed 'OperIs(GT_CNS_LNG)'. I guess I should use AsIntConCommon()->IntegralValue().

[tannergooding]

What platform are you getting the assertion on? and for which line?

There shouldn't be any GT_CNS_LNG being produced on 64-bit, they all get routed to GT_CNS_INT. Then you shouldn't be producing a GT_CNS_LNG for TYP_INT on 32-bit either, correspondingly.

inlinessize_tGenTreeIntConCommon::IconValue() const
{
assert(OperIs(GT_CNS_INT)); // We should never see a GT_CNS_LNG for a 64-bit target!returnAsIntCon()->gtIconVal;
}

and

inlineINT64GenTreeIntConCommon::LngValue() const
{
#ifndef TARGET_64BIT
assert(OperIs(GT_CNS_LNG));
returnAsLngCon()->gtLconVal;
#elsereturnIconValue();
#endif
}

and

GenTreeLngCon(INT64 val)
: GenTreeIntConCommon(GT_CNS_NATIVELONG, TYP_LONG)
{
SetLngValue(val);
}

and

GenTreeIntCon(var_types type, ssize_t value DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(nullptr)
{
}
GenTreeIntCon(var_types type, ssize_t value, FieldSeq* fields DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(fields)
{
}

[BoyBaykiller]

I will be able to properly answer you tomorrow but you should be able to reproduce on win-x64 main.

[BoyBaykiller]

Split into
Canon (cns to right): #127661 (more general, better version)
Canon (bit-test): #128533
Pow2: #127615
Unify: #127883

[BoyBaykiller]

Unifying the compare opts to naturally handle canonicalization of constant to the right for GT_EQ, GT_NE. This now also calls fgOptimizeCmpWithCasts for GT_EQ, GT_NE which was missing too.

I also added 2 canonicalizations:

1. '(A & pow2) == pow2' -> '(A & pow2) != 0'
2. '(A & pow2) != pow2' -> '(A & pow2) == 0'

These will help optimizeBools and consequentenly #126852 and probably other stuff. I'd also like this for #125899 if I add e.g SELECT(x & 16 == 16, y | 16, y) -> y | (x & 16)

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[EgorBo]

I see that there is already a logic that moves constants to the right, why do we repeat it here?

[BoyBaykiller]

I totally agreee. But the fact that its written in a switch makes it hard/ugly to freely share code. I noticed the exact thing when I was adding fgOptimizeDistributiveArithemtic from my other PR. Which personally motivates me to rewrite this whole thing to using classic if stmts without switch

[EgorBo]

I'm not sure I follow, for me it sounds like it repeats work that should be done elsewhere. It's already done in fgMorphSmpOp for all comparisons, we probably might want to have a some shared way to do that for all commutative opers

[BoyBaykiller]

I though you are talking about this right down below from where I stole it which only does it for some:

runtime/src/coreclr/jit/morph.cpp

Lines 7792 to 7809 in e19c810

	caseGT_LT:
	caseGT_LE:
	caseGT_GE:
	caseGT_GT:
	// Change "CNS relop op2" to "op2 relop* CNS"
	if (op1->IsIntegralConst() && tree->OperIsCompare() && gtCanSwapOrder(op1, op2))
	{
	std::swap(tree->AsOp()->gtOp1, tree->AsOp()->gtOp2);
	tree->gtOper = GenTree::SwapRelop(tree->OperGet());
	oper = tree->OperGet();
	op1 = tree->gtGetOp1();
	op2 = tree->gtGetOp2();
	}
	if (op1->OperIs(GT_CAST) || op2->OperIs(GT_CAST))
	{

Not sure what place you are refering to

[BoyBaykiller]

we probably might want to have a some shared way to do that for all commutative opers

That'd be nice

[tannergooding]

The check just below it is also notably too lenient, it is valid for floating-point as well and we handle that for SIMD nodes.

You can likely just extract it out to a small helper function if you don't want to duplicate the logic between EQ/NE and the other compares, although notably EQ/NE are simpler since they are commutative.

[tannergooding]

If you happen to know where this code is that should already be handling comparisons, please let me know. So I can try to improve it. I will try to find it now.

Looks like it might indeed be missing, outside of costing.

[tannergooding]

I'd recommend doing that one in it's own PR (it's much more broadly impacting and not isolated to the ispow2 work) and ideally fixing it to not only do it for integrals, we can do it for all types just fine and just check SwapRelop

[BoyBaykiller]

and ideally fixing it to not only do it for integrals, we can do it for all types

are there optimizations on relops with floats? if yes where

[tannergooding]

In some cases, yes, and we do folding and other transforms based on them. There are more for vectors than there are for scalars right now, but ideally they are mirrored. In general we want to push constants right for other reasons as well, such as CSE, containment, VN, etc.

[tannergooding]

nit: This is not correct as it checks for GT_CNS_INT where you must instead check for TypeIs(TYP_INT)

[BoyBaykiller]

if (IsIntegralConst())
{
if (TypeIs(TYP_INT))
{
returnisPow2((uint32_t)AsIntCon()->IconValue());
}
returnisPow2((uint64_t)AsLngCon()->LngValue());
}

Input:

boolTest1(longA,longB){return8==(A&8);}

Gives me: Assertion failed 'OperIs(GT_CNS_LNG)'. I guess I should use AsIntConCommon()->IntegralValue().

[tannergooding]

What platform are you getting the assertion on? and for which line?

There shouldn't be any GT_CNS_LNG being produced on 64-bit, they all get routed to GT_CNS_INT. Then you shouldn't be producing a GT_CNS_LNG for TYP_INT on 32-bit either, correspondingly.

inlinessize_tGenTreeIntConCommon::IconValue() const
{
assert(OperIs(GT_CNS_INT)); // We should never see a GT_CNS_LNG for a 64-bit target!returnAsIntCon()->gtIconVal;
}

and

inlineINT64GenTreeIntConCommon::LngValue() const
{
#ifndef TARGET_64BIT
assert(OperIs(GT_CNS_LNG));
returnAsLngCon()->gtLconVal;
#elsereturnIconValue();
#endif
}

and

GenTreeLngCon(INT64 val)
: GenTreeIntConCommon(GT_CNS_NATIVELONG, TYP_LONG)
{
SetLngValue(val);
}

and

GenTreeIntCon(var_types type, ssize_t value DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(nullptr)
{
}
GenTreeIntCon(var_types type, ssize_t value, FieldSeq* fields DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(fields)
{
}

[BoyBaykiller]

I will be able to properly answer you tomorrow but you should be able to reproduce on win-x64 main.

[BoyBaykiller]

Split into
Canon (cns to right): #127661 (more general, better version)
Canon (bit-test): #128533
Pow2: #127615
Unify: #127883

[BoyBaykiller]

Unifying the compare opts to naturally handle canonicalization of constant to the right for GT_EQ, GT_NE. This now also calls fgOptimizeCmpWithCasts for GT_EQ, GT_NE which was missing too.

I also added 2 canonicalizations:

1. '(A & pow2) == pow2' -> '(A & pow2) != 0'
2. '(A & pow2) != pow2' -> '(A & pow2) == 0'

These will help optimizeBools and consequentenly #126852 and probably other stuff. I'd also like this for #125899 if I add e.g SELECT(x & 16 == 16, y | 16, y) -> y | (x & 16)

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[EgorBo]

I see that there is already a logic that moves constants to the right, why do we repeat it here?

[BoyBaykiller]

I totally agreee. But the fact that its written in a switch makes it hard/ugly to freely share code. I noticed the exact thing when I was adding fgOptimizeDistributiveArithemtic from my other PR. Which personally motivates me to rewrite this whole thing to using classic if stmts without switch

[EgorBo]

I'm not sure I follow, for me it sounds like it repeats work that should be done elsewhere. It's already done in fgMorphSmpOp for all comparisons, we probably might want to have a some shared way to do that for all commutative opers

[BoyBaykiller]

I though you are talking about this right down below from where I stole it which only does it for some:

runtime/src/coreclr/jit/morph.cpp

Lines 7792 to 7809 in e19c810

	caseGT_LT:
	caseGT_LE:
	caseGT_GE:
	caseGT_GT:
	// Change "CNS relop op2" to "op2 relop* CNS"
	if (op1->IsIntegralConst() && tree->OperIsCompare() && gtCanSwapOrder(op1, op2))
	{
	std::swap(tree->AsOp()->gtOp1, tree->AsOp()->gtOp2);
	tree->gtOper = GenTree::SwapRelop(tree->OperGet());
	oper = tree->OperGet();
	op1 = tree->gtGetOp1();
	op2 = tree->gtGetOp2();
	}
	if (op1->OperIs(GT_CAST) || op2->OperIs(GT_CAST))
	{

Not sure what place you are refering to

[BoyBaykiller]

we probably might want to have a some shared way to do that for all commutative opers

That'd be nice

[tannergooding]

The check just below it is also notably too lenient, it is valid for floating-point as well and we handle that for SIMD nodes.

You can likely just extract it out to a small helper function if you don't want to duplicate the logic between EQ/NE and the other compares, although notably EQ/NE are simpler since they are commutative.

[tannergooding]

If you happen to know where this code is that should already be handling comparisons, please let me know. So I can try to improve it. I will try to find it now.

Looks like it might indeed be missing, outside of costing.

[tannergooding]

I'd recommend doing that one in it's own PR (it's much more broadly impacting and not isolated to the ispow2 work) and ideally fixing it to not only do it for integrals, we can do it for all types just fine and just check SwapRelop

[BoyBaykiller]

and ideally fixing it to not only do it for integrals, we can do it for all types

are there optimizations on relops with floats? if yes where

[tannergooding]

In some cases, yes, and we do folding and other transforms based on them. There are more for vectors than there are for scalars right now, but ideally they are mirrored. In general we want to push constants right for other reasons as well, such as CSE, containment, VN, etc.

[tannergooding]

nit: This is not correct as it checks for GT_CNS_INT where you must instead check for TypeIs(TYP_INT)

[BoyBaykiller]

if (IsIntegralConst())
{
if (TypeIs(TYP_INT))
{
returnisPow2((uint32_t)AsIntCon()->IconValue());
}
returnisPow2((uint64_t)AsLngCon()->LngValue());
}

Input:

boolTest1(longA,longB){return8==(A&8);}

Gives me: Assertion failed 'OperIs(GT_CNS_LNG)'. I guess I should use AsIntConCommon()->IntegralValue().

[tannergooding]

What platform are you getting the assertion on? and for which line?

There shouldn't be any GT_CNS_LNG being produced on 64-bit, they all get routed to GT_CNS_INT. Then you shouldn't be producing a GT_CNS_LNG for TYP_INT on 32-bit either, correspondingly.

inlinessize_tGenTreeIntConCommon::IconValue() const
{
assert(OperIs(GT_CNS_INT)); // We should never see a GT_CNS_LNG for a 64-bit target!returnAsIntCon()->gtIconVal;
}

and

inlineINT64GenTreeIntConCommon::LngValue() const
{
#ifndef TARGET_64BIT
assert(OperIs(GT_CNS_LNG));
returnAsLngCon()->gtLconVal;
#elsereturnIconValue();
#endif
}

and

GenTreeLngCon(INT64 val)
: GenTreeIntConCommon(GT_CNS_NATIVELONG, TYP_LONG)
{
SetLngValue(val);
}

and

GenTreeIntCon(var_types type, ssize_t value DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(nullptr)
{
}
GenTreeIntCon(var_types type, ssize_t value, FieldSeq* fields DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(fields)
{
}

[BoyBaykiller]

I will be able to properly answer you tomorrow but you should be able to reproduce on win-x64 main.

[BoyBaykiller]

Split into
Canon (cns to right): #127661 (more general, better version)
Canon (bit-test): #128533
Pow2: #127615
Unify: #127883

[BoyBaykiller]

Unifying the compare opts to naturally handle canonicalization of constant to the right for GT_EQ, GT_NE. This now also calls fgOptimizeCmpWithCasts for GT_EQ, GT_NE which was missing too.

I also added 2 canonicalizations:

1. '(A & pow2) == pow2' -> '(A & pow2) != 0'
2. '(A & pow2) != pow2' -> '(A & pow2) == 0'

These will help optimizeBools and consequentenly #126852 and probably other stuff. I'd also like this for #125899 if I add e.g SELECT(x & 16 == 16, y | 16, y) -> y | (x & 16)

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[EgorBo]

I see that there is already a logic that moves constants to the right, why do we repeat it here?

[BoyBaykiller]

I totally agreee. But the fact that its written in a switch makes it hard/ugly to freely share code. I noticed the exact thing when I was adding fgOptimizeDistributiveArithemtic from my other PR. Which personally motivates me to rewrite this whole thing to using classic if stmts without switch

[EgorBo]

I'm not sure I follow, for me it sounds like it repeats work that should be done elsewhere. It's already done in fgMorphSmpOp for all comparisons, we probably might want to have a some shared way to do that for all commutative opers

[BoyBaykiller]

I though you are talking about this right down below from where I stole it which only does it for some:

runtime/src/coreclr/jit/morph.cpp

Lines 7792 to 7809 in e19c810

	caseGT_LT:
	caseGT_LE:
	caseGT_GE:
	caseGT_GT:
	// Change "CNS relop op2" to "op2 relop* CNS"
	if (op1->IsIntegralConst() && tree->OperIsCompare() && gtCanSwapOrder(op1, op2))
	{
	std::swap(tree->AsOp()->gtOp1, tree->AsOp()->gtOp2);
	tree->gtOper = GenTree::SwapRelop(tree->OperGet());
	oper = tree->OperGet();
	op1 = tree->gtGetOp1();
	op2 = tree->gtGetOp2();
	}
	if (op1->OperIs(GT_CAST) || op2->OperIs(GT_CAST))
	{

Not sure what place you are refering to

[BoyBaykiller]

we probably might want to have a some shared way to do that for all commutative opers

That'd be nice

[tannergooding]

The check just below it is also notably too lenient, it is valid for floating-point as well and we handle that for SIMD nodes.

You can likely just extract it out to a small helper function if you don't want to duplicate the logic between EQ/NE and the other compares, although notably EQ/NE are simpler since they are commutative.

[tannergooding]

If you happen to know where this code is that should already be handling comparisons, please let me know. So I can try to improve it. I will try to find it now.

Looks like it might indeed be missing, outside of costing.

[tannergooding]

I'd recommend doing that one in it's own PR (it's much more broadly impacting and not isolated to the ispow2 work) and ideally fixing it to not only do it for integrals, we can do it for all types just fine and just check SwapRelop

[BoyBaykiller]

and ideally fixing it to not only do it for integrals, we can do it for all types

are there optimizations on relops with floats? if yes where

[tannergooding]

In some cases, yes, and we do folding and other transforms based on them. There are more for vectors than there are for scalars right now, but ideally they are mirrored. In general we want to push constants right for other reasons as well, such as CSE, containment, VN, etc.

[tannergooding]

nit: This is not correct as it checks for GT_CNS_INT where you must instead check for TypeIs(TYP_INT)

[BoyBaykiller]

if (IsIntegralConst())
{
if (TypeIs(TYP_INT))
{
returnisPow2((uint32_t)AsIntCon()->IconValue());
}
returnisPow2((uint64_t)AsLngCon()->LngValue());
}

Input:

boolTest1(longA,longB){return8==(A&8);}

Gives me: Assertion failed 'OperIs(GT_CNS_LNG)'. I guess I should use AsIntConCommon()->IntegralValue().

[tannergooding]

What platform are you getting the assertion on? and for which line?

There shouldn't be any GT_CNS_LNG being produced on 64-bit, they all get routed to GT_CNS_INT. Then you shouldn't be producing a GT_CNS_LNG for TYP_INT on 32-bit either, correspondingly.

inlinessize_tGenTreeIntConCommon::IconValue() const
{
assert(OperIs(GT_CNS_INT)); // We should never see a GT_CNS_LNG for a 64-bit target!returnAsIntCon()->gtIconVal;
}

and

inlineINT64GenTreeIntConCommon::LngValue() const
{
#ifndef TARGET_64BIT
assert(OperIs(GT_CNS_LNG));
returnAsLngCon()->gtLconVal;
#elsereturnIconValue();
#endif
}

and

GenTreeLngCon(INT64 val)
: GenTreeIntConCommon(GT_CNS_NATIVELONG, TYP_LONG)
{
SetLngValue(val);
}

and

GenTreeIntCon(var_types type, ssize_t value DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(nullptr)
{
}
GenTreeIntCon(var_types type, ssize_t value, FieldSeq* fields DEBUGARG(bool largeNode = false))
: GenTreeIntConCommon(GT_CNS_INT, type DEBUGARG(largeNode))
, gtIconVal(value)
, gtCompileTimeHandle(0)
, gtFieldSeq(fields)
{
}

[BoyBaykiller]

I will be able to properly answer you tomorrow but you should be able to reproduce on win-x64 main.

[BoyBaykiller]

Split into
Canon (cns to right): #127661 (more general, better version)
Canon (bit-test): #128533
Pow2: #127615
Unify: #127883