- rGf2e60fc4e8c4: [SimpligyCFG] NFC intended, remove GCD that was only used for powers of two
rL363422: [SimpligyCFG] NFC intended, remove GCD that was only used for powers of two
rG636220e83c2a: [SimpligyCFG] NFC intended, remove GCD that was only used for powers of two
rL363227: [SimpligyCFG] NFC intended, remove GCD that was only used for powers of two
Thanks for splitting these up! They're really much easier to read.
Some comments, but none of them fatal.
It feels to me like this comment still has value - perhaps it should be kept?
I personally would rewrite this whole thing as:
unsigned Shift = 64; for (auto &V : Values) Shift = std::min(Shift, countTrailingZeros((uint64_t)V);
I don't see the need for the zero bailout.
This comment could be clearer. I've tried to understand it a couple of times and only vaguely do. Could you please rewrite it so that it clearly states *which* edge condition you're referring to (I presume ctz(0)?)
Shift is an integer, not a pointer or boolean, so please use if (Shift != 0).
Nit: I'd use:
V >>= Shift;
Just to make it ultra-clear that we're updating V in-place (updating a mutable reference iterator is fine in this case, but it's something readers normally expect so it'd be good to make this very clear).
It's rather hidden due to overzealous auto use, but this is going to perform an arithmetic shift. This doesn't seem correct to me, as the used rotate operation degenerates to a logical shift for valid values (with low bits unset).
I completely misinterpreted this comment and this code. I thought your comment referred to *runtime behaviour* of generated cttz instructions. Actually it's just referring to the return value of llvm::countTrailingZeros().
I'd recommend making this comment less scary. My suggestion would be:
countTrailingZeros(0) returns 64. As Values is guaranteed to have more than one element and LLVM disallows duplicate cases, Shift is guaranteed to be less than 64.
... and then add an assert(Shift < 64) afterwards to prove it.
I'd recommend just switching to using APInt. That's what it's for after all - making things like this self-explaining.