Use __shiftright128 intrinsic in secp256k1_u128_rshift on MSVC #1336

1. 
   hebasto commented at 12:28 pm on June 1, 2023: member

   Closes #1324.

   As the __shiftright128 docs state:

   The Shift value is always modulo 64…

   it is not applicable for the n >= 64 branch.

2. 
   real-or-random commented at 8:01 pm on June 1, 2023: contributor

   Thanks for working on this.

   it is not applicable for the n >= 64 branch.

   Good point, can you add a VERIFY_CHECK(n < 64); over the intrinsic line to document this?

   This #ifdef should be active on CI, right? So this is tested on CI? It may make sense to add a few simple (fixed-input) tests for that function.

3. hebasto force-pushed on Jun 4, 2023
4. 
   hebasto commented at 3:21 pm on June 4, 2023: member

   it is not applicable for the n >= 64 branch.

   Good point, can you add a VERIFY_CHECK(n < 64); over the intrinsic line to document this?

   Added.

   This #ifdef should be active on CI, right?

   Yes.

   So this is tested on CI?

   It is.

   It may make sense to add a few simple (fixed-input) tests for that function.

   In addition to https://github.com/bitcoin-core/secp256k1/blob/60556c9f49a9384efd7f16b734820ae19108f053/src/tests.c#L1990-L1995 ?

5. Use `__shiftright128` intrinsic in `secp256k1_u128_rshift` on MSVC 5b7bf2e9d4
6. hebasto force-pushed on Jun 4, 2023
7. 
   hebasto commented at 5:04 pm on June 4, 2023: member

   Rebased to avoid CI failures.
8. real-or-random approved
9. 
   real-or-random commented at 6:53 am on June 5, 2023: contributor

   utACK 5b7bf2e9d4ee02cbec1105ad6e890c34a4da1beb

   In addition to …

   Sorry, I missed the existing test.

10. real-or-random added the label performance on Jun 5, 2023
11. 
    sipa commented at 8:14 pm on June 23, 2023: contributor

    utACK 5b7bf2e9d4ee02cbec1105ad6e890c34a4da1beb
12. real-or-random approved
13. 
    real-or-random commented at 8:15 am on June 24, 2023: contributor

    ACK 5b7bf2e9d4ee02cbec1105ad6e890c34a4da1beb tested with MSVC x64
14. real-or-random merged this on Jun 24, 2023
15. real-or-random closed this on Jun 24, 2023

    ---

16. hebasto deleted the branch on Jun 24, 2023
17. vmta referenced this in commit 8f03457eed on Jul 1, 2023
18. fanquake referenced this in commit 56c05c5ec4 on Jul 17, 2023
19. fanquake referenced this in commit ff061fde18 on Jul 18, 2023
20. hebasto referenced this in commit 270d2b37b8 on Jul 21, 2023
21. jonasnick cross-referenced this on Jul 26, 2023 from issue Upstream PRs 1314, 1317, 1318, 1316, 1327, 1310, 1328, 1333, 1330, 1334, 1337, 1341, 1339, 1350, 1349, 1338, 1129, 1347, 1336, 1295, 1354, 1355, 1356 by jonasnick

Contributors
hebasto real-or-random sipa

Labels
performance