4 ms·

Even so, if you wrap it in a function and make sure to document what's happening, I would argue that a version without asm() is preferable. It gives the compile

Even so, if you wrap it in a function and make sure to document what's happening, I would argue that a version without asm() is preferable. It gives the compiler more leeway for optimization and is easier to read for someone who isn't well-versed in GCC's weird asm syntax. See the generated code for these two alternative implementations: https://godbolt.org/z/3vno6G46j https://godbolt.org/z/3vno6G46j

jcalvinowens 2y ago
That's actually just a bug: the x86 asm constraints were needlessly forcing the compiler to use rdx as the input register for the multiply instruction. With that fixed, it's the same: https://godbolt.org/z/M571P371K https://godbolt.org/z/M571P371K But after staring at this for a little while, I realized simply reversing the order of the fields in the dword struct will make the result from the multiply instruction already match the way 128-bit structs are returned in registers under the x86_64 calling convention! This is quite a bit better: https://godbolt.org/z/MbfG63vej https://godbolt.org/z/MbfG63vej Also worth noting the asm makes nicer code on arm64: https://godbolt.org/z/7eas6s9vK https://godbolt.org/z/7eas6s9vK https://github.com/jcalvinowens/toy-rsa/commit/59ef9ea905dbda3698136065553c776adbefc2bb https://github.com/jcalvinowens/toy-rsa/commit/59ef9ea905dbd... https://github.com/jcalvinowens/toy-rsa/commit/ceaa8a4dd083402aac7a6606014577563f48034c https://github.com/jcalvinowens/toy-rsa/commit/ceaa8a4dd0834...
- winternewt 2y ago
  Yes I also noticed the bug in the constraints and came to the same conclusion that changing them yields the same code for both implementations. But I figured that the fact that a function with a single asm instruction has a bug kind of supports my point. :) As for the codegen on ARM, I don't think your asm implementation is correct there either. As far as I can tell the UMULL instruction only cares about the least significant 32 bits in the source registers, regardless if you're on a 64-bit CPU: https://developer.arm.com/documentation/ddi0602/2024-03/Base-Instructions/UMULL--Unsigned-Multiply-Long--an-alias-of-UMADDL- https://developer.arm.com/documentation/ddi0602/2024-03/Base...
  - jcalvinowens 2y ago
    Very much disagree with your point about constraints. A bug existing in 10+ year old code that has never really been tested and run by four people doesn't support any point. In real life one actually checks these things, it's not that complex :) Case in point: counting the f's in your constants took me longer than finding that constraint bug did. You would argue ULLONG_MAX would fix that, I suppose. You're right, that's the 32-bit arm instruction, doh. In my defense, this code was written before 64-bit ARM existed!

jcalvinowens 2y ago

That's actually just a bug: the x86 asm constraints were needlessly forcing the compiler to use rdx as the input register for the multiply instruction. With that fixed, it's the same: https://godbolt.org/z/M571P371K https://godbolt.org/z/M571P371K But after staring at this for a little while, I realized simply reversing the order of the fields in the dword struct will make the result from the multiply instruction already match the way 128-bit structs are returned in registers under the x86_64 calling convention! This is quite a bit better: https://godbolt.org/z/MbfG63vej https://godbolt.org/z/MbfG63vej Also worth noting the asm makes nicer code on arm64: https://godbolt.org/z/7eas6s9vK https://godbolt.org/z/7eas6s9vK https://github.com/jcalvinowens/toy-rsa/commit/59ef9ea905dbda3698136065553c776adbefc2bb https://github.com/jcalvinowens/toy-rsa/commit/59ef9ea905dbd... https://github.com/jcalvinowens/toy-rsa/commit/ceaa8a4dd083402aac7a6606014577563f48034c https://github.com/jcalvinowens/toy-rsa/commit/ceaa8a4dd0834...

winternewt 2y ago
Yes I also noticed the bug in the constraints and came to the same conclusion that changing them yields the same code for both implementations. But I figured that the fact that a function with a single asm instruction has a bug kind of supports my point. :) As for the codegen on ARM, I don't think your asm implementation is correct there either. As far as I can tell the UMULL instruction only cares about the least significant 32 bits in the source registers, regardless if you're on a 64-bit CPU: https://developer.arm.com/documentation/ddi0602/2024-03/Base-Instructions/UMULL--Unsigned-Multiply-Long--an-alias-of-UMADDL- https://developer.arm.com/documentation/ddi0602/2024-03/Base...
- jcalvinowens 2y ago
  Very much disagree with your point about constraints. A bug existing in 10+ year old code that has never really been tested and run by four people doesn't support any point. In real life one actually checks these things, it's not that complex :) Case in point: counting the f's in your constants took me longer than finding that constraint bug did. You would argue ULLONG_MAX would fix that, I suppose. You're right, that's the 32-bit arm instruction, doh. In my defense, this code was written before 64-bit ARM existed!