Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix method names of hardware intrinsic APIs #15471

Merged
merged 1 commit into from
Dec 17, 2017
Merged

Conversation

fiigii
Copy link

@fiigii fiigii commented Dec 11, 2017

Avoid language-specific type names in hardware intrinsic APIs. Discussed in #15341 (comment)

@tannergooding @jkotas

@@ -263,19 +263,19 @@ public static class Sse2
/// <summary>
/// __m128i _mm_cvtps_epi32 (__m128 a)
/// </summary>
public static Vector128<int> ConvertToInt(Vector128<float> value) { throw new PlatformNotSupportedException(); }
public static Vector128<int> ConvertToInt32(Vector128<float> value) { throw new PlatformNotSupportedException(); }
Copy link
Member

Choose a reason for hiding this comment

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

This should be ConvertToVector128Int32 to match the naming convention the other methods have been given.

/// <summary>
/// __m128i _mm_cvtpd_epi32 (__m128d a)
/// </summary>
public static Vector128<int> ConvertToInt(Vector128<double> value) { throw new PlatformNotSupportedException(); }
public static Vector128<int> ConvertToInt32(Vector128<double> value) { throw new PlatformNotSupportedException(); }
Copy link
Member

Choose a reason for hiding this comment

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

Same

/// <summary>
/// __m128 _mm_cvtepi32_ps (__m128i a)
/// </summary>
public static Vector128<float> ConvertToFloat(Vector128<int> value) { throw new PlatformNotSupportedException(); }
public static Vector128<float> ConvertToSingle(Vector128<int> value) { throw new PlatformNotSupportedException(); }
Copy link
Member

Choose a reason for hiding this comment

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

Same

/// <summary>
/// __m128 _mm_cvtpd_ps (__m128d a)
/// </summary>
public static Vector128<float> ConvertToFloat(Vector128<double> value) { throw new PlatformNotSupportedException(); }
public static Vector128<float> ConvertToSingle(Vector128<double> value) { throw new PlatformNotSupportedException(); }
Copy link
Member

Choose a reason for hiding this comment

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

Same

@@ -288,11 +288,11 @@ public static class Sse2
/// <summary>
/// __m128i _mm_cvttps_epi32 (__m128 a)
/// </summary>
public static Vector128<int> ConvertToIntWithTruncation(Vector128<float> value) { throw new PlatformNotSupportedException(); }
public static Vector128<int> ConvertToInt32WithTruncation(Vector128<float> value) { throw new PlatformNotSupportedException(); }
Copy link
Member

Choose a reason for hiding this comment

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

ConvertToVector128Int32WithTruncation

/// <summary>
/// __m128i _mm_cvttpd_epi32 (__m128d a)
/// </summary>
public static Vector128<int> ConvertToIntWithTruncation(Vector128<double> value) { throw new PlatformNotSupportedException(); }
public static Vector128<int> ConvertToInt32WithTruncation(Vector128<double> value) { throw new PlatformNotSupportedException(); }
Copy link
Member

Choose a reason for hiding this comment

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

Same

Copy link
Author

Choose a reason for hiding this comment

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

Thank you! Will fix.

@fiigii
Copy link
Author

fiigii commented Dec 12, 2017

cc @eerhardt

@jkotas
Copy link
Member

jkotas commented Dec 14, 2017

FYI @KrzysztofCwalina @karelz @terrajobst This is changing shape of Intel intrinsics that was approved earlier. It looks like an oversight to me that can just go in without review.

@jkotas
Copy link
Member

jkotas commented Dec 14, 2017

@fiigii Could you please prepare PR with a matching change in CoreFX?

@fiigii
Copy link
Author

fiigii commented Dec 14, 2017

@jkotas Yes, but I plan to wait for #15341 to avoid breaking CoreCLR/CoreFX synchronization multip times. That may make your work easier?

@jkotas
Copy link
Member

jkotas commented Dec 14, 2017

#15341 is adding new public APIs. Adding new APIs does not break anything. It does not need to be done in lock step.

Your change is changing existing public APIs, and thus it needs to be done in lock step.

@fiigii
Copy link
Author

fiigii commented Dec 14, 2017

@jkotas Thank you for explaining, will submit the CoreFX PR.

@tannergooding
Copy link
Member

@fiigii, since they are also naming issues. There are a few other inconsistencies that would be nice to fix.

We use Sqrt in some places and SquareRoot in others

We use Hi in some places and High in others (might be worth checking for Lo vs Low, although I didn't see this specifically).

@fiigii
Copy link
Author

fiigii commented Dec 15, 2017

We use Sqrt in some places and SquareRoot in others

Ok, will make consistent to Sqrt that Math uses.

We use Hi in some places and High in others (might be worth checking for Lo vs Low, although I didn't see this specifically).

Will change to High. Thank you!

Copy link

@terrajobst terrajobst left a comment

Choose a reason for hiding this comment

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

New naming looks good me as it's making things more consistent with prior art. LGTM.

@fiigii fiigii changed the title Fix type names in hardware intrinsic APIs Fix method names of hardware intrinsic APIs Dec 16, 2017
@fiigii fiigii force-pushed the fixapi branch 2 times, most recently from 60889e7 to 4360c89 Compare December 16, 2017 08:20
@fiigii
Copy link
Author

fiigii commented Dec 16, 2017

@tannergooding PTAL. I will make the CoreFX PR if this looks good to you.

@fiigii
Copy link
Author

fiigii commented Dec 17, 2017

CoreFX PR dotnet/corefx#25965

@jkotas jkotas merged commit 767f5ff into dotnet:master Dec 17, 2017
@fiigii fiigii deleted the fixapi branch December 20, 2017 21:55
mikedn pushed a commit to mikedn/coreclr that referenced this pull request Dec 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants