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

[James Molloy via Phabricator via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.

Thanks for splitting these up! They're really much easier to read.

Some comments, but none of them fatal.

Cheers!



================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5543
 
-  // Now we have signed numbers that have been shifted so that, given enough
-  // precision, there are no negative values. Since the rest of the transform
----------------
It feels to me like this comment still has value - perhaps it should be kept?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5550
+  unsigned Shift = 64;
+  for (auto &V : Values) {
+    // There is no edge condition when the BitWidth is less than 64, because if
----------------
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.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5551
+  for (auto &V : Values) {
+    // There is no edge condition when the BitWidth is less than 64, because if
+    // 0 is the only value then a shift does nothing, and LLVM requires
----------------
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)?)


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5561
+  }
+  if (Shift) {
+    for (auto &V : Values)
----------------
Shift is an integer, not a pointer or boolean, so please use if (Shift != 0).


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5563
+    for (auto &V : Values)
+      V = V >> Shift;
+  }
----------------
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).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61151