Improve BMI2 MultiplyNoFlags APIs by fiigii · Pull Request #21362 · dotnet/coreclr

fiigii · 2018-12-04T20:30:49Z

Close https://github.com/dotnet/corefx/issues/33615

tannergooding

LGTM.

tannergooding · 2018-12-04T20:32:56Z

+            /// The above native signature does not directly correspond to the managed signature. 
+            /// This intrinisc is only available on 64-bit processes
+            /// </summary>
+            public static ulong MultiplyNoFlags(ulong left, ulong right) { throw new PlatformNotSupportedException(); }


nit: Might be nice to order this above the one that takes 3 parameters.

Done, thanks.

mikedn · 2018-12-04T20:36:19Z

+        /// The above native signature does not directly correspond to the managed signature. 
        /// </summary>
-        public static unsafe uint MultiplyNoFlags(uint left, uint right, uint* high) { throw new PlatformNotSupportedException(); }
+        public static unsafe uint MultiplyNoFlags(uint left, uint right, uint* low) { throw new PlatformNotSupportedException(); }


Erm, implementing this intrinsic will be fun.

@mikedn Concerns?

I think it's the same concern raised in the other thread, in that (like with Div/Rem) we don't necessarily have an easy way to return multiple results from a single instruction.

I think the "inefficient" codegen might be easier, but it will require an explicit store to memory as part of the codegen (where-as the efficient one would let us keep it in register/etc).

think the "inefficient" codegen might be easier, but it will require an explicit store to

The perf of "explicit store" would be fine, uint/ulong can be handled by store forward on Intel CPUs. And we have provided "no store" version, which avoids additional stores when users do not need "low".

This intrinsic both stores to memory and returns a value. That's a bit unusual, I think the only similar opers are the atomic ones. We'll see how it goes.

Well, it's still kind of useless, or better said inconvenient. This particular functionality exists on any x86/x64 CPU, but it is exposed only as part of BMI2

The BMI2 specific instruction, which doesn't set any CPU flags will be part of BMI2 exclusively.

There is a separate discussion about exposing additional Base intrinsics (for things like mul, div, etc) that needs a proposal, review, etc

We probably want "general-purpose" versions exposed in System.Math (and for existing methods to be special-cases, where applicable). However, there are still special semantics to the x86 instructions that may not be generally applicable and for which exposing intrinsics would still be desirable

Also, to be clear, the Base intrinsics would not apply to just any instruction; but for ones which expose special semantics that are desirable for high-perf scenarios (much as is done for the C/C++ intrinsics that fit this category)

The BMI2 specific instruction, which doesn't set any CPU flags will be part of BMI2 exclusively.

Apparently you are having trouble understanding that nobody needs or asked for the "no flags" functionality while everyone asked for the broadly available 128 bit functionality. And instead of that they've got the somewhat less available BMI2 form.

while everyone asked for the broadly available 128 bit functionality

No, I completely understood that users have wanted the more broadly available functionality, which is why I mentioned the existing discussions that have been had for the other requested functionality.

And instead of that they've got the somewhat less available BMI2 form.

Yes, they will get this initially, because it is part of the already reviewed/approved API set.

Adding APIs for things like __addcarry, _bittest, _mul128, _mulh, etc... were not part of the initially reviewed/approved API set and have to go through the process independently (and are tracked by issues such as https://github.com/dotnet/corefx/issues/32075, and others which are scattered about).

Yes, they will get this initially, because it is part of the already reviewed/approved API set.

Then something went wrong somewhere with the review/approve/prioritize process.

fiigii · 2018-12-04T20:40:01Z

Pushed the CoreFX change in dotnet/corefx#33805 because the change relies on it. @tannergooding Is it okay?

tannergooding · 2018-12-04T20:44:02Z

That's fine with me. The pump still appears to be blocked so I think everything should line up.

mikedn · 2018-12-04T21:01:00Z

        /// <summary>
        /// unsigned int _mulx_u32 (unsigned int a, unsigned int b, unsigned int* hi)
        ///   MULX r32a, r32b, reg/m32
+        /// The above native signature does not directly correspond to the managed signature. 


Is it me or the comment above is wrong? It shows a unsigned int* hi parameter.

This PR changes the APIs to return “high”, which is different from C++.

fiigii · 2018-12-05T00:02:07Z

@tannergooding Can we merge this PR?

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

* Improve BMI2 MultiplyNoFlags APIs (dotnet/coreclr#21362) Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> * Add AsyncIteratorStateMachineAttribute Exactly follows the design of AsyncStateMachineAttribute and IteratorStateMachineAttribute; the only thing different is the type name, "AsyncIterator" instead of "Async" and "Iterator". Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> * Address PR feedback Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

pentp · 2018-12-05T22:51:19Z

@@ -33,9 +33,18 @@ internal X64() { }
            /// <summary>
            /// unsigned __int64 _mulx_u64 (unsigned __int64 a, unsigned __int64 b, unsigned __int64* hi)
            ///   MULX r64a, r64b, reg/m64


This could be better expressed as MULX r64a, r64a, reg/m64 (both src1 and dest being the same register) as that is the actual instruction that will return only the high part (which should be called out here in docs).

* Improve BMI2 MultiplyNoFlags APIs (dotnet/coreclr#21362) Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> * Add AsyncIteratorStateMachineAttribute Exactly follows the design of AsyncStateMachineAttribute and IteratorStateMachineAttribute; the only thing different is the type name, "AsyncIterator" instead of "Async" and "Iterator". Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> * Address PR feedback Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

* Improve BMI2 MultiplyNoFlags APIs (dotnet/coreclr#21362) Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> * Add AsyncIteratorStateMachineAttribute Exactly follows the design of AsyncStateMachineAttribute and IteratorStateMachineAttribute; the only thing different is the type name, "AsyncIterator" instead of "Async" and "Iterator". Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> * Address PR feedback Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> (cherry picked from commit 1c47747)

Commit migrated from dotnet/coreclr@2217719

tannergooding approved these changes Dec 4, 2018

View reviewed changes

tannergooding reviewed Dec 4, 2018

View reviewed changes

Improve BMI2 MultiplyNoFlags APIs

695de5f

fiigii force-pushed the bmiapi branch from 14c1704 to 695de5f Compare December 4, 2018 20:35

mikedn reviewed Dec 4, 2018

View reviewed changes

fiigii mentioned this pull request Dec 4, 2018

Remove legacy 64-bit intrinsic APIs and improve BMI2 MultiplyNoFlags APIs dotnet/corefx#33805

Merged

mikedn reviewed Dec 4, 2018

View reviewed changes

tannergooding merged commit 2217719 into dotnet:master Dec 5, 2018

fiigii deleted the bmiapi branch December 5, 2018 00:32

Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request Dec 5, 2018

Improve BMI2 MultiplyNoFlags APIs (dotnet/coreclr#21362)

5f4b815

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request Dec 5, 2018

Improve BMI2 MultiplyNoFlags APIs (dotnet/coreclr#21362)

08303f5

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corert that referenced this pull request Dec 5, 2018

Improve BMI2 MultiplyNoFlags APIs (dotnet/coreclr#21362)

975f1a4

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

stephentoub pushed a commit to dotnet/corert that referenced this pull request Dec 5, 2018

Improve BMI2 MultiplyNoFlags APIs (dotnet/coreclr#21362)

9d44971

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

pentp reviewed Dec 5, 2018

View reviewed changes

fiigii mentioned this pull request Dec 11, 2018

Implement the remaining BMI1/2 intrinsic #21480

Merged

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Improve BMI2 MultiplyNoFlags APIs (dotnet/coreclr#21362)

4a3b539

Commit migrated from dotnet/coreclr@2217719

Conversation

fiigii commented Dec 4, 2018

Uh oh!

tannergooding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fiigii commented Dec 4, 2018

Uh oh!

tannergooding commented Dec 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fiigii commented Dec 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tannergooding Dec 4, 2018 •

edited

Loading