[Interpreter] Array #6965

tonerdo · 2019-02-07T17:24:00Z

This PR adds support for single-dimension zero-based arrays. It adds interpretation of the following opcodes:

newarr
ldlen
stelem.*
ldelem.*

which allows methods like the following to be interpreted:

public static int[] GetArray()
{
    int[] array = new int[5];
    array[0] = 23;
    array[1] = 34;
    array[2] = 43;
    array[3] = 122;
    array[4] = 4;
    return array;
}

public static int GetArrayElement(int[] array, int index)
{
    return array[index];
}

public static int GetArrayLength(int l)
{
    return new int[l].Length;
}

tonerdo · 2019-02-07T17:28:02Z

src/System.Private.Interpreter/src/Internal/Runtime/Interpreter/ILInterpreter.cs

+            Debug.Assert(length >= 0);
+
+            TypeDesc arrayType = ((TypeDesc)_methodIL.GetObject(token)).MakeArrayType();
+            Array array = RuntimeAugments.NewArray(arrayType.GetRuntimeTypeHandle(), length);


This doesn't work for non primitive types. Something like new DateTime[10] fails because the runtime is unable to find the RuntimeTypeHandle for DateTime[] even though there exists one for DateTime. Is there a way to probe the metadata of the interpreted assembly (and its dependencies) for runtime type information?

It should work for arbitrary reference type arrays - value types are harder.

You should be able to work around by adding this array to RD.XML or adding typeof(DateTime[]) somewhere in the code that hosts the interpreter.

Hmm....didn't work when I tried it with an Exception type. I'll just add a TODO to add support for arbitrary types

tonerdo · 2019-02-07T17:30:41Z

src/System.Private.Interpreter/src/Internal/Runtime/Interpreter/ILInterpreter.cs

+                    array.SetValue((sbyte)valueItem.AsInt32(), index);
+                    break;
+                case ILOpcode.stelem_i2:
+                    array.SetValue((short)valueItem.AsInt32(), index);


I would've preferred to use the generic type Array<T> to prevent unnecessary boxing but when I do, ILC fails to compile the CoreRT console app with the following error:

Assertion Failed Asking for Array<T> EEType at ILCompiler.DependencyAnalysis.EETypeNode.OnMarked(NodeFactory context) in C:\Users\toni\Workspace\corert\src\ILCompiler.Compiler\src\Compiler\DependencyAnalysis\EETypeNode.c s:line 1087 at ILCompiler.DependencyAnalysisFramework.DependencyNodeCore`1.CallOnMarked(DependencyContextType context) in C:\Users\toni\Workspace\corert\src\ILCompiler.DependencyAnalysisFr amework\src\DependencyNodeCore.cs:line 136 at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.AddToMarkStack(DependencyNodeCore`1 node, String reason, DependencyNodeCore`1 reason1, DependencyNodeCore`1 reaso n2) in C:\Users\toni\Workspace\corert\src\ILCompiler.DependencyAnalysisFramework\src\DependencyAnalyzer.cs:line 313 at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.GetStaticDependenciesImpl(DependencyNodeCore`1 node) in C:\Users\toni\Workspace\corert\src\ILCompiler.DependencyA nalysisFramework\src\DependencyAnalyzer.cs:line 150 at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ComputeMarkedNodes() in C:\Users\toni\Workspace\corert\src\ILCompiler.DependencyAnalysisFramework\src\DependencyA nalyzer.cs:line 266 at ILCompiler.RyuJitCompilation.CompileInternal(String outputFile, ObjectDumper dumper) in C:\Users\toni\Workspace\corert\src\ILCompiler.RyuJit\src\Compiler\RyuJitCompilation.c s:line 40 at ILCompiler.Compilation.ILCompiler.ICompilation.Compile(String outputFile, ObjectDumper dumper) in C:\Users\toni\Workspace\corert\src\ILCompiler.Compiler\src\Compiler\Compila tion.cs:line 387 at ILCompiler.Program.Run(String[] args) in C:\Users\toni\Workspace\corert\src\ILCompiler\src\Program.cs:line 575 at ILCompiler.Program.Main(String[] args) in C:\Users\toni\Workspace\corert\src\ILCompiler\src\Program.cs:line 731

The runtime builds successfully

Array<T> is just a type that we have in the corelib so that we can write methods that support the generic interfaces on arrays. We take it's vtable and use it as the vtable of T[]. The type itself never exists. Even if we were to remove the check in the compiler, you wouldn't be able to cast to it at runtime because Array<T> is different from T[].

You can call GetRawArrayData() to get a byref to the first element of the array and then use Unsafe class to do pointer arithmetic and writes/reads. Note that it will require adding bounds checks.

GetArrayData is internal to System.Private.CoreLib so I can't use it

I wouldn't mind making it public. The other option is to expose it through the RuntimeAugments class which is how we do it for other corelib internal APIs (reasons for RuntimeAugments are historic but we still do it for consistency).

Is this what you have in mind?

ref byte start = ref RuntimeAugments.GetRawDataForArray(array); ref byte position = ref Unsafe.Add(ref start, index); // Read from int[] Unsafe.Read<int>(ref position); // Write to int[] Unsafe.Write<int>(ref position, 23);

Is this what you have in mind?

We need to multiply index by element size (available as Array.ElementSize in the CoreLib), but yes, that's the idea.

Maybe use Unsafe.As to cast to the right element type and avoid the Unsafe.Read and Unsafe.Write. I don't know whether Unsafe.Write does the right write barriers if the element is a reference type.

I don't know whether Unsafe.Write does the right write barriers if the element is a reference type.

It works fine.

Thanks! Array.ElementSize is definitely what I was missing

src/System.Private.CoreLib/src/Internal/Runtime/Augments/RuntimeAugments.cs

jkotas · 2019-02-11T11:12:33Z

src/System.Private.Interpreter/src/Internal/Runtime/Interpreter/ILInterpreter.cs

+                    index = indexItem.AsInt32();
+                    break;
+                case StackValueKind.NativeInt:
+                    index = (int)indexItem.AsNativeInt();


This needs bounds check.

Do you mean the conversion from IntPtr to int or the the value of index relating to the array length (I already have this in my local branch)?

Both of them.

jkotas · 2019-02-11T11:22:53Z

src/System.Private.Interpreter/src/Internal/Runtime/Interpreter/ILInterpreter.cs

+                    Unsafe.Write(ref position, valueItem.AsDouble());
+                    break;
+                case ILOpcode.stelem_ref:
+                    Unsafe.Write(ref position, valueItem.AsObjectRef());


This needs assignability check. It should end up calling System.Runtime.TypeCast.StelemRef

Either need to expose the method to do this via RuntimeAugments; or cast the array to object[] and assign it (Array.SetValue does this).

jkotas · 2019-02-12T11:16:41Z

src/System.Private.Interpreter/src/Internal/Runtime/Interpreter/ILInterpreter.cs

+                case StackValueKind.NativeInt:
+                    {
+                        long value = (long)indexItem.AsNativeInt();
+                        Debug.Assert(value >= int.MinValue && value <= int.MaxValue);


This should throw IndexOutOfRangeException, not assert.

jkotas · 2019-02-12T11:18:21Z

src/System.Private.Interpreter/src/Internal/Runtime/Interpreter/ILInterpreter.cs

+                    break;
+            }
+
+            if (index < 0 || index >= array.Length)


Nit: This can be if ((uint)index >= (uint)array.Length) to make it a bit faster. We use this trick frequently for array bound checks.

Maybe this check can be inside GetSzArrayElementAddress.

We use this trick frequently for array bound checks.

Oh I saw it on Dictionary, is there also some special jit treatment if it recognize pattern?

It is one comparison instead of two. It would be nice if the JIT recognized the two comparisons and converted them into one, but we are not there yet.

jkotas · 2019-02-12T11:22:26Z

src/System.Private.CoreLib/src/Internal/Runtime/Augments/RuntimeAugments.cs

@@ -158,6 +164,17 @@ public static unsafe Array NewMultiDimArray(RuntimeTypeHandle typeHandleForArray
            return Array.NewMultiDimArray(typeHandleForArrayType.ToEETypePtr(), pLengths, lengths.Length);
        }

+        public static void StelemRef(Array array, int index, object obj)
+        {
+            TypeCast.StelemRef(array, index, obj);


I suspect that this won't compile in the closed source ProjectN build (the Runtime,Base is not part of CoreLib in that build). @MichalStrehovsky Do you agree?

I think it would be easier to delete this method; and replace the calls with Unsafe.As<Object[]>(array)[index] = obj;.

Yeah, it wouldn't compile. Unsafe.As sounds good!

jkotas

Thank you!

tonerdo added 6 commits February 5, 2019 14:16

add support for newarr opcode (dotnet#5011)

45abf53

add support for ldlen opcode (dotnet#5011)

24b5131

add support for stelem.* opcodes (dotnet#5011)

94a016b

add support for ldelem.* opcodes (dotnet#5011)

b85b5bb

use the right arguement type metadata (dotnet#5011)

26c1315

handle when index values are native int (dotnet#5011)

cefb29f

tonerdo commented Feb 7, 2019

View reviewed changes

use generic Unsafe class to read/write array elements (dotnet#5011)

93cbf3c

jkotas reviewed Feb 11, 2019

View reviewed changes

src/System.Private.CoreLib/src/Internal/Runtime/Augments/RuntimeAugments.cs Show resolved Hide resolved

jkotas reviewed Feb 11, 2019

View reviewed changes

tonerdo added 4 commits February 11, 2019 12:44

add array bounds check (dotnet#5011)

1a675e1

remove nuint from public api surface (dotnet#5011)

b970c0c

add assignability checks for stelem.ref opcode (dotnet#5011)

966ce8b

add IntPtr to int conversion bounds check (dotnet#5011)

74f24c9

jkotas reviewed Feb 12, 2019

View reviewed changes

tonerdo added 2 commits February 12, 2019 12:37

address PR comments (dotnet#5011)

9517413

simplify IntPtr<->int bounds check expression (dotnet#5011)

591b308

jkotas approved these changes Feb 12, 2019

View reviewed changes

jkotas merged commit d8d1a91 into dotnet:master Feb 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Interpreter] Array #6965

[Interpreter] Array #6965

tonerdo commented Feb 7, 2019

tonerdo Feb 7, 2019

MichalStrehovsky Feb 7, 2019

tonerdo Feb 7, 2019

tonerdo Feb 7, 2019

MichalStrehovsky Feb 7, 2019

tonerdo Feb 10, 2019

MichalStrehovsky Feb 10, 2019

tonerdo Feb 11, 2019

MichalStrehovsky Feb 11, 2019

MichalStrehovsky Feb 11, 2019

jkotas Feb 11, 2019

tonerdo Feb 11, 2019

jkotas Feb 11, 2019

tonerdo Feb 11, 2019

jkotas Feb 11, 2019

jkotas Feb 11, 2019

jkotas Feb 11, 2019

jkotas Feb 12, 2019

jkotas Feb 12, 2019

jkotas Feb 12, 2019

MarcoRossignoli Feb 12, 2019

jkotas Feb 12, 2019

jkotas Feb 12, 2019 •

edited

Loading

MichalStrehovsky Feb 12, 2019

jkotas left a comment

[Interpreter] Array #6965

[Interpreter] Array #6965

Conversation

tonerdo commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

jkotas Feb 12, 2019 •

edited

Loading