Change bound checking in SSE/AVX intrinsics to avoid pointer overflow - #821

Closed
briancylui wants to merge 3 commits into
dotnet:masterfrom
briancylui:SecureBoundChecks
Closed

Change bound checking in SSE/AVX intrinsics to avoid pointer overflow#821
briancylui wants to merge 3 commits into
dotnet:masterfrom
briancylui:SecureBoundChecks

Conversation

@briancylui

@briancyluibriancylui commented Sep 5, 2018•
edited
Loading

Copy link
Copy Markdown

Aims to solve #980

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:

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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU159.4 us1.104 us0.9784 us
NativePerformanceTestsSumU283.5 us5.492 us4.8687 us
SsePerformanceTestsSumU281.2 us1.472 us1.3045 us
AvxPerformanceTestsAddU276.1 us3.018 us2.520 us
NativePerformanceTestsAddU330.1 us3.585 us3.178 us
SsePerformanceTestsAddU325.6 us6.883 us7.926 us

After the change:

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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU183.5 us3.621 us3.023 us
NativePerformanceTestsSumU281.6 us5.261 us4.921 us
SsePerformanceTestsSumU294.1 us2.080 us1.946 us
AvxPerformanceTestsAddU296.3 us5.185 us4.850 us
NativePerformanceTestsAddU335.1 us3.053 us2.707 us
SsePerformanceTestsAddU345.0 us2.155 us1.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 briancylui#2):

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

@eerhardt

eerhardt commented Sep 5, 2018•
edited
Loading

Copy link
Copy Markdown
Member

I’m not sure I follow the reasoning. These are pointer operations. So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?
I also agree with the point that we are checking the array/span length above these methods, so we are guaranteed to be within the bounds of memory.

@briancylui

briancylui commented Sep 5, 2018

Copy link
Copy Markdown
Author

@ahsonkhan: I may share the same concern as @eerhardt - would love to learn and hear back.

@ahsonkhan

ahsonkhan commented Sep 5, 2018•
edited
Loading

Copy link
Copy Markdown

Another question I have is: would pDstCurrent + 8 OR 4 ever have the possibility to result in integer overflow

Imo, given these are public APIs that anyone can call (with potentially invalid inputs), the inputs should be verified within the method body. Being explicit about assertions like src length == dst length would be good (and also act as self-documentation).
Edit: Nevermind, the class is internal.

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.

I am not too familiar with the code base here, but don't contract.asserts run in debug mode only? Are these checks unnecessary in release?

Both SSE and AVX implementations are slower by 10-20% after this change.

How about something like the following:

inti=0;for(;i<src.Length-8;i+=8){Vector256<float>srcVector=Avx.LoadVector256(pSrcCurrent);Vector256<float>dstVector=Avx.LoadVector256(pDstCurrent);result256=Avx.Add(result256,Avx.Multiply(srcVector,dstVector));pSrcCurrent+=8;pDstCurrent+=8;}if(src.Length-i<=4){
...i+=4;}while(i<src.Length){
...i++;}

The number of assembly instruction is essentially identical here (saves a lea, costs an extra add): https://www.diffchecker.com/MywBeoFH (before in red, after in green)

image

Just my two cents. I would leave it up to others who have more context in this space to validate.

Would the OS ever let us get that close?

No, it won't. I discussed this with @GrabYourPitchforks, and its a general coding guideline to avoid arithmetic overflows like this (on the, albeit unlikely, chance the OS behavior changes in the future).

@briancylui

briancylui commented Sep 5, 2018

Copy link
Copy Markdown
Author

@ahsonkhan: Thank you for your comments! My apologies that I may have given the wrong hint that the Sse/AvxIntrinsics class is public during our previous conversation - it's actually internal after checking. Thank you very much for pointing that out!

Regarding Contracts.Assert being run in Debug only, it may be a very good point for future follow-up. If the APIs currently do not do any length checking in Release, probably we should do that in the future.

Thank you for the link to the DiffChecker - it looks amazing! Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?

@ahsonkhan

ahsonkhan commented Sep 5, 2018•
edited
Loading

Copy link
Copy Markdown

Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?

That has a similar concern (but with underflow). If src.Length < 8, and in the unlikely chance that the array starts at a memory address close to the beginning of the address space, then (pCurrent <= pEnd - 8) would be true, even though it shouldn't. That is because comparisons are done as if they are unsigned integers.

https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/unsafe-code-pointers/pointer-comparison

The comparison operators compare the addresses of the two operands as if they are unsigned integers.

For example:

// Assume:src.Length=6;pCurrent=4;pEnd=pCurrent+src.Length// 4 + 6 * 4 = 28;if(pCurrent<=pEnd-8){// 4 <= 28 - (8 * 4)// 4 <= 28 - 32// 4 <= (ulong)-4// We don't expect to be here, but we will since -4 as a ulong is a really large number.}

@GrabYourPitchforks

GrabYourPitchforks commented Sep 6, 2018

Copy link
Copy Markdown
Member

So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?

@ahsonkhan and I spoke about this at length yesterday.

With current operating systems, no. But I don't know what OSes we'll be running on in 10 years. Maybe we'll have a fully managed OS like Singularity, and maybe it'll give us access to the full range of addressable memory. I can't predict the future, and I don't want to chance having to chase down logic errors in this code in ten years' time if there's a theoretical overflow condition that we can identify and address now.

@GrabYourPitchforks

GrabYourPitchforks commented Sep 6, 2018•
edited
Loading

Copy link
Copy Markdown
Member

FWIW, there is an exception that both C# and C allow when comparing pointers. The element just past the end of the array is always addressable, and it's always guaranteed to compare greater than the address of any element in the array.

That is:

T*ptr; // = pointer to first element in arraysize_tnumElements; // total number of elements in the arrayfor (size_ti=0; i<numElements; i++) {
assert(&ptr[i] <&ptr[numElements], "This is guaranteed by the language.");
}

A corollary to this is that T* end = ptr + numElements is guaranteed not to integer overflow (so the array can't be placed at the very, very end of addressable memory), but T* end = ptr + numElements + 1 has no such guarantee.

Note: the element just past the end of the array is addressable but not necessarily dereferenceable. That is, T element = ptr[numElements] still has undefined behavior, such as populating the register with garbage or even AVing.

@eerhardt

eerhardt commented Sep 6, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Ah, ok. I did some more reading, looked at this code again and I understand the concern. The array may be at the very end of memory space. And when pDstCurrent is less than 8 elements from the end, then while (pDstCurrent + 8 <= pDstEnd) will go out of bounds, and potentially overflow. And if it overflows, it will wrap around making condition true.

I think it makes sense using the for (; i < src.Length - 8; i += 8) pattern that @ahsonkhan proposes above or the remainder pattern suggested by @tannergooding in an earlier PR review.

@briancylui

briancylui commented Sep 6, 2018

Copy link
Copy Markdown
Author

Thanks for all the comments. After some reading, I agree that the problem is potential illegal memory access due to incrementing the pCurrent pointer out of bounds of the Span<float> object. Accessing out-of-bound object can already cause security concerns, and it would be even worse if the pointer got incremented into illegal memory. I should probably change the title of this PR to "avoid pointer overflow" or "avoid illegal memory access" to avoid any possible confusion, and look forward to implementing this fix tomorrow.

A very good takeaway from @ahsonkhan and @GrabYourPitchforks's comments is that we may not want to increment/decrement any pointer for bound checking, and I think it is also a very good opportunity to follow through on @eerhardt and @tannergooding's suggestion, which will pave the way for implementing double-computing. I will spend some time tomorrow to look into how to implement these fixes concretely.

Thank you very much for all the comments!

@briancyluibriancylui changed the title Change bound checking in SSE/AVX intrinsics to avoid integer overflowChange bound checking in SSE/AVX intrinsics to avoid pointer overflowSep 6, 2018
@briancylui

briancylui commented Sep 7, 2018

Copy link
Copy Markdown
Author

Pushed a new commit and deleted the previous commit to show you all a sample of the changes I will make globally.

In the new commit, only the AddScalarU intrinsic has been changed in its SSE and AVX implementations. Once you think that the changes look fine to you, or you may some suggestions to edit them, the changes can be made on a global scale to the rest of the intrinsics.

The performance test results are shown below:

After the change:

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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU142.9 us2.8338 us7.0045 us140.1 us
NativePerformanceTestsAddScalarU191.2 us2.3916 us1.8672 us190.9 us
SsePerformanceTestsAddScalarU172.7 us0.7552 us0.5896 us172.6 us

Before the change:

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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU141.8 us1.623 us1.439 us141.5 us
NativePerformanceTestsAddScalarU191.8 us4.514 us10.901 us186.5 us
SsePerformanceTestsAddScalarU180.3 us3.653 us7.125 us177.1 us

The performance results are comparable after the change: SEE got faster by a bit, while AVX got slower by just a bit. The second use of Math.DivRem for countSse and remainderSse may have made the AVX implementation a bit slower.

Look forward to all your comments. Since tomorrow is the last day of my internship, and I have other work items to finish before I go, I don't think I can finish this PR by tomorrow, but I will keep my branch in my fork up so that other contributors can use your previous commits. While I will leave this PR open for the moment, please feel free to close it whenever you feel appropriate. Thank you.

@briancylui

briancylui commented Sep 7, 2018

Copy link
Copy Markdown
Author

test MachineLearning-CI please

tannergooding
tannergooding reviewed Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs Outdated
private static readonly Vector256<float> _absMask256 = Avx.StaticCast<int, float>(Avx.SetAllVector256(0x7FFFFFFF));

// The count of 32-bit floats in Vector256<T>
private const int AvxAlignment = 8;

@tannergoodingtannergoodingSep 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This isn't alignment, but rather the number of 32-bit elements that Vector256 can hold...

Maybe Vector256SingleElementCount or something similar...

@briancyluibriancyluiSep 7, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

What about Vector256FloatCount? Is there any preference for SingleElement?

tannergooding
tannergooding reviewed Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs Outdated
tannergooding
tannergooding reviewed Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/SseIntrinsics.cs Outdated
while (pDstCurrent < pDstEnd)
for (int i = 0; i < remainder; i++)
{
Vector128<float> dstVector = Sse.LoadScalarVector128(pDstCurrent);

@tannergoodingtannergoodingSep 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This would be way more readable as pDstCurrent[i] += scalar, and it should produce the same code.

The various scalar intrinsics are really meant for places where you have to interop between Vector and Scalar code, or where you need some scalar operations which aren't expressible in normal C# code.

@briancyluibriancyluiSep 7, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, perf test results show that this change improves the runtime significantly:

After all changes, including changing scalar intrinsics to indexed code:

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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us

After partial changes (removing the 2nd time of Math.DivRem):

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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us

Before the change:

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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us

@briancylui

briancylui commented Sep 7, 2018•
edited
Loading

Copy link
Copy Markdown
Author

Pushed a new commit responding to @tannergooding's latest comments. Since the following suggested changes show a significant +10% perf improvement, I will propagate these changes globally to get end-to-end perf results as soon as possible today:

After all changes, including changing scalar intrinsics to indexed code:

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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us

After partial changes (removing the 2nd time of Math.DivRem):

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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us

Before the change:

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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us

@ahsonkhan

ahsonkhan commented Sep 7, 2018

Copy link
Copy Markdown

we may not want to increment/decrement any pointer for bound checking

Right.

Since the following suggested changes show a significant +10% perf improvement

Nice! The implementation can be reasoned about more easily now as well.
+ We removed the risks related to overflow :)

Brian Lui
Respond to PR feedback: Replaced scalar operations by indexed code fo…
f26f112
…r all AVX intrinsics, except MatMul's and those involving AbsMask
ahsonkhan
ahsonkhan reviewed Sep 8, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
}

for (int i = 0; i < remainder - 4; i++)
for (int i = 0; i < remainder % 4; i++)

@ahsonkhanahsonkhanSep 8, 2018•
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I wouldn't use the relatively expensive modulo operator here.

Maybe, do remainder -= 4 in the above if (remainder >= 4) block and then your for loop can just be:

for(inti=0;i<remainder;i++){
...}

@briancylui

briancylui commented Sep 8, 2018

Copy link
Copy Markdown
Author

The latest commit contains the global replacement of scalar operations by indexed code for almost all AVX intrinsics (except for MatMul's and those methods that involve AbsMask). Changes to SSE intrinsics have not been done yet. The latest perf results are shown at the bottom of briancylui#1. Please note that now it is ready to do end-to-end perf testing using the KMeansAndLogisticRegression benchmark in test\Microsoft.ML.Benchmarks, but it is only that I didn't have enough time to test it by the end of my internship.

Thank you everyone for your feedback! I will keep my branches in my fork up there, and feel free to continue developing using my commits. Have a nice day!

@markusweimer

markusweimer commented Sep 21, 2018

Copy link
Copy Markdown

This PR does not reference an issue. Can you please file one and reference it in the PR description?

@briancylui

briancylui commented Sep 21, 2018

Copy link
Copy Markdown
Author

@ahsonkhan raised the issue and suggested the change. Anyone familiar with the issue is welcome to carry on this project after my internship if time allows.

@briancyluibriancylui mentioned this pull request Sep 21, 2018
Open
@briancylui

briancylui commented Sep 21, 2018

Copy link
Copy Markdown
Author

Created an issue #980, which this PR now references. The issue may need some polishing, but it solves the problem for the time being. Hope it helps other people interested take on this project!

@danmoseley

danmoseley commented Sep 24, 2018

Copy link
Copy Markdown

Seems this is almost done. @tannergooding will finish it up in a little bit.

@Zruty0

Zruty0 commented Oct 5, 2018

Copy link
Copy Markdown
Contributor

@tannergooding , are you working on this?

@tannergooding

tannergooding commented Oct 5, 2018

Copy link
Copy Markdown
Member

It's on my backlog, but lower priority than a few other items right now.

@shauheen

shauheen commented Oct 19, 2018

Copy link
Copy Markdown
Contributor

@tannergooding should we close the PR and open it when you think you can update the branch?

@tannergooding

tannergooding commented Oct 19, 2018

Copy link
Copy Markdown
Member

That is fine with me.

@eerhardteerhardt mentioned this pull request Mar 26, 2019
Closed
@ghostghost locked as resolved and limited conversation to collaborators Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tannergoodingtannergoodingtannergooding left review comments
+1 more reviewer
@ahsonkhanahsonkhanahsonkhan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tannergoodingtannergooding

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@briancylui@eerhardt@ahsonkhan@GrabYourPitchforks@markusweimer@danmoseley@Zruty0@tannergooding@shauheen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Change bound checking in SSE/AVX intrinsics to avoid pointer overflow - #821
Closed
briancylui wants to merge 3 commits into
dotnet:masterfrom 
briancylui:SecureBoundChecks
Closed
Change bound checking in SSE/AVX intrinsics to avoid pointer overflow#821
briancylui wants to merge 3 commits into
dotnet:masterfrom 
briancylui:SecureBoundChecks
Conversation
@briancylui
@briancyluibriancylui
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Aims to solve #980
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:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU159.4 us1.104 us0.9784 us
NativePerformanceTestsSumU283.5 us5.492 us4.8687 us
SsePerformanceTestsSumU281.2 us1.472 us1.3045 us
AvxPerformanceTestsAddU276.1 us3.018 us2.520 us
NativePerformanceTestsAddU330.1 us3.585 us3.178 us
SsePerformanceTestsAddU325.6 us6.883 us7.926 us
After the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU183.5 us3.621 us3.023 us
NativePerformanceTestsSumU281.6 us5.261 us4.921 us
SsePerformanceTestsSumU294.1 us2.080 us1.946 us
AvxPerformanceTestsAddU296.3 us5.185 us4.850 us
NativePerformanceTestsAddU335.1 us3.053 us2.707 us
SsePerformanceTestsAddU345.0 us2.155 us1.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 briancylui#2):
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
@eerhardt
eerhardt
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Member
I’m not sure I follow the reasoning. These are pointer operations. So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?

I also agree with the point that we are checking the array/span length above these methods, so we are guaranteed to be within the bounds of memory.
@briancylui
briancylui
commented
Sep 5, 2018

Copy link


Copy Markdown

Author
@ahsonkhan: I may share the same concern as @eerhardt - would love to learn and hear back.
@ahsonkhan
ahsonkhan
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Another question I have is: would pDstCurrent + 8 OR 4 ever have the possibility to result in integer overflow
Imo, given these are public APIs that anyone can call (with potentially invalid inputs), the inputs should be verified within the method body. Being explicit about assertions like src length == dst length would be good (and also act as self-documentation).
Edit: Nevermind, the class is internal.
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.
I am not too familiar with the code base here, but don't contract.asserts run in debug mode only? Are these checks unnecessary in release?
Both SSE and AVX implementations are slower by 10-20% after this change.
How about something like the following:
inti=0;for(;i<src.Length-8;i+=8){Vector256<float>srcVector=Avx.LoadVector256(pSrcCurrent);Vector256<float>dstVector=Avx.LoadVector256(pDstCurrent);result256=Avx.Add(result256,Avx.Multiply(srcVector,dstVector));pSrcCurrent+=8;pDstCurrent+=8;}if(src.Length-i<=4){
...i+=4;}while(i<src.Length){
...i++;}
The number of assembly instruction is essentially identical here (saves a lea, costs an extra add): https://www.diffchecker.com/MywBeoFH (before in red, after in green)
image
Just my two cents. I would leave it up to others who have more context in this space to validate.
Would the OS ever let us get that close?
No, it won't. I discussed this with @GrabYourPitchforks, and its a general coding guideline to avoid arithmetic overflows like this (on the, albeit unlikely, chance the OS behavior changes in the future).
@briancylui
briancylui
commented
Sep 5, 2018

Copy link


Copy Markdown

Author
@ahsonkhan: Thank you for your comments! My apologies that I may have given the wrong hint that the Sse/AvxIntrinsics class is public during our previous conversation - it's actually internal after checking. Thank you very much for pointing that out!
Regarding Contracts.Assert being run in Debug only, it may be a very good point for future follow-up. If the APIs currently do not do any length checking in Release, probably we should do that in the future.
Thank you for the link to the DiffChecker - it looks amazing! Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?
@ahsonkhan
ahsonkhan
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?
That has a similar concern (but with underflow). If src.Length < 8, and in the unlikely chance that the array starts at a memory address close to the beginning of the address space, then (pCurrent <= pEnd - 8) would be true, even though it shouldn't. That is because comparisons are done as if they are unsigned integers.
https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/unsafe-code-pointers/pointer-comparison
The comparison operators compare the addresses of the two operands as if they are unsigned integers.
For example:
// Assume:src.Length=6;pCurrent=4;pEnd=pCurrent+src.Length// 4 + 6 * 4 = 28;if(pCurrent<=pEnd-8){// 4 <= 28 - (8 * 4)// 4 <= 28 - 32// 4 <= (ulong)-4// We don't expect to be here, but we will since -4 as a ulong is a really large number.}
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 6, 2018

Copy link


Copy Markdown

Member
So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?
@ahsonkhan and I spoke about this at length yesterday.
With current operating systems, no. But I don't know what OSes we'll be running on in 10 years. Maybe we'll have a fully managed OS like Singularity, and maybe it'll give us access to the full range of addressable memory. I can't predict the future, and I don't want to chance having to chase down logic errors in this code in ten years' time if there's a theoretical overflow condition that we can identify and address now.
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 6, 2018•

edited

Loading

Copy link


Copy Markdown

Member
FWIW, there is an exception that both C# and C allow when comparing pointers. The element just past the end of the array is always addressable, and it's always guaranteed to compare greater than the address of any element in the array.
That is:
T*ptr; // = pointer to first element in arraysize_tnumElements; // total number of elements in the arrayfor (size_ti=0; i<numElements; i++) {
assert(&ptr[i] <&ptr[numElements], "This is guaranteed by the language.");
}
A corollary to this is that T* end = ptr + numElements is guaranteed not to integer overflow (so the array can't be placed at the very, very end of addressable memory), but T* end = ptr + numElements + 1 has no such guarantee.
Note: the element just past the end of the array is addressable but not necessarily dereferenceable. That is, T element = ptr[numElements] still has undefined behavior, such as populating the register with garbage or even AVing.
@eerhardt
eerhardt
commented
Sep 6, 2018•

edited

Loading

Copy link


Copy Markdown

Member
Ah, ok. I did some more reading, looked at this code again and I understand the concern. The array may be at the very end of memory space. And when pDstCurrent is less than 8 elements from the end, then while (pDstCurrent + 8 <= pDstEnd) will go out of bounds, and potentially overflow. And if it overflows, it will wrap around making condition true.
I think it makes sense using the for (; i < src.Length - 8; i += 8) pattern that @ahsonkhan proposes above or the remainder pattern suggested by @tannergooding in an earlier PR review.
@briancylui
briancylui
commented
Sep 6, 2018

Copy link


Copy Markdown

Author
Thanks for all the comments. After some reading, I agree that the problem is potential illegal memory access due to incrementing the pCurrent pointer out of bounds of the Span<float> object. Accessing out-of-bound object can already cause security concerns, and it would be even worse if the pointer got incremented into illegal memory. I should probably change the title of this PR to "avoid pointer overflow" or "avoid illegal memory access" to avoid any possible confusion, and look forward to implementing this fix tomorrow.
A very good takeaway from @ahsonkhan and @GrabYourPitchforks's comments is that we may not want to increment/decrement any pointer for bound checking, and I think it is also a very good opportunity to follow through on @eerhardt and @tannergooding's suggestion, which will pave the way for implementing double-computing. I will spend some time tomorrow to look into how to implement these fixes concretely.
Thank you very much for all the comments!
@briancyluibriancylui
changed the title
Change bound checking in SSE/AVX intrinsics to avoid integer overflowChange bound checking in SSE/AVX intrinsics to avoid pointer overflowSep 6, 2018
@briancylui
briancylui
commented
Sep 7, 2018

Copy link


Copy Markdown

Author
Pushed a new commit and deleted the previous commit to show you all a sample of the changes I will make globally.
In the new commit, only the AddScalarU intrinsic has been changed in its SSE and AVX implementations. Once you think that the changes look fine to you, or you may some suggestions to edit them, the changes can be made on a global scale to the rest of the intrinsics.
The performance test results are shown below:
After the change:
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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU142.9 us2.8338 us7.0045 us140.1 us
NativePerformanceTestsAddScalarU191.2 us2.3916 us1.8672 us190.9 us
SsePerformanceTestsAddScalarU172.7 us0.7552 us0.5896 us172.6 us
Before the change:
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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU141.8 us1.623 us1.439 us141.5 us
NativePerformanceTestsAddScalarU191.8 us4.514 us10.901 us186.5 us
SsePerformanceTestsAddScalarU180.3 us3.653 us7.125 us177.1 us
The performance results are comparable after the change: SEE got faster by a bit, while AVX got slower by just a bit. The second use of Math.DivRem for countSse and remainderSse may have made the AVX implementation a bit slower.
Look forward to all your comments. Since tomorrow is the last day of my internship, and I have other work items to finish before I go, I don't think I can finish this PR by tomorrow, but I will keep my branch in my fork up so that other contributors can use your previous commits. While I will leave this PR open for the moment, please feel free to close it whenever you feel appropriate. Thank you.
@briancylui
briancylui
commented
Sep 7, 2018

Copy link


Copy Markdown

Author
test MachineLearning-CI please
tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
Outdated

 private static readonly Vector256<float> _absMask256 = Avx.StaticCast<int, float>(Avx.SetAllVector256(0x7FFFFFFF));

 // The count of 32-bit floats in Vector256<T>
 private const int AvxAlignment = 8;
@tannergoodingtannergoodingSep 7, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This isn't alignment, but rather the number of 32-bit elements that Vector256 can hold...
Maybe Vector256SingleElementCount or something similar...
@briancyluibriancyluiSep 7, 2018

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

What about Vector256FloatCount? Is there any preference for SingleElement?
tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
Outdated

tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/SseIntrinsics.cs
Outdated

while (pDstCurrent < pDstEnd)
for (int i = 0; i < remainder; i++)
 {
 Vector128<float> dstVector = Sse.LoadScalarVector128(pDstCurrent);
@tannergoodingtannergoodingSep 7, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This would be way more readable as pDstCurrent[i] += scalar, and it should produce the same code.
The various scalar intrinsics are really meant for places where you have to interop between Vector and Scalar code, or where you need some scalar operations which aren't expressible in normal C# code.
@briancyluibriancyluiSep 7, 2018

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

Yes, perf test results show that this change improves the runtime significantly:
After all changes, including changing scalar intrinsics to indexed code:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us
After partial changes (removing the 2nd time of Math.DivRem):
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us
Before the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us
@briancylui
briancylui
commented
Sep 7, 2018•

edited

Loading

Copy link


Copy Markdown

Author
Pushed a new commit responding to @tannergooding's latest comments. Since the following suggested changes show a significant +10% perf improvement, I will propagate these changes globally to get end-to-end perf results as soon as possible today:
After all changes, including changing scalar intrinsics to indexed code:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us
After partial changes (removing the 2nd time of Math.DivRem):
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us
Before the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us
@ahsonkhan
ahsonkhan
commented
Sep 7, 2018

Copy link


Copy Markdown

we may not want to increment/decrement any pointer for bound checking
Right.
Since the following suggested changes show a significant +10% perf improvement
Nice! The implementation can be reasoned about more easily now as well.

+ We removed the risks related to overflow :)
Brian Lui
Respond to PR feedback: Replaced scalar operations by indexed code fo…
f26f112
…r all AVX intrinsics, except MatMul's and those involving AbsMask
ahsonkhan
ahsonkhan
reviewed
Sep 8, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
 }

 for (int i = 0; i < remainder - 4; i++)
 for (int i = 0; i < remainder % 4; i++)
@ahsonkhanahsonkhanSep 8, 2018•

edited

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I wouldn't use the relatively expensive modulo operator here.
Maybe, do remainder -= 4 in the above if (remainder >= 4) block and then your for loop can just be:
for(inti=0;i<remainder;i++){
...}
@briancylui
briancylui
commented
Sep 8, 2018

Copy link


Copy Markdown

Author
The latest commit contains the global replacement of scalar operations by indexed code for almost all AVX intrinsics (except for MatMul's and those methods that involve AbsMask). Changes to SSE intrinsics have not been done yet. The latest perf results are shown at the bottom of briancylui#1. Please note that now it is ready to do end-to-end perf testing using the KMeansAndLogisticRegression benchmark in test\Microsoft.ML.Benchmarks, but it is only that I didn't have enough time to test it by the end of my internship.
Thank you everyone for your feedback! I will keep my branches in my fork up there, and feel free to continue developing using my commits. Have a nice day!
@markusweimer
markusweimer
commented
Sep 21, 2018

Copy link


Copy Markdown

This PR does not reference an issue. Can you please file one and reference it in the PR description?
@briancylui
briancylui
commented
Sep 21, 2018

Copy link


Copy Markdown

Author
@ahsonkhan raised the issue and suggested the change. Anyone familiar with the issue is welcome to carry on this project after my internship if time allows.
@briancyluibriancylui
mentioned this pull request
Sep 21, 2018

Open

@briancylui
briancylui
commented
Sep 21, 2018

Copy link


Copy Markdown

Author
Created an issue #980, which this PR now references. The issue may need some polishing, but it solves the problem for the time being. Hope it helps other people interested take on this project!
@danmoseley
danmoseley
commented
Sep 24, 2018

Copy link


Copy Markdown

Seems this is almost done. @tannergooding will finish it up in a little bit.
@Zruty0
Zruty0
commented
Oct 5, 2018

Copy link


Copy Markdown

Contributor
@tannergooding , are you working on this?
@tannergooding
tannergooding
commented
Oct 5, 2018

Copy link


Copy Markdown

Member
It's on my backlog, but lower priority than a few other items right now.
@shauheen
shauheen
commented
Oct 19, 2018

Copy link


Copy Markdown

Contributor
@tannergooding should we close the PR and open it when you think you can update the branch?
@tannergooding
tannergooding
commented
Oct 19, 2018

Copy link


Copy Markdown

Member
That is fine with me.
@eerhardteerhardt
mentioned this pull request
Mar 26, 2019
 Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding left review comments

+1 more reviewer

@ahsonkhanahsonkhanahsonkhan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tannergoodingtannergooding

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

9 participants

@briancylui@eerhardt@ahsonkhan@GrabYourPitchforks@markusweimer@danmoseley@Zruty0@tannergooding@shauheen

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Change bound checking in SSE/AVX intrinsics to avoid pointer overflow - #821
Closed
briancylui wants to merge 3 commits into
dotnet:masterfrom 
briancylui:SecureBoundChecks
Closed
Change bound checking in SSE/AVX intrinsics to avoid pointer overflow#821
briancylui wants to merge 3 commits into
dotnet:masterfrom 
briancylui:SecureBoundChecks
Conversation
@briancylui
@briancyluibriancylui
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Aims to solve #980
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:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU159.4 us1.104 us0.9784 us
NativePerformanceTestsSumU283.5 us5.492 us4.8687 us
SsePerformanceTestsSumU281.2 us1.472 us1.3045 us
AvxPerformanceTestsAddU276.1 us3.018 us2.520 us
NativePerformanceTestsAddU330.1 us3.585 us3.178 us
SsePerformanceTestsAddU325.6 us6.883 us7.926 us
After the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU183.5 us3.621 us3.023 us
NativePerformanceTestsSumU281.6 us5.261 us4.921 us
SsePerformanceTestsSumU294.1 us2.080 us1.946 us
AvxPerformanceTestsAddU296.3 us5.185 us4.850 us
NativePerformanceTestsAddU335.1 us3.053 us2.707 us
SsePerformanceTestsAddU345.0 us2.155 us1.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 briancylui#2):
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
@eerhardt
eerhardt
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Member
I’m not sure I follow the reasoning. These are pointer operations. So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?

I also agree with the point that we are checking the array/span length above these methods, so we are guaranteed to be within the bounds of memory.
@briancylui
briancylui
commented
Sep 5, 2018

Copy link


Copy Markdown

Author
@ahsonkhan: I may share the same concern as @eerhardt - would love to learn and hear back.
@ahsonkhan
ahsonkhan
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Another question I have is: would pDstCurrent + 8 OR 4 ever have the possibility to result in integer overflow
Imo, given these are public APIs that anyone can call (with potentially invalid inputs), the inputs should be verified within the method body. Being explicit about assertions like src length == dst length would be good (and also act as self-documentation).
Edit: Nevermind, the class is internal.
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.
I am not too familiar with the code base here, but don't contract.asserts run in debug mode only? Are these checks unnecessary in release?
Both SSE and AVX implementations are slower by 10-20% after this change.
How about something like the following:
inti=0;for(;i<src.Length-8;i+=8){Vector256<float>srcVector=Avx.LoadVector256(pSrcCurrent);Vector256<float>dstVector=Avx.LoadVector256(pDstCurrent);result256=Avx.Add(result256,Avx.Multiply(srcVector,dstVector));pSrcCurrent+=8;pDstCurrent+=8;}if(src.Length-i<=4){
...i+=4;}while(i<src.Length){
...i++;}
The number of assembly instruction is essentially identical here (saves a lea, costs an extra add): https://www.diffchecker.com/MywBeoFH (before in red, after in green)
image
Just my two cents. I would leave it up to others who have more context in this space to validate.
Would the OS ever let us get that close?
No, it won't. I discussed this with @GrabYourPitchforks, and its a general coding guideline to avoid arithmetic overflows like this (on the, albeit unlikely, chance the OS behavior changes in the future).
@briancylui
briancylui
commented
Sep 5, 2018

Copy link


Copy Markdown

Author
@ahsonkhan: Thank you for your comments! My apologies that I may have given the wrong hint that the Sse/AvxIntrinsics class is public during our previous conversation - it's actually internal after checking. Thank you very much for pointing that out!
Regarding Contracts.Assert being run in Debug only, it may be a very good point for future follow-up. If the APIs currently do not do any length checking in Release, probably we should do that in the future.
Thank you for the link to the DiffChecker - it looks amazing! Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?
@ahsonkhan
ahsonkhan
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?
That has a similar concern (but with underflow). If src.Length < 8, and in the unlikely chance that the array starts at a memory address close to the beginning of the address space, then (pCurrent <= pEnd - 8) would be true, even though it shouldn't. That is because comparisons are done as if they are unsigned integers.
https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/unsafe-code-pointers/pointer-comparison
The comparison operators compare the addresses of the two operands as if they are unsigned integers.
For example:
// Assume:src.Length=6;pCurrent=4;pEnd=pCurrent+src.Length// 4 + 6 * 4 = 28;if(pCurrent<=pEnd-8){// 4 <= 28 - (8 * 4)// 4 <= 28 - 32// 4 <= (ulong)-4// We don't expect to be here, but we will since -4 as a ulong is a really large number.}
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 6, 2018

Copy link


Copy Markdown

Member
So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?
@ahsonkhan and I spoke about this at length yesterday.
With current operating systems, no. But I don't know what OSes we'll be running on in 10 years. Maybe we'll have a fully managed OS like Singularity, and maybe it'll give us access to the full range of addressable memory. I can't predict the future, and I don't want to chance having to chase down logic errors in this code in ten years' time if there's a theoretical overflow condition that we can identify and address now.
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 6, 2018•

edited

Loading

Copy link


Copy Markdown

Member
FWIW, there is an exception that both C# and C allow when comparing pointers. The element just past the end of the array is always addressable, and it's always guaranteed to compare greater than the address of any element in the array.
That is:
T*ptr; // = pointer to first element in arraysize_tnumElements; // total number of elements in the arrayfor (size_ti=0; i<numElements; i++) {
assert(&ptr[i] <&ptr[numElements], "This is guaranteed by the language.");
}
A corollary to this is that T* end = ptr + numElements is guaranteed not to integer overflow (so the array can't be placed at the very, very end of addressable memory), but T* end = ptr + numElements + 1 has no such guarantee.
Note: the element just past the end of the array is addressable but not necessarily dereferenceable. That is, T element = ptr[numElements] still has undefined behavior, such as populating the register with garbage or even AVing.
@eerhardt
eerhardt
commented
Sep 6, 2018•

edited

Loading

Copy link


Copy Markdown

Member
Ah, ok. I did some more reading, looked at this code again and I understand the concern. The array may be at the very end of memory space. And when pDstCurrent is less than 8 elements from the end, then while (pDstCurrent + 8 <= pDstEnd) will go out of bounds, and potentially overflow. And if it overflows, it will wrap around making condition true.
I think it makes sense using the for (; i < src.Length - 8; i += 8) pattern that @ahsonkhan proposes above or the remainder pattern suggested by @tannergooding in an earlier PR review.
@briancylui
briancylui
commented
Sep 6, 2018

Copy link


Copy Markdown

Author
Thanks for all the comments. After some reading, I agree that the problem is potential illegal memory access due to incrementing the pCurrent pointer out of bounds of the Span<float> object. Accessing out-of-bound object can already cause security concerns, and it would be even worse if the pointer got incremented into illegal memory. I should probably change the title of this PR to "avoid pointer overflow" or "avoid illegal memory access" to avoid any possible confusion, and look forward to implementing this fix tomorrow.
A very good takeaway from @ahsonkhan and @GrabYourPitchforks's comments is that we may not want to increment/decrement any pointer for bound checking, and I think it is also a very good opportunity to follow through on @eerhardt and @tannergooding's suggestion, which will pave the way for implementing double-computing. I will spend some time tomorrow to look into how to implement these fixes concretely.
Thank you very much for all the comments!
@briancyluibriancylui
changed the title
Change bound checking in SSE/AVX intrinsics to avoid integer overflowChange bound checking in SSE/AVX intrinsics to avoid pointer overflowSep 6, 2018
@briancylui
briancylui
commented
Sep 7, 2018

Copy link


Copy Markdown

Author
Pushed a new commit and deleted the previous commit to show you all a sample of the changes I will make globally.
In the new commit, only the AddScalarU intrinsic has been changed in its SSE and AVX implementations. Once you think that the changes look fine to you, or you may some suggestions to edit them, the changes can be made on a global scale to the rest of the intrinsics.
The performance test results are shown below:
After the change:
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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU142.9 us2.8338 us7.0045 us140.1 us
NativePerformanceTestsAddScalarU191.2 us2.3916 us1.8672 us190.9 us
SsePerformanceTestsAddScalarU172.7 us0.7552 us0.5896 us172.6 us
Before the change:
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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU141.8 us1.623 us1.439 us141.5 us
NativePerformanceTestsAddScalarU191.8 us4.514 us10.901 us186.5 us
SsePerformanceTestsAddScalarU180.3 us3.653 us7.125 us177.1 us
The performance results are comparable after the change: SEE got faster by a bit, while AVX got slower by just a bit. The second use of Math.DivRem for countSse and remainderSse may have made the AVX implementation a bit slower.
Look forward to all your comments. Since tomorrow is the last day of my internship, and I have other work items to finish before I go, I don't think I can finish this PR by tomorrow, but I will keep my branch in my fork up so that other contributors can use your previous commits. While I will leave this PR open for the moment, please feel free to close it whenever you feel appropriate. Thank you.
@briancylui
briancylui
commented
Sep 7, 2018

Copy link


Copy Markdown

Author
test MachineLearning-CI please
tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
Outdated

 private static readonly Vector256<float> _absMask256 = Avx.StaticCast<int, float>(Avx.SetAllVector256(0x7FFFFFFF));

 // The count of 32-bit floats in Vector256<T>
 private const int AvxAlignment = 8;
@tannergoodingtannergoodingSep 7, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This isn't alignment, but rather the number of 32-bit elements that Vector256 can hold...
Maybe Vector256SingleElementCount or something similar...
@briancyluibriancyluiSep 7, 2018

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

What about Vector256FloatCount? Is there any preference for SingleElement?
tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
Outdated

tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/SseIntrinsics.cs
Outdated

while (pDstCurrent < pDstEnd)
for (int i = 0; i < remainder; i++)
 {
 Vector128<float> dstVector = Sse.LoadScalarVector128(pDstCurrent);
@tannergoodingtannergoodingSep 7, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This would be way more readable as pDstCurrent[i] += scalar, and it should produce the same code.
The various scalar intrinsics are really meant for places where you have to interop between Vector and Scalar code, or where you need some scalar operations which aren't expressible in normal C# code.
@briancyluibriancyluiSep 7, 2018

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

Yes, perf test results show that this change improves the runtime significantly:
After all changes, including changing scalar intrinsics to indexed code:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us
After partial changes (removing the 2nd time of Math.DivRem):
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us
Before the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us
@briancylui
briancylui
commented
Sep 7, 2018•

edited

Loading

Copy link


Copy Markdown

Author
Pushed a new commit responding to @tannergooding's latest comments. Since the following suggested changes show a significant +10% perf improvement, I will propagate these changes globally to get end-to-end perf results as soon as possible today:
After all changes, including changing scalar intrinsics to indexed code:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us
After partial changes (removing the 2nd time of Math.DivRem):
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us
Before the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us
@ahsonkhan
ahsonkhan
commented
Sep 7, 2018

Copy link


Copy Markdown

we may not want to increment/decrement any pointer for bound checking
Right.
Since the following suggested changes show a significant +10% perf improvement
Nice! The implementation can be reasoned about more easily now as well.

+ We removed the risks related to overflow :)
Brian Lui
Respond to PR feedback: Replaced scalar operations by indexed code fo…
f26f112
…r all AVX intrinsics, except MatMul's and those involving AbsMask
ahsonkhan
ahsonkhan
reviewed
Sep 8, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
 }

 for (int i = 0; i < remainder - 4; i++)
 for (int i = 0; i < remainder % 4; i++)
@ahsonkhanahsonkhanSep 8, 2018•

edited

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I wouldn't use the relatively expensive modulo operator here.
Maybe, do remainder -= 4 in the above if (remainder >= 4) block and then your for loop can just be:
for(inti=0;i<remainder;i++){
...}
@briancylui
briancylui
commented
Sep 8, 2018

Copy link


Copy Markdown

Author
The latest commit contains the global replacement of scalar operations by indexed code for almost all AVX intrinsics (except for MatMul's and those methods that involve AbsMask). Changes to SSE intrinsics have not been done yet. The latest perf results are shown at the bottom of briancylui#1. Please note that now it is ready to do end-to-end perf testing using the KMeansAndLogisticRegression benchmark in test\Microsoft.ML.Benchmarks, but it is only that I didn't have enough time to test it by the end of my internship.
Thank you everyone for your feedback! I will keep my branches in my fork up there, and feel free to continue developing using my commits. Have a nice day!
@markusweimer
markusweimer
commented
Sep 21, 2018

Copy link


Copy Markdown

This PR does not reference an issue. Can you please file one and reference it in the PR description?
@briancylui
briancylui
commented
Sep 21, 2018

Copy link


Copy Markdown

Author
@ahsonkhan raised the issue and suggested the change. Anyone familiar with the issue is welcome to carry on this project after my internship if time allows.
@briancyluibriancylui
mentioned this pull request
Sep 21, 2018

Open

@briancylui
briancylui
commented
Sep 21, 2018

Copy link


Copy Markdown

Author
Created an issue #980, which this PR now references. The issue may need some polishing, but it solves the problem for the time being. Hope it helps other people interested take on this project!
@danmoseley
danmoseley
commented
Sep 24, 2018

Copy link


Copy Markdown

Seems this is almost done. @tannergooding will finish it up in a little bit.
@Zruty0
Zruty0
commented
Oct 5, 2018

Copy link


Copy Markdown

Contributor
@tannergooding , are you working on this?
@tannergooding
tannergooding
commented
Oct 5, 2018

Copy link


Copy Markdown

Member
It's on my backlog, but lower priority than a few other items right now.
@shauheen
shauheen
commented
Oct 19, 2018

Copy link


Copy Markdown

Contributor
@tannergooding should we close the PR and open it when you think you can update the branch?
@tannergooding
tannergooding
commented
Oct 19, 2018

Copy link


Copy Markdown

Member
That is fine with me.
@eerhardteerhardt
mentioned this pull request
Mar 26, 2019
 Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding left review comments

+1 more reviewer

@ahsonkhanahsonkhanahsonkhan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tannergoodingtannergooding

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

9 participants

@briancylui@eerhardt@ahsonkhan@GrabYourPitchforks@markusweimer@danmoseley@Zruty0@tannergooding@shauheen

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Change bound checking in SSE/AVX intrinsics to avoid pointer overflow - #821
Closed
briancylui wants to merge 3 commits into
dotnet:masterfrom 
briancylui:SecureBoundChecks
Closed
Change bound checking in SSE/AVX intrinsics to avoid pointer overflow#821
briancylui wants to merge 3 commits into
dotnet:masterfrom 
briancylui:SecureBoundChecks
Conversation
@briancylui
@briancyluibriancylui
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Aims to solve #980
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:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU159.4 us1.104 us0.9784 us
NativePerformanceTestsSumU283.5 us5.492 us4.8687 us
SsePerformanceTestsSumU281.2 us1.472 us1.3045 us
AvxPerformanceTestsAddU276.1 us3.018 us2.520 us
NativePerformanceTestsAddU330.1 us3.585 us3.178 us
SsePerformanceTestsAddU325.6 us6.883 us7.926 us
After the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU183.5 us3.621 us3.023 us
NativePerformanceTestsSumU281.6 us5.261 us4.921 us
SsePerformanceTestsSumU294.1 us2.080 us1.946 us
AvxPerformanceTestsAddU296.3 us5.185 us4.850 us
NativePerformanceTestsAddU335.1 us3.053 us2.707 us
SsePerformanceTestsAddU345.0 us2.155 us1.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 briancylui#2):
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
@eerhardt
eerhardt
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Member
I’m not sure I follow the reasoning. These are pointer operations. So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?

I also agree with the point that we are checking the array/span length above these methods, so we are guaranteed to be within the bounds of memory.
@briancylui
briancylui
commented
Sep 5, 2018

Copy link


Copy Markdown

Author
@ahsonkhan: I may share the same concern as @eerhardt - would love to learn and hear back.
@ahsonkhan
ahsonkhan
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Another question I have is: would pDstCurrent + 8 OR 4 ever have the possibility to result in integer overflow
Imo, given these are public APIs that anyone can call (with potentially invalid inputs), the inputs should be verified within the method body. Being explicit about assertions like src length == dst length would be good (and also act as self-documentation).
Edit: Nevermind, the class is internal.
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.
I am not too familiar with the code base here, but don't contract.asserts run in debug mode only? Are these checks unnecessary in release?
Both SSE and AVX implementations are slower by 10-20% after this change.
How about something like the following:
inti=0;for(;i<src.Length-8;i+=8){Vector256<float>srcVector=Avx.LoadVector256(pSrcCurrent);Vector256<float>dstVector=Avx.LoadVector256(pDstCurrent);result256=Avx.Add(result256,Avx.Multiply(srcVector,dstVector));pSrcCurrent+=8;pDstCurrent+=8;}if(src.Length-i<=4){
...i+=4;}while(i<src.Length){
...i++;}
The number of assembly instruction is essentially identical here (saves a lea, costs an extra add): https://www.diffchecker.com/MywBeoFH (before in red, after in green)
image
Just my two cents. I would leave it up to others who have more context in this space to validate.
Would the OS ever let us get that close?
No, it won't. I discussed this with @GrabYourPitchforks, and its a general coding guideline to avoid arithmetic overflows like this (on the, albeit unlikely, chance the OS behavior changes in the future).
@briancylui
briancylui
commented
Sep 5, 2018

Copy link


Copy Markdown

Author
@ahsonkhan: Thank you for your comments! My apologies that I may have given the wrong hint that the Sse/AvxIntrinsics class is public during our previous conversation - it's actually internal after checking. Thank you very much for pointing that out!
Regarding Contracts.Assert being run in Debug only, it may be a very good point for future follow-up. If the APIs currently do not do any length checking in Release, probably we should do that in the future.
Thank you for the link to the DiffChecker - it looks amazing! Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?
@ahsonkhan
ahsonkhan
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?
That has a similar concern (but with underflow). If src.Length < 8, and in the unlikely chance that the array starts at a memory address close to the beginning of the address space, then (pCurrent <= pEnd - 8) would be true, even though it shouldn't. That is because comparisons are done as if they are unsigned integers.
https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/unsafe-code-pointers/pointer-comparison
The comparison operators compare the addresses of the two operands as if they are unsigned integers.
For example:
// Assume:src.Length=6;pCurrent=4;pEnd=pCurrent+src.Length// 4 + 6 * 4 = 28;if(pCurrent<=pEnd-8){// 4 <= 28 - (8 * 4)// 4 <= 28 - 32// 4 <= (ulong)-4// We don't expect to be here, but we will since -4 as a ulong is a really large number.}
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 6, 2018

Copy link


Copy Markdown

Member
So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?
@ahsonkhan and I spoke about this at length yesterday.
With current operating systems, no. But I don't know what OSes we'll be running on in 10 years. Maybe we'll have a fully managed OS like Singularity, and maybe it'll give us access to the full range of addressable memory. I can't predict the future, and I don't want to chance having to chase down logic errors in this code in ten years' time if there's a theoretical overflow condition that we can identify and address now.
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 6, 2018•

edited

Loading

Copy link


Copy Markdown

Member
FWIW, there is an exception that both C# and C allow when comparing pointers. The element just past the end of the array is always addressable, and it's always guaranteed to compare greater than the address of any element in the array.
That is:
T*ptr; // = pointer to first element in arraysize_tnumElements; // total number of elements in the arrayfor (size_ti=0; i<numElements; i++) {
assert(&ptr[i] <&ptr[numElements], "This is guaranteed by the language.");
}
A corollary to this is that T* end = ptr + numElements is guaranteed not to integer overflow (so the array can't be placed at the very, very end of addressable memory), but T* end = ptr + numElements + 1 has no such guarantee.
Note: the element just past the end of the array is addressable but not necessarily dereferenceable. That is, T element = ptr[numElements] still has undefined behavior, such as populating the register with garbage or even AVing.
@eerhardt
eerhardt
commented
Sep 6, 2018•

edited

Loading

Copy link


Copy Markdown

Member
Ah, ok. I did some more reading, looked at this code again and I understand the concern. The array may be at the very end of memory space. And when pDstCurrent is less than 8 elements from the end, then while (pDstCurrent + 8 <= pDstEnd) will go out of bounds, and potentially overflow. And if it overflows, it will wrap around making condition true.
I think it makes sense using the for (; i < src.Length - 8; i += 8) pattern that @ahsonkhan proposes above or the remainder pattern suggested by @tannergooding in an earlier PR review.
@briancylui
briancylui
commented
Sep 6, 2018

Copy link


Copy Markdown

Author
Thanks for all the comments. After some reading, I agree that the problem is potential illegal memory access due to incrementing the pCurrent pointer out of bounds of the Span<float> object. Accessing out-of-bound object can already cause security concerns, and it would be even worse if the pointer got incremented into illegal memory. I should probably change the title of this PR to "avoid pointer overflow" or "avoid illegal memory access" to avoid any possible confusion, and look forward to implementing this fix tomorrow.
A very good takeaway from @ahsonkhan and @GrabYourPitchforks's comments is that we may not want to increment/decrement any pointer for bound checking, and I think it is also a very good opportunity to follow through on @eerhardt and @tannergooding's suggestion, which will pave the way for implementing double-computing. I will spend some time tomorrow to look into how to implement these fixes concretely.
Thank you very much for all the comments!
@briancyluibriancylui
changed the title
Change bound checking in SSE/AVX intrinsics to avoid integer overflowChange bound checking in SSE/AVX intrinsics to avoid pointer overflowSep 6, 2018
@briancylui
briancylui
commented
Sep 7, 2018

Copy link


Copy Markdown

Author
Pushed a new commit and deleted the previous commit to show you all a sample of the changes I will make globally.
In the new commit, only the AddScalarU intrinsic has been changed in its SSE and AVX implementations. Once you think that the changes look fine to you, or you may some suggestions to edit them, the changes can be made on a global scale to the rest of the intrinsics.
The performance test results are shown below:
After the change:
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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU142.9 us2.8338 us7.0045 us140.1 us
NativePerformanceTestsAddScalarU191.2 us2.3916 us1.8672 us190.9 us
SsePerformanceTestsAddScalarU172.7 us0.7552 us0.5896 us172.6 us
Before the change:
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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU141.8 us1.623 us1.439 us141.5 us
NativePerformanceTestsAddScalarU191.8 us4.514 us10.901 us186.5 us
SsePerformanceTestsAddScalarU180.3 us3.653 us7.125 us177.1 us
The performance results are comparable after the change: SEE got faster by a bit, while AVX got slower by just a bit. The second use of Math.DivRem for countSse and remainderSse may have made the AVX implementation a bit slower.
Look forward to all your comments. Since tomorrow is the last day of my internship, and I have other work items to finish before I go, I don't think I can finish this PR by tomorrow, but I will keep my branch in my fork up so that other contributors can use your previous commits. While I will leave this PR open for the moment, please feel free to close it whenever you feel appropriate. Thank you.
@briancylui
briancylui
commented
Sep 7, 2018

Copy link


Copy Markdown

Author
test MachineLearning-CI please
tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
Outdated

 private static readonly Vector256<float> _absMask256 = Avx.StaticCast<int, float>(Avx.SetAllVector256(0x7FFFFFFF));

 // The count of 32-bit floats in Vector256<T>
 private const int AvxAlignment = 8;
@tannergoodingtannergoodingSep 7, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This isn't alignment, but rather the number of 32-bit elements that Vector256 can hold...
Maybe Vector256SingleElementCount or something similar...
@briancyluibriancyluiSep 7, 2018

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

What about Vector256FloatCount? Is there any preference for SingleElement?
tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
Outdated

tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/SseIntrinsics.cs
Outdated

while (pDstCurrent < pDstEnd)
for (int i = 0; i < remainder; i++)
 {
 Vector128<float> dstVector = Sse.LoadScalarVector128(pDstCurrent);
@tannergoodingtannergoodingSep 7, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This would be way more readable as pDstCurrent[i] += scalar, and it should produce the same code.
The various scalar intrinsics are really meant for places where you have to interop between Vector and Scalar code, or where you need some scalar operations which aren't expressible in normal C# code.
@briancyluibriancyluiSep 7, 2018

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

Yes, perf test results show that this change improves the runtime significantly:
After all changes, including changing scalar intrinsics to indexed code:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us
After partial changes (removing the 2nd time of Math.DivRem):
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us
Before the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us
@briancylui
briancylui
commented
Sep 7, 2018•

edited

Loading

Copy link


Copy Markdown

Author
Pushed a new commit responding to @tannergooding's latest comments. Since the following suggested changes show a significant +10% perf improvement, I will propagate these changes globally to get end-to-end perf results as soon as possible today:
After all changes, including changing scalar intrinsics to indexed code:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us
After partial changes (removing the 2nd time of Math.DivRem):
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us
Before the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us
@ahsonkhan
ahsonkhan
commented
Sep 7, 2018

Copy link


Copy Markdown

we may not want to increment/decrement any pointer for bound checking
Right.
Since the following suggested changes show a significant +10% perf improvement
Nice! The implementation can be reasoned about more easily now as well.

+ We removed the risks related to overflow :)
Brian Lui
Respond to PR feedback: Replaced scalar operations by indexed code fo…
f26f112
…r all AVX intrinsics, except MatMul's and those involving AbsMask
ahsonkhan
ahsonkhan
reviewed
Sep 8, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
 }

 for (int i = 0; i < remainder - 4; i++)
 for (int i = 0; i < remainder % 4; i++)
@ahsonkhanahsonkhanSep 8, 2018•

edited

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I wouldn't use the relatively expensive modulo operator here.
Maybe, do remainder -= 4 in the above if (remainder >= 4) block and then your for loop can just be:
for(inti=0;i<remainder;i++){
...}
@briancylui
briancylui
commented
Sep 8, 2018

Copy link


Copy Markdown

Author
The latest commit contains the global replacement of scalar operations by indexed code for almost all AVX intrinsics (except for MatMul's and those methods that involve AbsMask). Changes to SSE intrinsics have not been done yet. The latest perf results are shown at the bottom of briancylui#1. Please note that now it is ready to do end-to-end perf testing using the KMeansAndLogisticRegression benchmark in test\Microsoft.ML.Benchmarks, but it is only that I didn't have enough time to test it by the end of my internship.
Thank you everyone for your feedback! I will keep my branches in my fork up there, and feel free to continue developing using my commits. Have a nice day!
@markusweimer
markusweimer
commented
Sep 21, 2018

Copy link


Copy Markdown

This PR does not reference an issue. Can you please file one and reference it in the PR description?
@briancylui
briancylui
commented
Sep 21, 2018

Copy link


Copy Markdown

Author
@ahsonkhan raised the issue and suggested the change. Anyone familiar with the issue is welcome to carry on this project after my internship if time allows.
@briancyluibriancylui
mentioned this pull request
Sep 21, 2018

Open

@briancylui
briancylui
commented
Sep 21, 2018

Copy link


Copy Markdown

Author
Created an issue #980, which this PR now references. The issue may need some polishing, but it solves the problem for the time being. Hope it helps other people interested take on this project!
@danmoseley
danmoseley
commented
Sep 24, 2018

Copy link


Copy Markdown

Seems this is almost done. @tannergooding will finish it up in a little bit.
@Zruty0
Zruty0
commented
Oct 5, 2018

Copy link


Copy Markdown

Contributor
@tannergooding , are you working on this?
@tannergooding
tannergooding
commented
Oct 5, 2018

Copy link


Copy Markdown

Member
It's on my backlog, but lower priority than a few other items right now.
@shauheen
shauheen
commented
Oct 19, 2018

Copy link


Copy Markdown

Contributor
@tannergooding should we close the PR and open it when you think you can update the branch?
@tannergooding
tannergooding
commented
Oct 19, 2018

Copy link


Copy Markdown

Member
That is fine with me.
@eerhardteerhardt
mentioned this pull request
Mar 26, 2019
 Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding left review comments

+1 more reviewer

@ahsonkhanahsonkhanahsonkhan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tannergoodingtannergooding

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

9 participants

@briancylui@eerhardt@ahsonkhan@GrabYourPitchforks@markusweimer@danmoseley@Zruty0@tannergooding@shauheen

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Change bound checking in SSE/AVX intrinsics to avoid pointer overflow - #821
Closed
briancylui wants to merge 3 commits into
dotnet:masterfrom 
briancylui:SecureBoundChecks
Closed
Change bound checking in SSE/AVX intrinsics to avoid pointer overflow#821
briancylui wants to merge 3 commits into
dotnet:masterfrom 
briancylui:SecureBoundChecks
Conversation
@briancylui
@briancyluibriancylui
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Aims to solve #980
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:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU159.4 us1.104 us0.9784 us
NativePerformanceTestsSumU283.5 us5.492 us4.8687 us
SsePerformanceTestsSumU281.2 us1.472 us1.3045 us
AvxPerformanceTestsAddU276.1 us3.018 us2.520 us
NativePerformanceTestsAddU330.1 us3.585 us3.178 us
SsePerformanceTestsAddU325.6 us6.883 us7.926 us
After the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU183.5 us3.621 us3.023 us
NativePerformanceTestsSumU281.6 us5.261 us4.921 us
SsePerformanceTestsSumU294.1 us2.080 us1.946 us
AvxPerformanceTestsAddU296.3 us5.185 us4.850 us
NativePerformanceTestsAddU335.1 us3.053 us2.707 us
SsePerformanceTestsAddU345.0 us2.155 us1.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 briancylui#2):
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
@eerhardt
eerhardt
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Member
I’m not sure I follow the reasoning. These are pointer operations. So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?

I also agree with the point that we are checking the array/span length above these methods, so we are guaranteed to be within the bounds of memory.
@briancylui
briancylui
commented
Sep 5, 2018

Copy link


Copy Markdown

Author
@ahsonkhan: I may share the same concern as @eerhardt - would love to learn and hear back.
@ahsonkhan
ahsonkhan
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Another question I have is: would pDstCurrent + 8 OR 4 ever have the possibility to result in integer overflow
Imo, given these are public APIs that anyone can call (with potentially invalid inputs), the inputs should be verified within the method body. Being explicit about assertions like src length == dst length would be good (and also act as self-documentation).
Edit: Nevermind, the class is internal.
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.
I am not too familiar with the code base here, but don't contract.asserts run in debug mode only? Are these checks unnecessary in release?
Both SSE and AVX implementations are slower by 10-20% after this change.
How about something like the following:
inti=0;for(;i<src.Length-8;i+=8){Vector256<float>srcVector=Avx.LoadVector256(pSrcCurrent);Vector256<float>dstVector=Avx.LoadVector256(pDstCurrent);result256=Avx.Add(result256,Avx.Multiply(srcVector,dstVector));pSrcCurrent+=8;pDstCurrent+=8;}if(src.Length-i<=4){
...i+=4;}while(i<src.Length){
...i++;}
The number of assembly instruction is essentially identical here (saves a lea, costs an extra add): https://www.diffchecker.com/MywBeoFH (before in red, after in green)
image
Just my two cents. I would leave it up to others who have more context in this space to validate.
Would the OS ever let us get that close?
No, it won't. I discussed this with @GrabYourPitchforks, and its a general coding guideline to avoid arithmetic overflows like this (on the, albeit unlikely, chance the OS behavior changes in the future).
@briancylui
briancylui
commented
Sep 5, 2018

Copy link


Copy Markdown

Author
@ahsonkhan: Thank you for your comments! My apologies that I may have given the wrong hint that the Sse/AvxIntrinsics class is public during our previous conversation - it's actually internal after checking. Thank you very much for pointing that out!
Regarding Contracts.Assert being run in Debug only, it may be a very good point for future follow-up. If the APIs currently do not do any length checking in Release, probably we should do that in the future.
Thank you for the link to the DiffChecker - it looks amazing! Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?
@ahsonkhan
ahsonkhan
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?
That has a similar concern (but with underflow). If src.Length < 8, and in the unlikely chance that the array starts at a memory address close to the beginning of the address space, then (pCurrent <= pEnd - 8) would be true, even though it shouldn't. That is because comparisons are done as if they are unsigned integers.
https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/unsafe-code-pointers/pointer-comparison
The comparison operators compare the addresses of the two operands as if they are unsigned integers.
For example:
// Assume:src.Length=6;pCurrent=4;pEnd=pCurrent+src.Length// 4 + 6 * 4 = 28;if(pCurrent<=pEnd-8){// 4 <= 28 - (8 * 4)// 4 <= 28 - 32// 4 <= (ulong)-4// We don't expect to be here, but we will since -4 as a ulong is a really large number.}
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 6, 2018

Copy link


Copy Markdown

Member
So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?
@ahsonkhan and I spoke about this at length yesterday.
With current operating systems, no. But I don't know what OSes we'll be running on in 10 years. Maybe we'll have a fully managed OS like Singularity, and maybe it'll give us access to the full range of addressable memory. I can't predict the future, and I don't want to chance having to chase down logic errors in this code in ten years' time if there's a theoretical overflow condition that we can identify and address now.
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 6, 2018•

edited

Loading

Copy link


Copy Markdown

Member
FWIW, there is an exception that both C# and C allow when comparing pointers. The element just past the end of the array is always addressable, and it's always guaranteed to compare greater than the address of any element in the array.
That is:
T*ptr; // = pointer to first element in arraysize_tnumElements; // total number of elements in the arrayfor (size_ti=0; i<numElements; i++) {
assert(&ptr[i] <&ptr[numElements], "This is guaranteed by the language.");
}
A corollary to this is that T* end = ptr + numElements is guaranteed not to integer overflow (so the array can't be placed at the very, very end of addressable memory), but T* end = ptr + numElements + 1 has no such guarantee.
Note: the element just past the end of the array is addressable but not necessarily dereferenceable. That is, T element = ptr[numElements] still has undefined behavior, such as populating the register with garbage or even AVing.
@eerhardt
eerhardt
commented
Sep 6, 2018•

edited

Loading

Copy link


Copy Markdown

Member
Ah, ok. I did some more reading, looked at this code again and I understand the concern. The array may be at the very end of memory space. And when pDstCurrent is less than 8 elements from the end, then while (pDstCurrent + 8 <= pDstEnd) will go out of bounds, and potentially overflow. And if it overflows, it will wrap around making condition true.
I think it makes sense using the for (; i < src.Length - 8; i += 8) pattern that @ahsonkhan proposes above or the remainder pattern suggested by @tannergooding in an earlier PR review.
@briancylui
briancylui
commented
Sep 6, 2018

Copy link


Copy Markdown

Author
Thanks for all the comments. After some reading, I agree that the problem is potential illegal memory access due to incrementing the pCurrent pointer out of bounds of the Span<float> object. Accessing out-of-bound object can already cause security concerns, and it would be even worse if the pointer got incremented into illegal memory. I should probably change the title of this PR to "avoid pointer overflow" or "avoid illegal memory access" to avoid any possible confusion, and look forward to implementing this fix tomorrow.
A very good takeaway from @ahsonkhan and @GrabYourPitchforks's comments is that we may not want to increment/decrement any pointer for bound checking, and I think it is also a very good opportunity to follow through on @eerhardt and @tannergooding's suggestion, which will pave the way for implementing double-computing. I will spend some time tomorrow to look into how to implement these fixes concretely.
Thank you very much for all the comments!
@briancyluibriancylui
changed the title
Change bound checking in SSE/AVX intrinsics to avoid integer overflowChange bound checking in SSE/AVX intrinsics to avoid pointer overflowSep 6, 2018
@briancylui
briancylui
commented
Sep 7, 2018

Copy link


Copy Markdown

Author
Pushed a new commit and deleted the previous commit to show you all a sample of the changes I will make globally.
In the new commit, only the AddScalarU intrinsic has been changed in its SSE and AVX implementations. Once you think that the changes look fine to you, or you may some suggestions to edit them, the changes can be made on a global scale to the rest of the intrinsics.
The performance test results are shown below:
After the change:
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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU142.9 us2.8338 us7.0045 us140.1 us
NativePerformanceTestsAddScalarU191.2 us2.3916 us1.8672 us190.9 us
SsePerformanceTestsAddScalarU172.7 us0.7552 us0.5896 us172.6 us
Before the change:
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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU141.8 us1.623 us1.439 us141.5 us
NativePerformanceTestsAddScalarU191.8 us4.514 us10.901 us186.5 us
SsePerformanceTestsAddScalarU180.3 us3.653 us7.125 us177.1 us
The performance results are comparable after the change: SEE got faster by a bit, while AVX got slower by just a bit. The second use of Math.DivRem for countSse and remainderSse may have made the AVX implementation a bit slower.
Look forward to all your comments. Since tomorrow is the last day of my internship, and I have other work items to finish before I go, I don't think I can finish this PR by tomorrow, but I will keep my branch in my fork up so that other contributors can use your previous commits. While I will leave this PR open for the moment, please feel free to close it whenever you feel appropriate. Thank you.
@briancylui
briancylui
commented
Sep 7, 2018

Copy link


Copy Markdown

Author
test MachineLearning-CI please
tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
Outdated

 private static readonly Vector256<float> _absMask256 = Avx.StaticCast<int, float>(Avx.SetAllVector256(0x7FFFFFFF));

 // The count of 32-bit floats in Vector256<T>
 private const int AvxAlignment = 8;
@tannergoodingtannergoodingSep 7, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This isn't alignment, but rather the number of 32-bit elements that Vector256 can hold...
Maybe Vector256SingleElementCount or something similar...
@briancyluibriancyluiSep 7, 2018

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

What about Vector256FloatCount? Is there any preference for SingleElement?
tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
Outdated

tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/SseIntrinsics.cs
Outdated

while (pDstCurrent < pDstEnd)
for (int i = 0; i < remainder; i++)
 {
 Vector128<float> dstVector = Sse.LoadScalarVector128(pDstCurrent);
@tannergoodingtannergoodingSep 7, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This would be way more readable as pDstCurrent[i] += scalar, and it should produce the same code.
The various scalar intrinsics are really meant for places where you have to interop between Vector and Scalar code, or where you need some scalar operations which aren't expressible in normal C# code.
@briancyluibriancyluiSep 7, 2018

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

Yes, perf test results show that this change improves the runtime significantly:
After all changes, including changing scalar intrinsics to indexed code:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us
After partial changes (removing the 2nd time of Math.DivRem):
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us
Before the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us
@briancylui
briancylui
commented
Sep 7, 2018•

edited

Loading

Copy link


Copy Markdown

Author
Pushed a new commit responding to @tannergooding's latest comments. Since the following suggested changes show a significant +10% perf improvement, I will propagate these changes globally to get end-to-end perf results as soon as possible today:
After all changes, including changing scalar intrinsics to indexed code:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us
After partial changes (removing the 2nd time of Math.DivRem):
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us
Before the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us
@ahsonkhan
ahsonkhan
commented
Sep 7, 2018

Copy link


Copy Markdown

we may not want to increment/decrement any pointer for bound checking
Right.
Since the following suggested changes show a significant +10% perf improvement
Nice! The implementation can be reasoned about more easily now as well.

+ We removed the risks related to overflow :)
Brian Lui
Respond to PR feedback: Replaced scalar operations by indexed code fo…
f26f112
…r all AVX intrinsics, except MatMul's and those involving AbsMask
ahsonkhan
ahsonkhan
reviewed
Sep 8, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
 }

 for (int i = 0; i < remainder - 4; i++)
 for (int i = 0; i < remainder % 4; i++)
@ahsonkhanahsonkhanSep 8, 2018•

edited

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I wouldn't use the relatively expensive modulo operator here.
Maybe, do remainder -= 4 in the above if (remainder >= 4) block and then your for loop can just be:
for(inti=0;i<remainder;i++){
...}
@briancylui
briancylui
commented
Sep 8, 2018

Copy link


Copy Markdown

Author
The latest commit contains the global replacement of scalar operations by indexed code for almost all AVX intrinsics (except for MatMul's and those methods that involve AbsMask). Changes to SSE intrinsics have not been done yet. The latest perf results are shown at the bottom of briancylui#1. Please note that now it is ready to do end-to-end perf testing using the KMeansAndLogisticRegression benchmark in test\Microsoft.ML.Benchmarks, but it is only that I didn't have enough time to test it by the end of my internship.
Thank you everyone for your feedback! I will keep my branches in my fork up there, and feel free to continue developing using my commits. Have a nice day!
@markusweimer
markusweimer
commented
Sep 21, 2018

Copy link


Copy Markdown

This PR does not reference an issue. Can you please file one and reference it in the PR description?
@briancylui
briancylui
commented
Sep 21, 2018

Copy link


Copy Markdown

Author
@ahsonkhan raised the issue and suggested the change. Anyone familiar with the issue is welcome to carry on this project after my internship if time allows.
@briancyluibriancylui
mentioned this pull request
Sep 21, 2018

Open

@briancylui
briancylui
commented
Sep 21, 2018

Copy link


Copy Markdown

Author
Created an issue #980, which this PR now references. The issue may need some polishing, but it solves the problem for the time being. Hope it helps other people interested take on this project!
@danmoseley
danmoseley
commented
Sep 24, 2018

Copy link


Copy Markdown

Seems this is almost done. @tannergooding will finish it up in a little bit.
@Zruty0
Zruty0
commented
Oct 5, 2018

Copy link


Copy Markdown

Contributor
@tannergooding , are you working on this?
@tannergooding
tannergooding
commented
Oct 5, 2018

Copy link


Copy Markdown

Member
It's on my backlog, but lower priority than a few other items right now.
@shauheen
shauheen
commented
Oct 19, 2018

Copy link


Copy Markdown

Contributor
@tannergooding should we close the PR and open it when you think you can update the branch?
@tannergooding
tannergooding
commented
Oct 19, 2018

Copy link


Copy Markdown

Member
That is fine with me.
@eerhardteerhardt
mentioned this pull request
Mar 26, 2019
 Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding left review comments

+1 more reviewer

@ahsonkhanahsonkhanahsonkhan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tannergoodingtannergooding

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

9 participants

@briancylui@eerhardt@ahsonkhan@GrabYourPitchforks@markusweimer@danmoseley@Zruty0@tannergooding@shauheen

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Change bound checking in SSE/AVX intrinsics to avoid pointer overflow - #821
Closed
briancylui wants to merge 3 commits into
dotnet:masterfrom 
briancylui:SecureBoundChecks
Closed
Change bound checking in SSE/AVX intrinsics to avoid pointer overflow#821
briancylui wants to merge 3 commits into
dotnet:masterfrom 
briancylui:SecureBoundChecks
Conversation
@briancylui
@briancyluibriancylui
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Aims to solve #980
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:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU159.4 us1.104 us0.9784 us
NativePerformanceTestsSumU283.5 us5.492 us4.8687 us
SsePerformanceTestsSumU281.2 us1.472 us1.3045 us
AvxPerformanceTestsAddU276.1 us3.018 us2.520 us
NativePerformanceTestsAddU330.1 us3.585 us3.178 us
SsePerformanceTestsAddU325.6 us6.883 us7.926 us
After the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU183.5 us3.621 us3.023 us
NativePerformanceTestsSumU281.6 us5.261 us4.921 us
SsePerformanceTestsSumU294.1 us2.080 us1.946 us
AvxPerformanceTestsAddU296.3 us5.185 us4.850 us
NativePerformanceTestsAddU335.1 us3.053 us2.707 us
SsePerformanceTestsAddU345.0 us2.155 us1.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 briancylui#2):
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
@eerhardt
eerhardt
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Member
I’m not sure I follow the reasoning. These are pointer operations. So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?

I also agree with the point that we are checking the array/span length above these methods, so we are guaranteed to be within the bounds of memory.
@briancylui
briancylui
commented
Sep 5, 2018

Copy link


Copy Markdown

Author
@ahsonkhan: I may share the same concern as @eerhardt - would love to learn and hear back.
@ahsonkhan
ahsonkhan
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Another question I have is: would pDstCurrent + 8 OR 4 ever have the possibility to result in integer overflow
Imo, given these are public APIs that anyone can call (with potentially invalid inputs), the inputs should be verified within the method body. Being explicit about assertions like src length == dst length would be good (and also act as self-documentation).
Edit: Nevermind, the class is internal.
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.
I am not too familiar with the code base here, but don't contract.asserts run in debug mode only? Are these checks unnecessary in release?
Both SSE and AVX implementations are slower by 10-20% after this change.
How about something like the following:
inti=0;for(;i<src.Length-8;i+=8){Vector256<float>srcVector=Avx.LoadVector256(pSrcCurrent);Vector256<float>dstVector=Avx.LoadVector256(pDstCurrent);result256=Avx.Add(result256,Avx.Multiply(srcVector,dstVector));pSrcCurrent+=8;pDstCurrent+=8;}if(src.Length-i<=4){
...i+=4;}while(i<src.Length){
...i++;}
The number of assembly instruction is essentially identical here (saves a lea, costs an extra add): https://www.diffchecker.com/MywBeoFH (before in red, after in green)
image
Just my two cents. I would leave it up to others who have more context in this space to validate.
Would the OS ever let us get that close?
No, it won't. I discussed this with @GrabYourPitchforks, and its a general coding guideline to avoid arithmetic overflows like this (on the, albeit unlikely, chance the OS behavior changes in the future).
@briancylui
briancylui
commented
Sep 5, 2018

Copy link


Copy Markdown

Author
@ahsonkhan: Thank you for your comments! My apologies that I may have given the wrong hint that the Sse/AvxIntrinsics class is public during our previous conversation - it's actually internal after checking. Thank you very much for pointing that out!
Regarding Contracts.Assert being run in Debug only, it may be a very good point for future follow-up. If the APIs currently do not do any length checking in Release, probably we should do that in the future.
Thank you for the link to the DiffChecker - it looks amazing! Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?
@ahsonkhan
ahsonkhan
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?
That has a similar concern (but with underflow). If src.Length < 8, and in the unlikely chance that the array starts at a memory address close to the beginning of the address space, then (pCurrent <= pEnd - 8) would be true, even though it shouldn't. That is because comparisons are done as if they are unsigned integers.
https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/unsafe-code-pointers/pointer-comparison
The comparison operators compare the addresses of the two operands as if they are unsigned integers.
For example:
// Assume:src.Length=6;pCurrent=4;pEnd=pCurrent+src.Length// 4 + 6 * 4 = 28;if(pCurrent<=pEnd-8){// 4 <= 28 - (8 * 4)// 4 <= 28 - 32// 4 <= (ulong)-4// We don't expect to be here, but we will since -4 as a ulong is a really large number.}
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 6, 2018

Copy link


Copy Markdown

Member
So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?
@ahsonkhan and I spoke about this at length yesterday.
With current operating systems, no. But I don't know what OSes we'll be running on in 10 years. Maybe we'll have a fully managed OS like Singularity, and maybe it'll give us access to the full range of addressable memory. I can't predict the future, and I don't want to chance having to chase down logic errors in this code in ten years' time if there's a theoretical overflow condition that we can identify and address now.
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 6, 2018•

edited

Loading

Copy link


Copy Markdown

Member
FWIW, there is an exception that both C# and C allow when comparing pointers. The element just past the end of the array is always addressable, and it's always guaranteed to compare greater than the address of any element in the array.
That is:
T*ptr; // = pointer to first element in arraysize_tnumElements; // total number of elements in the arrayfor (size_ti=0; i<numElements; i++) {
assert(&ptr[i] <&ptr[numElements], "This is guaranteed by the language.");
}
A corollary to this is that T* end = ptr + numElements is guaranteed not to integer overflow (so the array can't be placed at the very, very end of addressable memory), but T* end = ptr + numElements + 1 has no such guarantee.
Note: the element just past the end of the array is addressable but not necessarily dereferenceable. That is, T element = ptr[numElements] still has undefined behavior, such as populating the register with garbage or even AVing.
@eerhardt
eerhardt
commented
Sep 6, 2018•

edited

Loading

Copy link


Copy Markdown

Member
Ah, ok. I did some more reading, looked at this code again and I understand the concern. The array may be at the very end of memory space. And when pDstCurrent is less than 8 elements from the end, then while (pDstCurrent + 8 <= pDstEnd) will go out of bounds, and potentially overflow. And if it overflows, it will wrap around making condition true.
I think it makes sense using the for (; i < src.Length - 8; i += 8) pattern that @ahsonkhan proposes above or the remainder pattern suggested by @tannergooding in an earlier PR review.
@briancylui
briancylui
commented
Sep 6, 2018

Copy link


Copy Markdown

Author
Thanks for all the comments. After some reading, I agree that the problem is potential illegal memory access due to incrementing the pCurrent pointer out of bounds of the Span<float> object. Accessing out-of-bound object can already cause security concerns, and it would be even worse if the pointer got incremented into illegal memory. I should probably change the title of this PR to "avoid pointer overflow" or "avoid illegal memory access" to avoid any possible confusion, and look forward to implementing this fix tomorrow.
A very good takeaway from @ahsonkhan and @GrabYourPitchforks's comments is that we may not want to increment/decrement any pointer for bound checking, and I think it is also a very good opportunity to follow through on @eerhardt and @tannergooding's suggestion, which will pave the way for implementing double-computing. I will spend some time tomorrow to look into how to implement these fixes concretely.
Thank you very much for all the comments!
@briancyluibriancylui
changed the title
Change bound checking in SSE/AVX intrinsics to avoid integer overflowChange bound checking in SSE/AVX intrinsics to avoid pointer overflowSep 6, 2018
@briancylui
briancylui
commented
Sep 7, 2018

Copy link


Copy Markdown

Author
Pushed a new commit and deleted the previous commit to show you all a sample of the changes I will make globally.
In the new commit, only the AddScalarU intrinsic has been changed in its SSE and AVX implementations. Once you think that the changes look fine to you, or you may some suggestions to edit them, the changes can be made on a global scale to the rest of the intrinsics.
The performance test results are shown below:
After the change:
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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU142.9 us2.8338 us7.0045 us140.1 us
NativePerformanceTestsAddScalarU191.2 us2.3916 us1.8672 us190.9 us
SsePerformanceTestsAddScalarU172.7 us0.7552 us0.5896 us172.6 us
Before the change:
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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU141.8 us1.623 us1.439 us141.5 us
NativePerformanceTestsAddScalarU191.8 us4.514 us10.901 us186.5 us
SsePerformanceTestsAddScalarU180.3 us3.653 us7.125 us177.1 us
The performance results are comparable after the change: SEE got faster by a bit, while AVX got slower by just a bit. The second use of Math.DivRem for countSse and remainderSse may have made the AVX implementation a bit slower.
Look forward to all your comments. Since tomorrow is the last day of my internship, and I have other work items to finish before I go, I don't think I can finish this PR by tomorrow, but I will keep my branch in my fork up so that other contributors can use your previous commits. While I will leave this PR open for the moment, please feel free to close it whenever you feel appropriate. Thank you.
@briancylui
briancylui
commented
Sep 7, 2018

Copy link


Copy Markdown

Author
test MachineLearning-CI please
tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
Outdated

 private static readonly Vector256<float> _absMask256 = Avx.StaticCast<int, float>(Avx.SetAllVector256(0x7FFFFFFF));

 // The count of 32-bit floats in Vector256<T>
 private const int AvxAlignment = 8;
@tannergoodingtannergoodingSep 7, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This isn't alignment, but rather the number of 32-bit elements that Vector256 can hold...
Maybe Vector256SingleElementCount or something similar...
@briancyluibriancyluiSep 7, 2018

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

What about Vector256FloatCount? Is there any preference for SingleElement?
tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
Outdated

tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/SseIntrinsics.cs
Outdated

while (pDstCurrent < pDstEnd)
for (int i = 0; i < remainder; i++)
 {
 Vector128<float> dstVector = Sse.LoadScalarVector128(pDstCurrent);
@tannergoodingtannergoodingSep 7, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This would be way more readable as pDstCurrent[i] += scalar, and it should produce the same code.
The various scalar intrinsics are really meant for places where you have to interop between Vector and Scalar code, or where you need some scalar operations which aren't expressible in normal C# code.
@briancyluibriancyluiSep 7, 2018

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

Yes, perf test results show that this change improves the runtime significantly:
After all changes, including changing scalar intrinsics to indexed code:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us
After partial changes (removing the 2nd time of Math.DivRem):
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us
Before the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us
@briancylui
briancylui
commented
Sep 7, 2018•

edited

Loading

Copy link


Copy Markdown

Author
Pushed a new commit responding to @tannergooding's latest comments. Since the following suggested changes show a significant +10% perf improvement, I will propagate these changes globally to get end-to-end perf results as soon as possible today:
After all changes, including changing scalar intrinsics to indexed code:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us
After partial changes (removing the 2nd time of Math.DivRem):
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us
Before the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us
@ahsonkhan
ahsonkhan
commented
Sep 7, 2018

Copy link


Copy Markdown

we may not want to increment/decrement any pointer for bound checking
Right.
Since the following suggested changes show a significant +10% perf improvement
Nice! The implementation can be reasoned about more easily now as well.

+ We removed the risks related to overflow :)
Brian Lui
Respond to PR feedback: Replaced scalar operations by indexed code fo…
f26f112
…r all AVX intrinsics, except MatMul's and those involving AbsMask
ahsonkhan
ahsonkhan
reviewed
Sep 8, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
 }

 for (int i = 0; i < remainder - 4; i++)
 for (int i = 0; i < remainder % 4; i++)
@ahsonkhanahsonkhanSep 8, 2018•

edited

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I wouldn't use the relatively expensive modulo operator here.
Maybe, do remainder -= 4 in the above if (remainder >= 4) block and then your for loop can just be:
for(inti=0;i<remainder;i++){
...}
@briancylui
briancylui
commented
Sep 8, 2018

Copy link


Copy Markdown

Author
The latest commit contains the global replacement of scalar operations by indexed code for almost all AVX intrinsics (except for MatMul's and those methods that involve AbsMask). Changes to SSE intrinsics have not been done yet. The latest perf results are shown at the bottom of briancylui#1. Please note that now it is ready to do end-to-end perf testing using the KMeansAndLogisticRegression benchmark in test\Microsoft.ML.Benchmarks, but it is only that I didn't have enough time to test it by the end of my internship.
Thank you everyone for your feedback! I will keep my branches in my fork up there, and feel free to continue developing using my commits. Have a nice day!
@markusweimer
markusweimer
commented
Sep 21, 2018

Copy link


Copy Markdown

This PR does not reference an issue. Can you please file one and reference it in the PR description?
@briancylui
briancylui
commented
Sep 21, 2018

Copy link


Copy Markdown

Author
@ahsonkhan raised the issue and suggested the change. Anyone familiar with the issue is welcome to carry on this project after my internship if time allows.
@briancyluibriancylui
mentioned this pull request
Sep 21, 2018

Open

@briancylui
briancylui
commented
Sep 21, 2018

Copy link


Copy Markdown

Author
Created an issue #980, which this PR now references. The issue may need some polishing, but it solves the problem for the time being. Hope it helps other people interested take on this project!
@danmoseley
danmoseley
commented
Sep 24, 2018

Copy link


Copy Markdown

Seems this is almost done. @tannergooding will finish it up in a little bit.
@Zruty0
Zruty0
commented
Oct 5, 2018

Copy link


Copy Markdown

Contributor
@tannergooding , are you working on this?
@tannergooding
tannergooding
commented
Oct 5, 2018

Copy link


Copy Markdown

Member
It's on my backlog, but lower priority than a few other items right now.
@shauheen
shauheen
commented
Oct 19, 2018

Copy link


Copy Markdown

Contributor
@tannergooding should we close the PR and open it when you think you can update the branch?
@tannergooding
tannergooding
commented
Oct 19, 2018

Copy link


Copy Markdown

Member
That is fine with me.
@eerhardteerhardt
mentioned this pull request
Mar 26, 2019
 Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding left review comments

+1 more reviewer

@ahsonkhanahsonkhanahsonkhan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tannergoodingtannergooding

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

9 participants

@briancylui@eerhardt@ahsonkhan@GrabYourPitchforks@markusweimer@danmoseley@Zruty0@tannergooding@shauheen

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Change bound checking in SSE/AVX intrinsics to avoid pointer overflow - #821
Closed
briancylui wants to merge 3 commits into
dotnet:masterfrom 
briancylui:SecureBoundChecks
Closed
Change bound checking in SSE/AVX intrinsics to avoid pointer overflow#821
briancylui wants to merge 3 commits into
dotnet:masterfrom 
briancylui:SecureBoundChecks
Conversation
@briancylui
@briancyluibriancylui
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Aims to solve #980
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:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU159.4 us1.104 us0.9784 us
NativePerformanceTestsSumU283.5 us5.492 us4.8687 us
SsePerformanceTestsSumU281.2 us1.472 us1.3045 us
AvxPerformanceTestsAddU276.1 us3.018 us2.520 us
NativePerformanceTestsAddU330.1 us3.585 us3.178 us
SsePerformanceTestsAddU325.6 us6.883 us7.926 us
After the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU183.5 us3.621 us3.023 us
NativePerformanceTestsSumU281.6 us5.261 us4.921 us
SsePerformanceTestsSumU294.1 us2.080 us1.946 us
AvxPerformanceTestsAddU296.3 us5.185 us4.850 us
NativePerformanceTestsAddU335.1 us3.053 us2.707 us
SsePerformanceTestsAddU345.0 us2.155 us1.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 briancylui#2):
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
@eerhardt
eerhardt
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Member
I’m not sure I follow the reasoning. These are pointer operations. So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?

I also agree with the point that we are checking the array/span length above these methods, so we are guaranteed to be within the bounds of memory.
@briancylui
briancylui
commented
Sep 5, 2018

Copy link


Copy Markdown

Author
@ahsonkhan: I may share the same concern as @eerhardt - would love to learn and hear back.
@ahsonkhan
ahsonkhan
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Another question I have is: would pDstCurrent + 8 OR 4 ever have the possibility to result in integer overflow
Imo, given these are public APIs that anyone can call (with potentially invalid inputs), the inputs should be verified within the method body. Being explicit about assertions like src length == dst length would be good (and also act as self-documentation).
Edit: Nevermind, the class is internal.
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.
I am not too familiar with the code base here, but don't contract.asserts run in debug mode only? Are these checks unnecessary in release?
Both SSE and AVX implementations are slower by 10-20% after this change.
How about something like the following:
inti=0;for(;i<src.Length-8;i+=8){Vector256<float>srcVector=Avx.LoadVector256(pSrcCurrent);Vector256<float>dstVector=Avx.LoadVector256(pDstCurrent);result256=Avx.Add(result256,Avx.Multiply(srcVector,dstVector));pSrcCurrent+=8;pDstCurrent+=8;}if(src.Length-i<=4){
...i+=4;}while(i<src.Length){
...i++;}
The number of assembly instruction is essentially identical here (saves a lea, costs an extra add): https://www.diffchecker.com/MywBeoFH (before in red, after in green)
image
Just my two cents. I would leave it up to others who have more context in this space to validate.
Would the OS ever let us get that close?
No, it won't. I discussed this with @GrabYourPitchforks, and its a general coding guideline to avoid arithmetic overflows like this (on the, albeit unlikely, chance the OS behavior changes in the future).
@briancylui
briancylui
commented
Sep 5, 2018

Copy link


Copy Markdown

Author
@ahsonkhan: Thank you for your comments! My apologies that I may have given the wrong hint that the Sse/AvxIntrinsics class is public during our previous conversation - it's actually internal after checking. Thank you very much for pointing that out!
Regarding Contracts.Assert being run in Debug only, it may be a very good point for future follow-up. If the APIs currently do not do any length checking in Release, probably we should do that in the future.
Thank you for the link to the DiffChecker - it looks amazing! Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?
@ahsonkhan
ahsonkhan
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?
That has a similar concern (but with underflow). If src.Length < 8, and in the unlikely chance that the array starts at a memory address close to the beginning of the address space, then (pCurrent <= pEnd - 8) would be true, even though it shouldn't. That is because comparisons are done as if they are unsigned integers.
https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/unsafe-code-pointers/pointer-comparison
The comparison operators compare the addresses of the two operands as if they are unsigned integers.
For example:
// Assume:src.Length=6;pCurrent=4;pEnd=pCurrent+src.Length// 4 + 6 * 4 = 28;if(pCurrent<=pEnd-8){// 4 <= 28 - (8 * 4)// 4 <= 28 - 32// 4 <= (ulong)-4// We don't expect to be here, but we will since -4 as a ulong is a really large number.}
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 6, 2018

Copy link


Copy Markdown

Member
So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?
@ahsonkhan and I spoke about this at length yesterday.
With current operating systems, no. But I don't know what OSes we'll be running on in 10 years. Maybe we'll have a fully managed OS like Singularity, and maybe it'll give us access to the full range of addressable memory. I can't predict the future, and I don't want to chance having to chase down logic errors in this code in ten years' time if there's a theoretical overflow condition that we can identify and address now.
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 6, 2018•

edited

Loading

Copy link


Copy Markdown

Member
FWIW, there is an exception that both C# and C allow when comparing pointers. The element just past the end of the array is always addressable, and it's always guaranteed to compare greater than the address of any element in the array.
That is:
T*ptr; // = pointer to first element in arraysize_tnumElements; // total number of elements in the arrayfor (size_ti=0; i<numElements; i++) {
assert(&ptr[i] <&ptr[numElements], "This is guaranteed by the language.");
}
A corollary to this is that T* end = ptr + numElements is guaranteed not to integer overflow (so the array can't be placed at the very, very end of addressable memory), but T* end = ptr + numElements + 1 has no such guarantee.
Note: the element just past the end of the array is addressable but not necessarily dereferenceable. That is, T element = ptr[numElements] still has undefined behavior, such as populating the register with garbage or even AVing.
@eerhardt
eerhardt
commented
Sep 6, 2018•

edited

Loading

Copy link


Copy Markdown

Member
Ah, ok. I did some more reading, looked at this code again and I understand the concern. The array may be at the very end of memory space. And when pDstCurrent is less than 8 elements from the end, then while (pDstCurrent + 8 <= pDstEnd) will go out of bounds, and potentially overflow. And if it overflows, it will wrap around making condition true.
I think it makes sense using the for (; i < src.Length - 8; i += 8) pattern that @ahsonkhan proposes above or the remainder pattern suggested by @tannergooding in an earlier PR review.
@briancylui
briancylui
commented
Sep 6, 2018

Copy link


Copy Markdown

Author
Thanks for all the comments. After some reading, I agree that the problem is potential illegal memory access due to incrementing the pCurrent pointer out of bounds of the Span<float> object. Accessing out-of-bound object can already cause security concerns, and it would be even worse if the pointer got incremented into illegal memory. I should probably change the title of this PR to "avoid pointer overflow" or "avoid illegal memory access" to avoid any possible confusion, and look forward to implementing this fix tomorrow.
A very good takeaway from @ahsonkhan and @GrabYourPitchforks's comments is that we may not want to increment/decrement any pointer for bound checking, and I think it is also a very good opportunity to follow through on @eerhardt and @tannergooding's suggestion, which will pave the way for implementing double-computing. I will spend some time tomorrow to look into how to implement these fixes concretely.
Thank you very much for all the comments!
@briancyluibriancylui
changed the title
Change bound checking in SSE/AVX intrinsics to avoid integer overflowChange bound checking in SSE/AVX intrinsics to avoid pointer overflowSep 6, 2018
@briancylui
briancylui
commented
Sep 7, 2018

Copy link


Copy Markdown

Author
Pushed a new commit and deleted the previous commit to show you all a sample of the changes I will make globally.
In the new commit, only the AddScalarU intrinsic has been changed in its SSE and AVX implementations. Once you think that the changes look fine to you, or you may some suggestions to edit them, the changes can be made on a global scale to the rest of the intrinsics.
The performance test results are shown below:
After the change:
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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU142.9 us2.8338 us7.0045 us140.1 us
NativePerformanceTestsAddScalarU191.2 us2.3916 us1.8672 us190.9 us
SsePerformanceTestsAddScalarU172.7 us0.7552 us0.5896 us172.6 us
Before the change:
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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU141.8 us1.623 us1.439 us141.5 us
NativePerformanceTestsAddScalarU191.8 us4.514 us10.901 us186.5 us
SsePerformanceTestsAddScalarU180.3 us3.653 us7.125 us177.1 us
The performance results are comparable after the change: SEE got faster by a bit, while AVX got slower by just a bit. The second use of Math.DivRem for countSse and remainderSse may have made the AVX implementation a bit slower.
Look forward to all your comments. Since tomorrow is the last day of my internship, and I have other work items to finish before I go, I don't think I can finish this PR by tomorrow, but I will keep my branch in my fork up so that other contributors can use your previous commits. While I will leave this PR open for the moment, please feel free to close it whenever you feel appropriate. Thank you.
@briancylui
briancylui
commented
Sep 7, 2018

Copy link


Copy Markdown

Author
test MachineLearning-CI please
tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
Outdated

 private static readonly Vector256<float> _absMask256 = Avx.StaticCast<int, float>(Avx.SetAllVector256(0x7FFFFFFF));

 // The count of 32-bit floats in Vector256<T>
 private const int AvxAlignment = 8;
@tannergoodingtannergoodingSep 7, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This isn't alignment, but rather the number of 32-bit elements that Vector256 can hold...
Maybe Vector256SingleElementCount or something similar...
@briancyluibriancyluiSep 7, 2018

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

What about Vector256FloatCount? Is there any preference for SingleElement?
tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
Outdated

tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/SseIntrinsics.cs
Outdated

while (pDstCurrent < pDstEnd)
for (int i = 0; i < remainder; i++)
 {
 Vector128<float> dstVector = Sse.LoadScalarVector128(pDstCurrent);
@tannergoodingtannergoodingSep 7, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This would be way more readable as pDstCurrent[i] += scalar, and it should produce the same code.
The various scalar intrinsics are really meant for places where you have to interop between Vector and Scalar code, or where you need some scalar operations which aren't expressible in normal C# code.
@briancyluibriancyluiSep 7, 2018

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

Yes, perf test results show that this change improves the runtime significantly:
After all changes, including changing scalar intrinsics to indexed code:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us
After partial changes (removing the 2nd time of Math.DivRem):
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us
Before the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us
@briancylui
briancylui
commented
Sep 7, 2018•

edited

Loading

Copy link


Copy Markdown

Author
Pushed a new commit responding to @tannergooding's latest comments. Since the following suggested changes show a significant +10% perf improvement, I will propagate these changes globally to get end-to-end perf results as soon as possible today:
After all changes, including changing scalar intrinsics to indexed code:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us
After partial changes (removing the 2nd time of Math.DivRem):
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us
Before the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us
@ahsonkhan
ahsonkhan
commented
Sep 7, 2018

Copy link


Copy Markdown

we may not want to increment/decrement any pointer for bound checking
Right.
Since the following suggested changes show a significant +10% perf improvement
Nice! The implementation can be reasoned about more easily now as well.

+ We removed the risks related to overflow :)
Brian Lui
Respond to PR feedback: Replaced scalar operations by indexed code fo…
f26f112
…r all AVX intrinsics, except MatMul's and those involving AbsMask
ahsonkhan
ahsonkhan
reviewed
Sep 8, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
 }

 for (int i = 0; i < remainder - 4; i++)
 for (int i = 0; i < remainder % 4; i++)
@ahsonkhanahsonkhanSep 8, 2018•

edited

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I wouldn't use the relatively expensive modulo operator here.
Maybe, do remainder -= 4 in the above if (remainder >= 4) block and then your for loop can just be:
for(inti=0;i<remainder;i++){
...}
@briancylui
briancylui
commented
Sep 8, 2018

Copy link


Copy Markdown

Author
The latest commit contains the global replacement of scalar operations by indexed code for almost all AVX intrinsics (except for MatMul's and those methods that involve AbsMask). Changes to SSE intrinsics have not been done yet. The latest perf results are shown at the bottom of briancylui#1. Please note that now it is ready to do end-to-end perf testing using the KMeansAndLogisticRegression benchmark in test\Microsoft.ML.Benchmarks, but it is only that I didn't have enough time to test it by the end of my internship.
Thank you everyone for your feedback! I will keep my branches in my fork up there, and feel free to continue developing using my commits. Have a nice day!
@markusweimer
markusweimer
commented
Sep 21, 2018

Copy link


Copy Markdown

This PR does not reference an issue. Can you please file one and reference it in the PR description?
@briancylui
briancylui
commented
Sep 21, 2018

Copy link


Copy Markdown

Author
@ahsonkhan raised the issue and suggested the change. Anyone familiar with the issue is welcome to carry on this project after my internship if time allows.
@briancyluibriancylui
mentioned this pull request
Sep 21, 2018

Open

@briancylui
briancylui
commented
Sep 21, 2018

Copy link


Copy Markdown

Author
Created an issue #980, which this PR now references. The issue may need some polishing, but it solves the problem for the time being. Hope it helps other people interested take on this project!
@danmoseley
danmoseley
commented
Sep 24, 2018

Copy link


Copy Markdown

Seems this is almost done. @tannergooding will finish it up in a little bit.
@Zruty0
Zruty0
commented
Oct 5, 2018

Copy link


Copy Markdown

Contributor
@tannergooding , are you working on this?
@tannergooding
tannergooding
commented
Oct 5, 2018

Copy link


Copy Markdown

Member
It's on my backlog, but lower priority than a few other items right now.
@shauheen
shauheen
commented
Oct 19, 2018

Copy link


Copy Markdown

Contributor
@tannergooding should we close the PR and open it when you think you can update the branch?
@tannergooding
tannergooding
commented
Oct 19, 2018

Copy link


Copy Markdown

Member
That is fine with me.
@eerhardteerhardt
mentioned this pull request
Mar 26, 2019
 Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding left review comments

+1 more reviewer

@ahsonkhanahsonkhanahsonkhan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tannergoodingtannergooding

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

9 participants

@briancylui@eerhardt@ahsonkhan@GrabYourPitchforks@markusweimer@danmoseley@Zruty0@tannergooding@shauheen

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Change bound checking in SSE/AVX intrinsics to avoid pointer overflow - #821
Closed
briancylui wants to merge 3 commits into
dotnet:masterfrom 
briancylui:SecureBoundChecks
Closed
Change bound checking in SSE/AVX intrinsics to avoid pointer overflow#821
briancylui wants to merge 3 commits into
dotnet:masterfrom 
briancylui:SecureBoundChecks
Conversation
@briancylui
@briancyluibriancylui
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Aims to solve #980
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:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU159.4 us1.104 us0.9784 us
NativePerformanceTestsSumU283.5 us5.492 us4.8687 us
SsePerformanceTestsSumU281.2 us1.472 us1.3045 us
AvxPerformanceTestsAddU276.1 us3.018 us2.520 us
NativePerformanceTestsAddU330.1 us3.585 us3.178 us
SsePerformanceTestsAddU325.6 us6.883 us7.926 us
After the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsSumU183.5 us3.621 us3.023 us
NativePerformanceTestsSumU281.6 us5.261 us4.921 us
SsePerformanceTestsSumU294.1 us2.080 us1.946 us
AvxPerformanceTestsAddU296.3 us5.185 us4.850 us
NativePerformanceTestsAddU335.1 us3.053 us2.707 us
SsePerformanceTestsAddU345.0 us2.155 us1.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 briancylui#2):
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
@eerhardt
eerhardt
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Member
I’m not sure I follow the reasoning. These are pointer operations. So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?

I also agree with the point that we are checking the array/span length above these methods, so we are guaranteed to be within the bounds of memory.
@briancylui
briancylui
commented
Sep 5, 2018

Copy link


Copy Markdown

Author
@ahsonkhan: I may share the same concern as @eerhardt - would love to learn and hear back.
@ahsonkhan
ahsonkhan
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Another question I have is: would pDstCurrent + 8 OR 4 ever have the possibility to result in integer overflow
Imo, given these are public APIs that anyone can call (with potentially invalid inputs), the inputs should be verified within the method body. Being explicit about assertions like src length == dst length would be good (and also act as self-documentation).
Edit: Nevermind, the class is internal.
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.
I am not too familiar with the code base here, but don't contract.asserts run in debug mode only? Are these checks unnecessary in release?
Both SSE and AVX implementations are slower by 10-20% after this change.
How about something like the following:
inti=0;for(;i<src.Length-8;i+=8){Vector256<float>srcVector=Avx.LoadVector256(pSrcCurrent);Vector256<float>dstVector=Avx.LoadVector256(pDstCurrent);result256=Avx.Add(result256,Avx.Multiply(srcVector,dstVector));pSrcCurrent+=8;pDstCurrent+=8;}if(src.Length-i<=4){
...i+=4;}while(i<src.Length){
...i++;}
The number of assembly instruction is essentially identical here (saves a lea, costs an extra add): https://www.diffchecker.com/MywBeoFH (before in red, after in green)
image
Just my two cents. I would leave it up to others who have more context in this space to validate.
Would the OS ever let us get that close?
No, it won't. I discussed this with @GrabYourPitchforks, and its a general coding guideline to avoid arithmetic overflows like this (on the, albeit unlikely, chance the OS behavior changes in the future).
@briancylui
briancylui
commented
Sep 5, 2018

Copy link


Copy Markdown

Author
@ahsonkhan: Thank you for your comments! My apologies that I may have given the wrong hint that the Sse/AvxIntrinsics class is public during our previous conversation - it's actually internal after checking. Thank you very much for pointing that out!
Regarding Contracts.Assert being run in Debug only, it may be a very good point for future follow-up. If the APIs currently do not do any length checking in Release, probably we should do that in the future.
Thank you for the link to the DiffChecker - it looks amazing! Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?
@ahsonkhan
ahsonkhan
commented
Sep 5, 2018•

edited

Loading

Copy link


Copy Markdown

Following your logic, would changing while (pCurrent + 8 OR 4 <= pEnd) into while (pCurrent <= pEnd - 8 OR 4) have a similar effect?
That has a similar concern (but with underflow). If src.Length < 8, and in the unlikely chance that the array starts at a memory address close to the beginning of the address space, then (pCurrent <= pEnd - 8) would be true, even though it shouldn't. That is because comparisons are done as if they are unsigned integers.
https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/unsafe-code-pointers/pointer-comparison
The comparison operators compare the addresses of the two operands as if they are unsigned integers.
For example:
// Assume:src.Length=6;pCurrent=4;pEnd=pCurrent+src.Length// 4 + 6 * 4 = 28;if(pCurrent<=pEnd-8){// 4 <= 28 - (8 * 4)// 4 <= 28 - 32// 4 <= (ulong)-4// We don't expect to be here, but we will since -4 as a ulong is a really large number.}
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 6, 2018

Copy link


Copy Markdown

Member
So the concern is that we are less than 4 or 8 elements away from the end of the memory? Would the OS ever let us get that close?
@ahsonkhan and I spoke about this at length yesterday.
With current operating systems, no. But I don't know what OSes we'll be running on in 10 years. Maybe we'll have a fully managed OS like Singularity, and maybe it'll give us access to the full range of addressable memory. I can't predict the future, and I don't want to chance having to chase down logic errors in this code in ten years' time if there's a theoretical overflow condition that we can identify and address now.
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 6, 2018•

edited

Loading

Copy link


Copy Markdown

Member
FWIW, there is an exception that both C# and C allow when comparing pointers. The element just past the end of the array is always addressable, and it's always guaranteed to compare greater than the address of any element in the array.
That is:
T*ptr; // = pointer to first element in arraysize_tnumElements; // total number of elements in the arrayfor (size_ti=0; i<numElements; i++) {
assert(&ptr[i] <&ptr[numElements], "This is guaranteed by the language.");
}
A corollary to this is that T* end = ptr + numElements is guaranteed not to integer overflow (so the array can't be placed at the very, very end of addressable memory), but T* end = ptr + numElements + 1 has no such guarantee.
Note: the element just past the end of the array is addressable but not necessarily dereferenceable. That is, T element = ptr[numElements] still has undefined behavior, such as populating the register with garbage or even AVing.
@eerhardt
eerhardt
commented
Sep 6, 2018•

edited

Loading

Copy link


Copy Markdown

Member
Ah, ok. I did some more reading, looked at this code again and I understand the concern. The array may be at the very end of memory space. And when pDstCurrent is less than 8 elements from the end, then while (pDstCurrent + 8 <= pDstEnd) will go out of bounds, and potentially overflow. And if it overflows, it will wrap around making condition true.
I think it makes sense using the for (; i < src.Length - 8; i += 8) pattern that @ahsonkhan proposes above or the remainder pattern suggested by @tannergooding in an earlier PR review.
@briancylui
briancylui
commented
Sep 6, 2018

Copy link


Copy Markdown

Author
Thanks for all the comments. After some reading, I agree that the problem is potential illegal memory access due to incrementing the pCurrent pointer out of bounds of the Span<float> object. Accessing out-of-bound object can already cause security concerns, and it would be even worse if the pointer got incremented into illegal memory. I should probably change the title of this PR to "avoid pointer overflow" or "avoid illegal memory access" to avoid any possible confusion, and look forward to implementing this fix tomorrow.
A very good takeaway from @ahsonkhan and @GrabYourPitchforks's comments is that we may not want to increment/decrement any pointer for bound checking, and I think it is also a very good opportunity to follow through on @eerhardt and @tannergooding's suggestion, which will pave the way for implementing double-computing. I will spend some time tomorrow to look into how to implement these fixes concretely.
Thank you very much for all the comments!
@briancyluibriancylui
changed the title
Change bound checking in SSE/AVX intrinsics to avoid integer overflowChange bound checking in SSE/AVX intrinsics to avoid pointer overflowSep 6, 2018
@briancylui
briancylui
commented
Sep 7, 2018

Copy link


Copy Markdown

Author
Pushed a new commit and deleted the previous commit to show you all a sample of the changes I will make globally.
In the new commit, only the AddScalarU intrinsic has been changed in its SSE and AVX implementations. Once you think that the changes look fine to you, or you may some suggestions to edit them, the changes can be made on a global scale to the rest of the intrinsics.
The performance test results are shown below:
After the change:
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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU142.9 us2.8338 us7.0045 us140.1 us
NativePerformanceTestsAddScalarU191.2 us2.3916 us1.8672 us190.9 us
SsePerformanceTestsAddScalarU172.7 us0.7552 us0.5896 us172.6 us
Before the change:
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
TypeMethodMeanErrorStdDevMedian
AvxPerformanceTestsAddScalarU141.8 us1.623 us1.439 us141.5 us
NativePerformanceTestsAddScalarU191.8 us4.514 us10.901 us186.5 us
SsePerformanceTestsAddScalarU180.3 us3.653 us7.125 us177.1 us
The performance results are comparable after the change: SEE got faster by a bit, while AVX got slower by just a bit. The second use of Math.DivRem for countSse and remainderSse may have made the AVX implementation a bit slower.
Look forward to all your comments. Since tomorrow is the last day of my internship, and I have other work items to finish before I go, I don't think I can finish this PR by tomorrow, but I will keep my branch in my fork up so that other contributors can use your previous commits. While I will leave this PR open for the moment, please feel free to close it whenever you feel appropriate. Thank you.
@briancylui
briancylui
commented
Sep 7, 2018

Copy link


Copy Markdown

Author
test MachineLearning-CI please
tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
Outdated

 private static readonly Vector256<float> _absMask256 = Avx.StaticCast<int, float>(Avx.SetAllVector256(0x7FFFFFFF));

 // The count of 32-bit floats in Vector256<T>
 private const int AvxAlignment = 8;
@tannergoodingtannergoodingSep 7, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This isn't alignment, but rather the number of 32-bit elements that Vector256 can hold...
Maybe Vector256SingleElementCount or something similar...
@briancyluibriancyluiSep 7, 2018

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

What about Vector256FloatCount? Is there any preference for SingleElement?
tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
Outdated

tannergooding
tannergooding
reviewed
Sep 7, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/SseIntrinsics.cs
Outdated

while (pDstCurrent < pDstEnd)
for (int i = 0; i < remainder; i++)
 {
 Vector128<float> dstVector = Sse.LoadScalarVector128(pDstCurrent);
@tannergoodingtannergoodingSep 7, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This would be way more readable as pDstCurrent[i] += scalar, and it should produce the same code.
The various scalar intrinsics are really meant for places where you have to interop between Vector and Scalar code, or where you need some scalar operations which aren't expressible in normal C# code.
@briancyluibriancyluiSep 7, 2018

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

Yes, perf test results show that this change improves the runtime significantly:
After all changes, including changing scalar intrinsics to indexed code:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us
After partial changes (removing the 2nd time of Math.DivRem):
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us
Before the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us
@briancylui
briancylui
commented
Sep 7, 2018•

edited

Loading

Copy link


Copy Markdown

Author
Pushed a new commit responding to @tannergooding's latest comments. Since the following suggested changes show a significant +10% perf improvement, I will propagate these changes globally to get end-to-end perf results as soon as possible today:
After all changes, including changing scalar intrinsics to indexed code:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU172.1 us2.589 us2.422 us
NativePerformanceTestsAddScalarU216.9 us1.785 us1.491 us
SsePerformanceTestsAddScalarU209.7 us1.492 us1.246 us
After partial changes (removing the 2nd time of Math.DivRem):
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU193.3 us3.6653 us3.4285 us
NativePerformanceTestsAddScalarU215.4 us0.7014 us0.6218 us
SsePerformanceTestsAddScalarU238.2 us4.1955 us3.5034 us
Before the change:
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
TypeMethodMeanErrorStdDev
AvxPerformanceTestsAddScalarU200.3 us3.919 us4.513 us
NativePerformanceTestsAddScalarU249.4 us4.400 us4.116 us
SsePerformanceTestsAddScalarU235.3 us1.393 us1.235 us
@ahsonkhan
ahsonkhan
commented
Sep 7, 2018

Copy link


Copy Markdown

we may not want to increment/decrement any pointer for bound checking
Right.
Since the following suggested changes show a significant +10% perf improvement
Nice! The implementation can be reasoned about more easily now as well.

+ We removed the risks related to overflow :)
Brian Lui
Respond to PR feedback: Replaced scalar operations by indexed code fo…
f26f112
…r all AVX intrinsics, except MatMul's and those involving AbsMask
ahsonkhan
ahsonkhan
reviewed
Sep 8, 2018
View reviewed changes
Comment threadsrc/Microsoft.ML.CpuMath/AvxIntrinsics.cs
 }

 for (int i = 0; i < remainder - 4; i++)
 for (int i = 0; i < remainder % 4; i++)
@ahsonkhanahsonkhanSep 8, 2018•

edited

Loading

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

I wouldn't use the relatively expensive modulo operator here.
Maybe, do remainder -= 4 in the above if (remainder >= 4) block and then your for loop can just be:
for(inti=0;i<remainder;i++){
...}
@briancylui
briancylui
commented
Sep 8, 2018

Copy link


Copy Markdown

Author
The latest commit contains the global replacement of scalar operations by indexed code for almost all AVX intrinsics (except for MatMul's and those methods that involve AbsMask). Changes to SSE intrinsics have not been done yet. The latest perf results are shown at the bottom of briancylui#1. Please note that now it is ready to do end-to-end perf testing using the KMeansAndLogisticRegression benchmark in test\Microsoft.ML.Benchmarks, but it is only that I didn't have enough time to test it by the end of my internship.
Thank you everyone for your feedback! I will keep my branches in my fork up there, and feel free to continue developing using my commits. Have a nice day!
@markusweimer
markusweimer
commented
Sep 21, 2018

Copy link


Copy Markdown

This PR does not reference an issue. Can you please file one and reference it in the PR description?
@briancylui
briancylui
commented
Sep 21, 2018

Copy link


Copy Markdown

Author
@ahsonkhan raised the issue and suggested the change. Anyone familiar with the issue is welcome to carry on this project after my internship if time allows.
@briancyluibriancylui
mentioned this pull request
Sep 21, 2018

Open

@briancylui
briancylui
commented
Sep 21, 2018

Copy link


Copy Markdown

Author
Created an issue #980, which this PR now references. The issue may need some polishing, but it solves the problem for the time being. Hope it helps other people interested take on this project!
@danmoseley
danmoseley
commented
Sep 24, 2018

Copy link


Copy Markdown

Seems this is almost done. @tannergooding will finish it up in a little bit.
@Zruty0
Zruty0
commented
Oct 5, 2018

Copy link


Copy Markdown

Contributor
@tannergooding , are you working on this?
@tannergooding
tannergooding
commented
Oct 5, 2018

Copy link


Copy Markdown

Member
It's on my backlog, but lower priority than a few other items right now.
@shauheen
shauheen
commented
Oct 19, 2018

Copy link


Copy Markdown

Contributor
@tannergooding should we close the PR and open it when you think you can update the branch?
@tannergooding
tannergooding
commented
Oct 19, 2018

Copy link


Copy Markdown

Member
That is fine with me.
@eerhardteerhardt
mentioned this pull request
Mar 26, 2019
 Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding left review comments

+1 more reviewer

@ahsonkhanahsonkhanahsonkhan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tannergoodingtannergooding

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

9 participants

@briancylui@eerhardt@ahsonkhan@GrabYourPitchforks@markusweimer@danmoseley@Zruty0@tannergooding@shauheen