Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Object and UIntPtr as valid cast from pointer #512

Merged
merged 2 commits into from
Jan 2, 2021

Conversation

ThomasGoulet73
Copy link

Removes Object and UIntPtr as valid cast from pointer.

Fixes the following code which returned true and now returns false:

typeof(object).IsAssignableFrom(typeof(byte*))
typeof(UIntPtr).IsAssignableFrom(typeof(byte*))

Fixes #189

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

I spent some time research why this code was there in the first place, since this was written by Ati and that code is usually very well researched.

Seems like .NET Core did a breaking change in this behavior. The code being deleted is compatible with desktop .NET Framework and that's why it was there.

@MichalStrehovsky
Copy link
Member

Ah, could you also delete these lines:

<ExcludeList Include="$(XunitTestBinBase)/JIT/Intrinsics/TypeIntrinsics_r/*">
<Issue>https://github.com/dotnet/runtimelab/issues/189</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/JIT/Intrinsics/TypeIntrinsics_ro/*">
<Issue>https://github.com/dotnet/runtimelab/issues/189</Issue>
</ExcludeList>

So that we run the tests in the CI.

@dnfadmin
Copy link

dnfadmin commented Jan 2, 2021

CLA assistant check
All CLA requirements met.

@ThomasGoulet73
Copy link
Author

ThomasGoulet73 commented Jan 2, 2021

I wasn't able to test locally, I kept getting errors while building with this command: src\tests\build.cmd nativeaot Release. I got the following error: The CoreCLR artifacts path does not exist 'XXX\Runtimeab\artifacts\bin\coreclr\windows.x64.Release\'. The weird thing is that the path XXX\Runtimeab\artifacts\bin\coreclr\windows.x64.Debug\ exists even though I built using release. I'll try to get my local build working if the CI fails.

@MichalStrehovsky
Copy link
Member

If the Debug directory exists, the runtime must have been built with Debug configuration.

build.cmd nativeaot+libs+nativeaot.packages -c Release will build everything as Release. build.cmd nativeaot+libs+nativeaot.packages -rc Debug -lc Release will build the runtime (runtime config) as Debug, and libraries (library config) as release. You can combine these in various ways. The runtime test scripts always assume the libraries were built in Release config and the config that is passed to the script is the Runtime config.

We really need a more user friendly approach to the development inner loop. I rarely run the test scripts locally because it's a bit annoying.

@ThomasGoulet73 ThomasGoulet73 deleted the issue189 branch February 15, 2021 19:01
runtimelab-bot pushed a commit that referenced this pull request Feb 8, 2022
# Local heap optimizations on Arm64

1. When not required to zero the allocated space for local heap (for sizes up to 64 bytes) - do not emit zeroing sequence. Instead do stack probing and adjust stack pointer:

```diff
-            stp     xzr, xzr, [sp,#-16]!
-            stp     xzr, xzr, [sp,#-16]!
-            stp     xzr, xzr, [sp,#-16]!
-            stp     xzr, xzr, [sp,#-16]!
+            ldr     wzr, [sp],#-64
```

2. For sizes less than one `PAGE_SIZE` use `ldr wzr, [sp], #-amount` that does probing at `[sp]` and allocates the space at the same time. This saves one instruction for such local heap allocations:

```diff
-            ldr     wzr, [sp]
-            sub     sp, sp, #208
+            ldr     wzr, [sp],#-208
```

Use `ldp tmpReg, xzr, [sp], #-amount` when the offset not encodable by post-index variant of `ldr`:
```diff
-            ldr     wzr, [sp]
-            sub     sp, sp, #512
+            ldp     x0, xzr, [sp],#-512
```

3. Allow non-loop zeroing (i.e. unrolled sequence) for sizes up to 128 bytes (i.e. up to `LCLHEAP_UNROLL_LIMIT`). This frees up two internal integer registers for such cases:

```diff
-            mov     w11, #128
-                                               ;; bbWeight=0.50 PerfScore 0.25
-G_M44913_IG19:        ; gcrefRegs=00F9 {x0 x3 x4 x5 x6 x7}, byrefRegs=0000 {}, byref, isz
             stp     xzr, xzr, [sp,#-16]!
-            subs    x11, x11, #16
-            bne     G_M44913_IG19
+            stp     xzr, xzr, [sp,#-112]!
+            stp     xzr, xzr, [sp,#16]
+            stp     xzr, xzr, [sp,#32]
+            stp     xzr, xzr, [sp,#48]
+            stp     xzr, xzr, [sp,#64]
+            stp     xzr, xzr, [sp,#80]
+            stp     xzr, xzr, [sp,#96]
```

4. Do zeroing in ascending order of the effective address:

```diff
-            mov     w7, #96
-G_M49279_IG13:
             stp     xzr, xzr, [sp,#-16]!
-            subs    x7, x7, #16
-            bne     G_M49279_IG13
+            stp     xzr, xzr, [sp,#-80]!
+            stp     xzr, xzr, [sp,#16]
+            stp     xzr, xzr, [sp,#32]
+            stp     xzr, xzr, [sp,#48]
+            stp     xzr, xzr, [sp,#64]
```

In the example, the zeroing is done at `[initialSp-16], [initialSp-96], [initialSp-80], [initialSp-64], [initialSp-48], [initialSp-32]` addresses. The idea here is to allow a CPU to detect the sequential `memset` to `0` pattern and switch into write streaming mode.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants