The actual operands here are all swapped around. This is generating result = (upper << count) | (lower >>> (32 - count)) for example.

For DecomposeRotate when count < 32, this ends up being the following (which is correct):

result.lower = (value.lower << count) | (value.upper >>> (32 - count))
result.upper = (value.upper << count) | (value.lower >>> (32 - count))

This is because DecomposeRotate passes it in as hiCopy, loOp1 and then as loCopy, hiOp1.

Ideally this would be swapped so that its passed in as loOp1, hiCopy then hiOp1, loCopy with codegen doing:

    regNumber tgtReg = tree->GetRegNum();
    assert(regHi != tgtReg);

    inst_Mov(targetType, tgtReg, regLo, /* canSkip */ true);
    inst_RV_RV_IV(ins, emitTypeSize(targetType), tgtReg, regHi, count);

However, swapping these + LSRA was leading to failures and I didn't bother investigating further since its already working "as is".

However, swapping these + LSRA was leading to failures

I wonder if that is because you need to change LIR emitted for the ROR (the order of args/clones)

I had thought I had gotten that all when testing, but it's possible I missed something when trying. It's likely not worth extra effort to solve though since this is all working now.

presumably nothing prevents us from folding firstOp[0] to a constant and then perform constant folding in VN so the actual instruction will never be emitted

Right, but the bug repros in T0 and Debug, so it should get hit in general unless we start doing those foldings there as well

it sounds that we waste TP in Tier0 to recognize this pattern

The bug repro'd in T0 because we recognize long.RotateRight in import. It's a primitive intrinsic which is trivial to recognize, much like the other ones we recognize in T0.

it's a primitive intrinsic which is trivial to recognize

Ah ok, I though it's about that bigger one in morph that recognizes plain shifts