Issues with the Crc32 intrinsic in the Jit

[SingleAccretion]

1): The assert here is wrong because it does not account for small types. Reproduction:

static uint Problem(byte* pSrc, uint data)
{
    return Sse42.Crc32(*pSrc, data);
}

2) The same assert is also wrong because in lowering we purposefully remove narrowing long -> int casts to avoid generating redundant movs. Reproduction:

static uint Problem(uint crc, ulong data)
{
    return Sse42.Crc32(crc, (uint)(data >> 16));
}

3) The code in lowering is wrong because it can discard meaningful casts. Reproduction (data here will not be zero-extended):

static uint Problem(uint crc, uint data)
{
    return Sse42.Crc32(crc, (uint)(byte)data);
}

4) The code in lowering is also wrong because it can discard long -> int casts on x86, and end up with a GT_LONG operand, which is not handled downstream. Reproduction, run on x86:

[MethodImpl(MethodImplOptions.NoOptimization)]
static uint Problem(uint crc, ulong data)
{
    return Sse42.Crc32(crc, (uint)(data >> 16));
}

5) The code in lowering is also wrong because it can discard double/float -> int casts. Applies to all use sites of TryRemoveCastIfPresent. Reproduction:

static uint Problem(uint crc, double data)
{
    return Sse42.Crc32(crc, (uint)data);
}

Curiously enough, if we disable asserts, this will happily emit crc32 eax, xmm1.

6) The code in lowering is also wrong because it can discard checked casts. Applies to all use sites of TryRemoveCastIfPresent. Reproduction (observe the generated code will have no check):

static uint Problem(uint crc, int data)
{
    return Sse42.Crc32(crc, checked((uint)data));
}

[ghost]

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

Issue Details

---

1): The assert here is wrong because it does not account for small types. Reproduction:

static uint Problem(byte* pSrc, uint data)
{
    return Sse42.Crc32(*pSrc, data);
}

2) The same assert is also wrong because in lowering we purposefully remove narrowing long -> int casts to avoid generating redundant movs. Reproduction:

static uint Problem(uint crc, ulong data)
{
    return Sse42.Crc32(crc, (uint)(data >> 16));
}

3) The code in lowering is also wrong because it can discard meaningful casts. Reproduction (data here will not be zero-extended):

static uint Problem(uint crc, uint data)
{
    return Sse42.Crc32(crc, (uint)(byte)data);
}

Author:	SingleAccretion
Assignees:	-
Labels:	area-CodeGen-coreclr, untriaged
Milestone:	-

[EgorBo]

The LSRA assert is the 4th case?

[SingleAccretion]

Yep.

[deeprobin]

@EgorBo Do you know approximately when you're going to get around to looking at this issue?

It's probably nothing big, but the issue is blocking me (but nothing critical) :)

[EgorBo]

Looking at it now, sorry for the delay

[deeprobin]

Looking at it now, sorry for the delay

no problem. thanks