dotnet / dotnet/machinelearning
Avoids pointer overflow in bound checking of SSE/AVX intrinsics
- Dominant language
- C#
- Stars
- 9.4k
- Forks
- 2k
- Avg merge
- 2d 20h
- Merged PRs (30d)
- 11
Description
#821 references and aims to solve this issue.
---
Suggested by @ahsonkhan to avoid integer overflow in bound checking inside SSE/AVX intrinsics implementation, i.e. change all `while (pCurrent + 8 OR 4 <= pEnd)` into `while (pEnd - pCurrent >= 8 OR 4)`.
Perf tests results before and after the change are shown below:
## Before the change:
``` ini
BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.228 (1803/April2018Update/Redstone4)
Intel Core i7-7700 CPU 3.60GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100-alpha1-20180720-2
[Host] : .NET Core 3.0.0-preview1-26710-03 (CoreCLR 4.6.26710.05, CoreFX 4.6.26708.04), 64bit RyuJIT
Toolchain=InProcessToolchain
```
| Type | Method | Mean | Error | StdDev |
|----------------------- |------- |---------:|---------:|----------:|
| AvxPerformanceTests | SumU | 159.4 us | 1.104 us | 0.9784 us |
| NativePerformanceTests | SumU | 283.5 us | 5.492 us | 4.8687 us |
| SsePerformanceTests | SumU | 281.2 us | 1.472 us | 1.3045 us |
| AvxPerformanceTests | AddU | 276.1 us | 3.018 us | 2.520 us |
| NativePerformanceTests | AddU | 330.1 us | 3.585 us | 3.178 us |
| SsePerformanceTests | AddU | 325.6 us | 6.883 us | 7.926 us |
## After the change:
``` ini
BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.228 (1803/April2018Update/Redstone4)
Intel Core i7-7700 CPU 3.60GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100-alpha1-20180720-2
[Host] : .NET Core 3.0.0-preview1-26710-03 (CoreCLR 4.6.26710.05, CoreFX 4.6.26708.04), 64bit RyuJIT
Toolchain=InProcessToolchain
```
| Type | Method | Mean | Error | StdDev |
|----------------------- |------- |---------:|---------:|---------:|
| AvxPerformanceTests | SumU | 183.5 us | 3.621 us | 3.023 us |
| NativePerformanceTests | SumU | 281.6 us | 5.261 us | 4.921 us |
| SsePerformanceTests | SumU | 294.1 us | 2.080 us | 1.946 us |
| AvxPerformanceTests | AddU | 296.3 us | 5.185 us | 4.850 us |
| NativePerformanceTests | AddU | 335.1 us | 3.053 us | 2.707 us |
| SsePerformanceTests | AddU | 345.0 us | 2.155 us | 1.800 us |
Both SSE and AVX implementations are slower by 10-20% after this change.
In my opinion, after seeing the perf results, I may not recommend merging this PR. I may wait until the alternative suggested by @tannergooding in an earlier PR review has been implemented (2nd item under "Functionality" in https://github.com/briancylui/machinelearning/issues/2):
``` log
var remainder = count % elementsPerIteration;
float* pEnd = pdst + (count - remainder);
while (pDstCurrent < pEnd)
{ … }
```
Another question I have is: would `pDstCurrent + 8 OR 4` ever have the possibility to result in integer overflow? According to my knowledge, `pEnd` is initialized as `pDstCurrent + count`, and there are `Contract.Asserts` in the wrapper class to check that `count` does not exceed the original array length. I'm not sure, and am open to any PR comments and advice.
cc: @danmosemsft @eerhardt @tannergooding @ahsonkhan @markusweimer
Contributor guide
Assessment
This issue has not been assessed yet.