Change SseUtils call sites to call CpuMathUtils instead

[briancylui]

After all active SSE intrinsics have been implemented and tested for correctness and speed, this PR asks to make all external SseUtils call sites call CpuMathUtils instead. This would allow us to obtain a baseline performance of C# hardware intrinsics APIs in a future PR.

cc: @safern @danmosemsft @eerhardt @tannergooding

[briancylui]

All 5 checks have passed for this new PR submitted today.

After all active SSE intrinsics have been implemented and tested for correctness and speed, this PR asks to make all external SseUtils call sites call CpuMathUtils instead. This would allow us to obtain a baseline performance of C# hardware intrinsics APIs in a future PR.

This PR is now ready for review.

[safern]

It seems like you missed 2 instances:

machinelearning/src/Microsoft.ML.CpuMath/CpuAligenedMathUtils.cs

Line 19 in b51d9f9

Contracts.Assert(mat.Items.CbAlign == SseUtils.CbAlign);

machinelearning/src/Microsoft.ML.CpuMath/CpuAligenedMathUtils.cs

Line 32 in b51d9f9

Contracts.Assert(vec.Items.CbAlign == SseUtils.CbAlign);

[briancylui]

@safern Seems that those two SseUtils references are wrapped around by #if and #endif that are only used in Debug. Thank you for the nice catch - now fixed!

[eerhardt]

This shouldn’t be here. This is Sse specific, but CpuMathUtils should be agnostic of the ISA.

[eerhardt]

Maybe if you name it ‘SseCbAlign’, that would work.

[briancylui]

Agree and thanks! The only current call site of CbAlign is in this file:

machinelearning/src/Microsoft.ML.Transforms/RffTransform.cs

Lines 24 to 29 in b51d9f9

	namespace Microsoft.ML.Runtime.Data
	{
	using CpuUtils = SseUtils;
	public sealed class RffTransform : OneToOneTransformBase
	{

where Line 26 should be changed to using CpuMathUtils;, and the bunch of CpuUtils.CbAlign in subsequent lines should be changed to CpuMathUtils.SseCbAlign for the moment. While this change may expose the ISA-specific element (the Sse prefix of SseCbAlign) out of CpuMathUtils, I think it's good for now.

In the future when we also support AVX, should I change public const int CbAlign = 16; to something like the code below?

public const int CbAlign = Avx.IsSupported ? 32 : 16;
**OR**
public int GetCbAlign() => Avx.IsSupported ? 32 : 16;

[briancylui]

test VSTS: public-CI please

[briancylui]

test VSTS: public-CI

[tannergooding]

What does CbAlign stand for (count of bytes alignment)?

Following .NET Naming Conventions might be desirable...

[briancylui]

I also want to know what CbAlign stands for. I was following the existing code:

machinelearning/src/Microsoft.ML.CpuMath/Sse.cs

Lines 11 to 14 in b51d9f9

	public static class SseUtils
	{
	public const int CbAlign = 16;

[briancylui]

After some detective work, the convention seems to stem from AlignedArray.cs:

machinelearning/src/Microsoft.ML.CpuMath/AlignedArray.cs

Lines 19 to 28 in b51d9f9

	public sealed class AlignedArray
	{
	// Items includes "head" items filled with NaN, followed by _size entries, followed by "tail"
	// items, also filled with NaN. Note that _size * sizeof(Float) is divisible by _cbAlign.
	// It is illegal to access any slot outsize [_base, _base + _size). This is internal so clients
	// can easily pin it.
	internal Float[] Items;
	private readonly int _size; // Must be divisible by (_cbAlign / sizeof(Float)).
	private readonly int _cbAlign; // The alignment in bytes, a power of two, divisible by sizeof(Float).

[tannergooding]

Cb normally stands for something like count of bytes.

I would probably rename this to something like Vector128Alignment, or something similar. That makes it clear that it is the alignment for the Vector128<T> type, and follows a "normal" .NET Naming Convention.

[briancylui]

Changed the naming of SseCbAlign to Vector128Alignment in the latest commit. Thanks for your suggestion!

[tannergooding]

Overall LGTM.

I would still consider renaming SseCbAlign before merging.

[briancylui]

Incorporated naming suggestions in the latest commits. All 5 checks have passed with two reviewers' approvals.

[safern]

Merging since naming has been fixed and if @eerhardt has another suggestion, since he is out, could be done in a follow up.