[PATCH] D61151: [SimpligyCFG] NFC, remove GCD that was only used for powers of two

[James Molloy via Phabricator via llvm-commits]

jmolloy added inline comments.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5553
 
-  unsigned Shift = Log2_64(GCD);
+  // Cttz often has an edge condition on 0 which means that the bit-width
+  // is important, however here there is no such edge condition because if
----------------
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.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5563
+    for (auto &V : Values)
+      V = (int64_t)((uint64_t)V >> Shift);
 
----------------
I'd recommend just switching to using APInt. That's what it's for after all - making things like this self-explaining.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61151/new/

https://reviews.llvm.org/D61151