Removed redundant shift in std::gcd

[matt77hias]

No description provided.

[strega-nil-ms]

Please cache _Countr_zero_impl(_Nx_magnitude). Otherwise, this looks great!

[CaseyCarter]

I've pushed a change to avoid the redundant _Countr_zero_impl call by breaking in the middle of the loop. This is a large enough change that I've reset the review @strega-nil-ms.

[matt77hias]

Note that you can also write something like:

const auto _Mx_trailing_zeroes = static_cast<unsigned long>(_Countr_zero_impl(_Mx_magnitude));
const auto _Common_factors_of_2 = static_cast<unsigned long>(_Countr_zero_impl(_Mx_magnitude | _Nx_magnitude));

which I assume to be faster in debug builds as it does not involve a function (<> std::min).

Cfr. https://hbfs.wordpress.com/2013/12/10/the-speed-of-gcd/

[CaseyCarter]

Note that you can also write something like:

const auto _Mx_trailing_zeroes = static_cast<unsigned long>(_Countr_zero_impl(_Mx_magnitude));
const auto _Common_factors_of_2 = static_cast<unsigned long>(_Countr_zero_impl(_Mx_magnitude | _Nx_magnitude));

which I assume to be faster in debug builds as it does not involve a function (<> std::min).

Doesn't this just replace the call to min with a call to _Countr_zero_impl?

[StephanTLavavej]

It appears that the MS/GitHub infrastructure is running two different Contributor License Agreement bots (not specific to this PR, the second one appeared fairly recently) and one of them is saying "Contributor License Agreement is not agreed yet", possibly because @CaseyCarter expanded the original 1-line PR beyond the "too tiny to need a CLA" limit.

@matt77hias, have you gotten a link to click-sign the CLA?

[matt77hias]

Doesn't this just replace the call to min with a call to _Countr_zero_impl?

I would have expected this to map to assembly instructions. My bad.

have you gotten a link to click-sign the CLA?

No, I haven't received a link (not in the thread, not by email).

[StephanTLavavej]

No, I haven't received a link (not in the thread, not by email).

Ok, thanks. I'm going to try closing and reopening this PR, which in the past has gotten the CLA Bot to rerun its check, at which point it should hopefully send you the link. (Unlike the actual test runs, we don't have any other way to control the CLA Bot.) Let's see if this works...

[matt77hias]

@microsoft-github-policy-service agree

[matt77hias]

@microsoft-github-policy-service agree

[StephanTLavavej]

Thanks - the new (blue symbol) license/cla check appears to be green, saying "All CLA requirements met.", so I'm not sure why those "incorrect command" messages were emitted. (This is the first time I've seen the new bot in action.)

I think you're good to go, as the green check should allow us to merge this, after the build+tests pass (they unavoidably run again when a PR is reopened, should take ~25-30 minutes). 😸

[matt77hias]

I'm not sure why those "incorrect command" messages were emitted.

I tried (and deleted) @microsoft-github-policy-service agree [company=""] before those messages as well, because I did not receive any kind of acknowledgment for @microsoft-github-policy-service agree from the bot.

[StephanTLavavej]

Ah, thanks for explaining! I'm guessing that it doesn't reply with a comment for success, instead just turning the check green, but "silence is success" is quite surprising for a comment-driven interface.

Thanks for being a test subject here 😻 - now we'll know what to do the next time a new contributor runs into this bot behavior. 🤖

[AreaZR]

Any update on this?

[fsb4000]

Any update on this?

The PR is in "Ready To Merge" stage: https://github.com/microsoft/STL/projects/1

[StephanTLavavej]

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

[StephanTLavavej]

Thanks for noticing that the performance could be improved here - and congratulations on your first microsoft/STL commit! 🎉 😻 🚀

This will ship in VS 2022 17.5 Preview 2.