JIT: Fix GenTree::IsPow2 functions

[BoyBaykiller]

IsIntegralConstUnsignedPow2 was passing in sign extended value to isPow2. So it wasn't actually unsigned. For example: 1 << 31 would not be recognized as pow2.

Add a UnsignedIntegralValue that gives the zero-extended literal. Use it in IsIntegralConstUnsignedPow2 to fix it's impl.
Also use it in fgMorphUModToAndSub and a place in lower.

[dotnet-policy-service]

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

[BoyBaykiller]

@tannergooding

[BoyBaykiller]

There was an assert failure in arm emitter helper func which I still had to address

[tannergooding]

CC. @dotnet/jit-contrib, @EgorBo, @kg for secondary review

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

from a quick look, another instance of that in fgMorphUModToAndSub

[BoyBaykiller]

The problem is I am not even sure what IsIntegralConstUnsignedPow2() was meant to do in the first place which is why I originally just called popcnt(x) == 1 in my other PR and added a GenTree::UnsignedIntegralValue() thing that gives the zero-extended value.

It seems like existing callers of this function want to know if there is only a single bit set. For example the fgMorphUModToAndSub opt can only be performed in that case - so that's fine and I don't see any issue here. The diffs show that it enables the previously missed 1<<31 case.

Regarding the RISCV code in lower. I am not sure it is doing what the author intended.
We have:

// If 'test' is a single bit test...//if (bitOp->IsIntegralConstUnsignedPow2())
{
INT64 bit = bitOp->AsIntConCommon()->IntegralValue();
int log2 = BitOperations::Log2((UINT64)bit);
bitOp->AsIntConCommon()->SetIntegralValue(log2);
returntrue;
}

The Log2 here must be here to find the bit position. But the code is using bitOp->AsIntConCommon()->IntegralValue() which as we know sign extends values. So this code now gets entered for 1<<31 but then fails with it's assumptions at log2 because bitOp->AsIntConCommon()->IntegralValue() gives us a value with the upper 32 bits set.

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

[EgorBo]

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

I think we should just use truncated value in that RISC-V path

[BoyBaykiller]

I think we should just use truncated value in that RISC-V path

What's the corresponding API for getting such "truncated value"?

[EgorBo]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[BoyBaykiller]

@EgorBo should be good now

[EgorBo]

/ba-g deadletter

[EgorBo]

can't merge it while build analysis is red and /ba-g is broken 😐

[BoyBaykiller]

IsIntegralConstUnsignedPow2 was passing in sign extended value to isPow2. So it wasn't actually unsigned. For example: 1 << 31 would not be recognized as pow2.

Add a UnsignedIntegralValue that gives the zero-extended literal. Use it in IsIntegralConstUnsignedPow2 to fix it's impl.
Also use it in fgMorphUModToAndSub and a place in lower.

[dotnet-policy-service]

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

[BoyBaykiller]

@tannergooding

[BoyBaykiller]

There was an assert failure in arm emitter helper func which I still had to address

[tannergooding]

CC. @dotnet/jit-contrib, @EgorBo, @kg for secondary review

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

from a quick look, another instance of that in fgMorphUModToAndSub

[BoyBaykiller]

The problem is I am not even sure what IsIntegralConstUnsignedPow2() was meant to do in the first place which is why I originally just called popcnt(x) == 1 in my other PR and added a GenTree::UnsignedIntegralValue() thing that gives the zero-extended value.

It seems like existing callers of this function want to know if there is only a single bit set. For example the fgMorphUModToAndSub opt can only be performed in that case - so that's fine and I don't see any issue here. The diffs show that it enables the previously missed 1<<31 case.

Regarding the RISCV code in lower. I am not sure it is doing what the author intended.
We have:

// If 'test' is a single bit test...//if (bitOp->IsIntegralConstUnsignedPow2())
{
INT64 bit = bitOp->AsIntConCommon()->IntegralValue();
int log2 = BitOperations::Log2((UINT64)bit);
bitOp->AsIntConCommon()->SetIntegralValue(log2);
returntrue;
}

The Log2 here must be here to find the bit position. But the code is using bitOp->AsIntConCommon()->IntegralValue() which as we know sign extends values. So this code now gets entered for 1<<31 but then fails with it's assumptions at log2 because bitOp->AsIntConCommon()->IntegralValue() gives us a value with the upper 32 bits set.

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

[EgorBo]

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

I think we should just use truncated value in that RISC-V path

[BoyBaykiller]

I think we should just use truncated value in that RISC-V path

What's the corresponding API for getting such "truncated value"?

[EgorBo]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[BoyBaykiller]

@EgorBo should be good now

[EgorBo]

/ba-g deadletter

[EgorBo]

can't merge it while build analysis is red and /ba-g is broken 😐

[BoyBaykiller]

IsIntegralConstUnsignedPow2 was passing in sign extended value to isPow2. So it wasn't actually unsigned. For example: 1 << 31 would not be recognized as pow2.

Add a UnsignedIntegralValue that gives the zero-extended literal. Use it in IsIntegralConstUnsignedPow2 to fix it's impl.
Also use it in fgMorphUModToAndSub and a place in lower.

[dotnet-policy-service]

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

[BoyBaykiller]

@tannergooding

[BoyBaykiller]

There was an assert failure in arm emitter helper func which I still had to address

[tannergooding]

CC. @dotnet/jit-contrib, @EgorBo, @kg for secondary review

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

from a quick look, another instance of that in fgMorphUModToAndSub

[BoyBaykiller]

The problem is I am not even sure what IsIntegralConstUnsignedPow2() was meant to do in the first place which is why I originally just called popcnt(x) == 1 in my other PR and added a GenTree::UnsignedIntegralValue() thing that gives the zero-extended value.

It seems like existing callers of this function want to know if there is only a single bit set. For example the fgMorphUModToAndSub opt can only be performed in that case - so that's fine and I don't see any issue here. The diffs show that it enables the previously missed 1<<31 case.

Regarding the RISCV code in lower. I am not sure it is doing what the author intended.
We have:

// If 'test' is a single bit test...//if (bitOp->IsIntegralConstUnsignedPow2())
{
INT64 bit = bitOp->AsIntConCommon()->IntegralValue();
int log2 = BitOperations::Log2((UINT64)bit);
bitOp->AsIntConCommon()->SetIntegralValue(log2);
returntrue;
}

The Log2 here must be here to find the bit position. But the code is using bitOp->AsIntConCommon()->IntegralValue() which as we know sign extends values. So this code now gets entered for 1<<31 but then fails with it's assumptions at log2 because bitOp->AsIntConCommon()->IntegralValue() gives us a value with the upper 32 bits set.

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

[EgorBo]

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

I think we should just use truncated value in that RISC-V path

[BoyBaykiller]

I think we should just use truncated value in that RISC-V path

What's the corresponding API for getting such "truncated value"?

[EgorBo]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[BoyBaykiller]

@EgorBo should be good now

[EgorBo]

/ba-g deadletter

[EgorBo]

can't merge it while build analysis is red and /ba-g is broken 😐

[BoyBaykiller]

IsIntegralConstUnsignedPow2 was passing in sign extended value to isPow2. So it wasn't actually unsigned. For example: 1 << 31 would not be recognized as pow2.

Add a UnsignedIntegralValue that gives the zero-extended literal. Use it in IsIntegralConstUnsignedPow2 to fix it's impl.
Also use it in fgMorphUModToAndSub and a place in lower.

[dotnet-policy-service]

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

[BoyBaykiller]

@tannergooding

[BoyBaykiller]

There was an assert failure in arm emitter helper func which I still had to address

[tannergooding]

CC. @dotnet/jit-contrib, @EgorBo, @kg for secondary review

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

from a quick look, another instance of that in fgMorphUModToAndSub

[BoyBaykiller]

The problem is I am not even sure what IsIntegralConstUnsignedPow2() was meant to do in the first place which is why I originally just called popcnt(x) == 1 in my other PR and added a GenTree::UnsignedIntegralValue() thing that gives the zero-extended value.

It seems like existing callers of this function want to know if there is only a single bit set. For example the fgMorphUModToAndSub opt can only be performed in that case - so that's fine and I don't see any issue here. The diffs show that it enables the previously missed 1<<31 case.

Regarding the RISCV code in lower. I am not sure it is doing what the author intended.
We have:

// If 'test' is a single bit test...//if (bitOp->IsIntegralConstUnsignedPow2())
{
INT64 bit = bitOp->AsIntConCommon()->IntegralValue();
int log2 = BitOperations::Log2((UINT64)bit);
bitOp->AsIntConCommon()->SetIntegralValue(log2);
returntrue;
}

The Log2 here must be here to find the bit position. But the code is using bitOp->AsIntConCommon()->IntegralValue() which as we know sign extends values. So this code now gets entered for 1<<31 but then fails with it's assumptions at log2 because bitOp->AsIntConCommon()->IntegralValue() gives us a value with the upper 32 bits set.

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

[EgorBo]

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

I think we should just use truncated value in that RISC-V path

[BoyBaykiller]

I think we should just use truncated value in that RISC-V path

What's the corresponding API for getting such "truncated value"?

[EgorBo]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[BoyBaykiller]

@EgorBo should be good now

[EgorBo]

/ba-g deadletter

[EgorBo]

can't merge it while build analysis is red and /ba-g is broken 😐

[BoyBaykiller]

IsIntegralConstUnsignedPow2 was passing in sign extended value to isPow2. So it wasn't actually unsigned. For example: 1 << 31 would not be recognized as pow2.

Add a UnsignedIntegralValue that gives the zero-extended literal. Use it in IsIntegralConstUnsignedPow2 to fix it's impl.
Also use it in fgMorphUModToAndSub and a place in lower.

[dotnet-policy-service]

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

[BoyBaykiller]

@tannergooding

[BoyBaykiller]

There was an assert failure in arm emitter helper func which I still had to address

[tannergooding]

CC. @dotnet/jit-contrib, @EgorBo, @kg for secondary review

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

from a quick look, another instance of that in fgMorphUModToAndSub

[BoyBaykiller]

The problem is I am not even sure what IsIntegralConstUnsignedPow2() was meant to do in the first place which is why I originally just called popcnt(x) == 1 in my other PR and added a GenTree::UnsignedIntegralValue() thing that gives the zero-extended value.

It seems like existing callers of this function want to know if there is only a single bit set. For example the fgMorphUModToAndSub opt can only be performed in that case - so that's fine and I don't see any issue here. The diffs show that it enables the previously missed 1<<31 case.

Regarding the RISCV code in lower. I am not sure it is doing what the author intended.
We have:

// If 'test' is a single bit test...//if (bitOp->IsIntegralConstUnsignedPow2())
{
INT64 bit = bitOp->AsIntConCommon()->IntegralValue();
int log2 = BitOperations::Log2((UINT64)bit);
bitOp->AsIntConCommon()->SetIntegralValue(log2);
returntrue;
}

The Log2 here must be here to find the bit position. But the code is using bitOp->AsIntConCommon()->IntegralValue() which as we know sign extends values. So this code now gets entered for 1<<31 but then fails with it's assumptions at log2 because bitOp->AsIntConCommon()->IntegralValue() gives us a value with the upper 32 bits set.

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

[EgorBo]

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

I think we should just use truncated value in that RISC-V path

[BoyBaykiller]

I think we should just use truncated value in that RISC-V path

What's the corresponding API for getting such "truncated value"?

[EgorBo]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[BoyBaykiller]

@EgorBo should be good now

[EgorBo]

/ba-g deadletter

[EgorBo]

can't merge it while build analysis is red and /ba-g is broken 😐

[BoyBaykiller]

IsIntegralConstUnsignedPow2 was passing in sign extended value to isPow2. So it wasn't actually unsigned. For example: 1 << 31 would not be recognized as pow2.

Add a UnsignedIntegralValue that gives the zero-extended literal. Use it in IsIntegralConstUnsignedPow2 to fix it's impl.
Also use it in fgMorphUModToAndSub and a place in lower.

[dotnet-policy-service]

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

[BoyBaykiller]

@tannergooding

[BoyBaykiller]

There was an assert failure in arm emitter helper func which I still had to address

[tannergooding]

CC. @dotnet/jit-contrib, @EgorBo, @kg for secondary review

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

from a quick look, another instance of that in fgMorphUModToAndSub

[BoyBaykiller]

The problem is I am not even sure what IsIntegralConstUnsignedPow2() was meant to do in the first place which is why I originally just called popcnt(x) == 1 in my other PR and added a GenTree::UnsignedIntegralValue() thing that gives the zero-extended value.

It seems like existing callers of this function want to know if there is only a single bit set. For example the fgMorphUModToAndSub opt can only be performed in that case - so that's fine and I don't see any issue here. The diffs show that it enables the previously missed 1<<31 case.

Regarding the RISCV code in lower. I am not sure it is doing what the author intended.
We have:

// If 'test' is a single bit test...//if (bitOp->IsIntegralConstUnsignedPow2())
{
INT64 bit = bitOp->AsIntConCommon()->IntegralValue();
int log2 = BitOperations::Log2((UINT64)bit);
bitOp->AsIntConCommon()->SetIntegralValue(log2);
returntrue;
}

The Log2 here must be here to find the bit position. But the code is using bitOp->AsIntConCommon()->IntegralValue() which as we know sign extends values. So this code now gets entered for 1<<31 but then fails with it's assumptions at log2 because bitOp->AsIntConCommon()->IntegralValue() gives us a value with the upper 32 bits set.

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

[EgorBo]

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

I think we should just use truncated value in that RISC-V path

[BoyBaykiller]

I think we should just use truncated value in that RISC-V path

What's the corresponding API for getting such "truncated value"?

[EgorBo]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[BoyBaykiller]

@EgorBo should be good now

[EgorBo]

/ba-g deadletter

[EgorBo]

can't merge it while build analysis is red and /ba-g is broken 😐

[BoyBaykiller]

IsIntegralConstUnsignedPow2 was passing in sign extended value to isPow2. So it wasn't actually unsigned. For example: 1 << 31 would not be recognized as pow2.

Add a UnsignedIntegralValue that gives the zero-extended literal. Use it in IsIntegralConstUnsignedPow2 to fix it's impl.
Also use it in fgMorphUModToAndSub and a place in lower.

[dotnet-policy-service]

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

[BoyBaykiller]

@tannergooding

[BoyBaykiller]

There was an assert failure in arm emitter helper func which I still had to address

[tannergooding]

CC. @dotnet/jit-contrib, @EgorBo, @kg for secondary review

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

from a quick look, another instance of that in fgMorphUModToAndSub

[BoyBaykiller]

The problem is I am not even sure what IsIntegralConstUnsignedPow2() was meant to do in the first place which is why I originally just called popcnt(x) == 1 in my other PR and added a GenTree::UnsignedIntegralValue() thing that gives the zero-extended value.

It seems like existing callers of this function want to know if there is only a single bit set. For example the fgMorphUModToAndSub opt can only be performed in that case - so that's fine and I don't see any issue here. The diffs show that it enables the previously missed 1<<31 case.

Regarding the RISCV code in lower. I am not sure it is doing what the author intended.
We have:

// If 'test' is a single bit test...//if (bitOp->IsIntegralConstUnsignedPow2())
{
INT64 bit = bitOp->AsIntConCommon()->IntegralValue();
int log2 = BitOperations::Log2((UINT64)bit);
bitOp->AsIntConCommon()->SetIntegralValue(log2);
returntrue;
}

The Log2 here must be here to find the bit position. But the code is using bitOp->AsIntConCommon()->IntegralValue() which as we know sign extends values. So this code now gets entered for 1<<31 but then fails with it's assumptions at log2 because bitOp->AsIntConCommon()->IntegralValue() gives us a value with the upper 32 bits set.

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

[EgorBo]

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

I think we should just use truncated value in that RISC-V path

[BoyBaykiller]

I think we should just use truncated value in that RISC-V path

What's the corresponding API for getting such "truncated value"?

[EgorBo]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[BoyBaykiller]

@EgorBo should be good now

[EgorBo]

/ba-g deadletter

[EgorBo]

can't merge it while build analysis is red and /ba-g is broken 😐

[BoyBaykiller]

IsIntegralConstUnsignedPow2 was passing in sign extended value to isPow2. So it wasn't actually unsigned. For example: 1 << 31 would not be recognized as pow2.

Add a UnsignedIntegralValue that gives the zero-extended literal. Use it in IsIntegralConstUnsignedPow2 to fix it's impl.
Also use it in fgMorphUModToAndSub and a place in lower.

[dotnet-policy-service]

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

[BoyBaykiller]

@tannergooding

[BoyBaykiller]

There was an assert failure in arm emitter helper func which I still had to address

[tannergooding]

CC. @dotnet/jit-contrib, @EgorBo, @kg for secondary review

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

[EgorBo]

runtime/src/coreclr/jit/lower.cpp

Lines 4327 to 4329 in bd9e85e

	if (bitOp->IsIntegralConstUnsignedPow2())
	{
	INT64 bit = bitOp->AsIntConCommon()->IntegralValue();

While you change the behavior of this functions to ignore upper bits, it seems like we have uses where that is not expected, we need to inspect them all and fix, otherwise you're introducing a correctness issue

from a quick look, another instance of that in fgMorphUModToAndSub

[BoyBaykiller]

The problem is I am not even sure what IsIntegralConstUnsignedPow2() was meant to do in the first place which is why I originally just called popcnt(x) == 1 in my other PR and added a GenTree::UnsignedIntegralValue() thing that gives the zero-extended value.

It seems like existing callers of this function want to know if there is only a single bit set. For example the fgMorphUModToAndSub opt can only be performed in that case - so that's fine and I don't see any issue here. The diffs show that it enables the previously missed 1<<31 case.

Regarding the RISCV code in lower. I am not sure it is doing what the author intended.
We have:

// If 'test' is a single bit test...//if (bitOp->IsIntegralConstUnsignedPow2())
{
INT64 bit = bitOp->AsIntConCommon()->IntegralValue();
int log2 = BitOperations::Log2((UINT64)bit);
bitOp->AsIntConCommon()->SetIntegralValue(log2);
returntrue;
}

The Log2 here must be here to find the bit position. But the code is using bitOp->AsIntConCommon()->IntegralValue() which as we know sign extends values. So this code now gets entered for 1<<31 but then fails with it's assumptions at log2 because bitOp->AsIntConCommon()->IntegralValue() gives us a value with the upper 32 bits set.

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

[EgorBo]

I think we should add back my GenTree::UnsignedIntegralValue() and use that here (as well as in IsIntegralConstUnsignedPow2)

I think we should just use truncated value in that RISC-V path

[BoyBaykiller]

I think we should just use truncated value in that RISC-V path

What's the corresponding API for getting such "truncated value"?

[EgorBo]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[BoyBaykiller]

@EgorBo should be good now

[EgorBo]

/ba-g deadletter

[EgorBo]

can't merge it while build analysis is red and /ba-g is broken 😐