Cesium icon indicating copy to clipboard operation
Cesium copied to clipboard

Strange approach to `ldind`

Open ForNeVeR opened this issue 1 year ago • 1 comments

Currently, PrimitiveType.cs has the following code:

    internal static readonly Dictionary<PrimitiveTypeKind, (OpCode load, OpCode store)> Opcodes = new()
    {
        { PrimitiveTypeKind.Char, (OpCodes.Ldind_I1, OpCodes.Stind_I1) },
        { PrimitiveTypeKind.SignedChar, (OpCodes.Ldind_I1, OpCodes.Stind_I1) },
        { PrimitiveTypeKind.UnsignedChar, (OpCodes.Ldind_U1, OpCodes.Stind_I1) },
        { PrimitiveTypeKind.Short, (OpCodes.Ldind_I2, OpCodes.Stind_I2) },
        { PrimitiveTypeKind.UnsignedShort, (OpCodes.Ldind_I2, OpCodes.Stind_I2) },
        { PrimitiveTypeKind.Int, (OpCodes.Ldind_I4, OpCodes.Stind_I4) },
        { PrimitiveTypeKind.UnsignedInt, (OpCodes.Ldind_I4, OpCodes.Stind_I4) },
        { PrimitiveTypeKind.Float, (OpCodes.Ldind_R4, OpCodes.Stind_R4) },
        { PrimitiveTypeKind.Long, (OpCodes.Ldind_I8, OpCodes.Stind_I8) },
        { PrimitiveTypeKind.UnsignedLong, (OpCodes.Ldind_I8, OpCodes.Stind_I8) },
        { PrimitiveTypeKind.Double, (OpCodes.Ldind_R8, OpCodes.Stind_R8) }
    };

Note how all the other unsigned types have same get operation as their signed counterparts, except UnsignedChar that has OpCodes.Ldind_U1.

In CIL, there are four unsigned ldind operations: ldind.u1, ldind.u2, and ldind.u4 (notably, ldind.u8 is absent). And we don't use them and use the signed versions.

Another interesting part is that there are no stind.u* operations: all the setters are always signed.

We should figure out if there's any visible difference between these operations, and either

  • use the signed ones consistently for all the unsigned types
  • or use the unsigned operations where available

ForNeVeR avatar Nov 12 '23 20:11 ForNeVeR

A possible test case (in C# for explanation's sake, so we'll of course need to convert to C):

sbyte[] x = new[] { -1 };
sbyte y = x[0]; // 2
int z = (int)y; // 3

My hypothesis is that the int cast in line 3 won't do anything actually, and thus its result will depend on whether the ldind operation in line 2 does sign-extension or zero-extension of the sbyte value in the array x.

ForNeVeR avatar Nov 12 '23 20:11 ForNeVeR

Hi @ForNeVeR

I’m new here and I would like to take this one. Before I start making any changes, I want to make sure I understand what we’re dealing with here.

I guess we want to know if we should use unsigned ldind operations for unsigned types or signed ldind operations. So, the first thing to do is write test cases in C to examine how sign-extension and zero-extension affect the values when converting integer types.

So once we have a clear understanding , we'll decide whether to:

  • Use the signed ldind operations consistently for all unsigned types, or

  • Use the unsigned ldind operations where available.

In summary, my approach includes:

  1. Writing C test cases to observe the behavior of sign-extension and zero-extension.
  2. Analyzing the results to determine the appropriate usage of ldind instructions.
  3. Updating the PrimitiveType.cs file based on the findings.

Does this approach sound good ?

Thanks in advance

PapisKang avatar Jun 11 '24 21:06 PapisKang

My plan was to check the behavior of C#, and just do the same in Cesium 😅

(possibly by modifying the referenced code in PrimitiveType.cs and how's it used)

Generally, for CIL, signed vs unsigned numerics are pretty opaque, you can load them using signed instructions and then operate on them using unsigned ones, so I don't expect too much observable difference w.r.t. to the C standard.

So, my direction of thought was to just take a decision (probably by looking at C#), update the implementation, and make sure it's covered by tests (possibly by adding some new ones, see the test guide — I believe the bytecode tests should be enough).

I don't currently believe this task requires any integration / actual runnable C tests.

In everything else, I think your summary is correct.

ForNeVeR avatar Jun 12 '24 11:06 ForNeVeR

Ooh Thanks for the clarification. It looks easier then. I ll modify PrimitiveType.cs and see how it handles. And i ll probably add tests as you suggest and make sure they all are well covered

PapisKang avatar Jun 12 '24 13:06 PapisKang