[PATCH] D12202: Add Support for Small Size Reductions

[Chad Rosier via llvm-commits]

mcrosier added a comment.

Just some minor style comments.


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:46
@@ +45,3 @@
+  case RK_IntegerMinMax:
+    IsIntegerRecurrenceKind = true;
+  default:
----------------
You could simplify this with just a 'return true;'

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:47
@@ +46,3 @@
+    IsIntegerRecurrenceKind = true;
+  default:
+    break;
----------------
The default case is generally at the top of the switch unless all cases are covered.  In that case (i.e., all cases of the enum are covered), the default case should be excluded.  :P

http://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:50
@@ +49,3 @@
+  }
+  return IsIntegerRecurrenceKind;
+}
----------------
return false;

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:58
@@ +57,3 @@
+bool RecurrenceDescriptor::isArithmeticRecurrenceKind(RecurrenceKind Kind) {
+  bool IsArithmeticRecurrenceKind = false;
+  switch (Kind) {
----------------
Same comments as above.

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:75
@@ +74,3 @@
+                                     SmallPtrSetImpl<Instruction *> &CI) {
+  if (Phi->hasOneUse()) {
+    const APInt *M = nullptr;
----------------
You could reduce indentation by using an early exit:

if (!Phi->hasOneUse())
  return Phi;

http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:101
@@ +100,3 @@
+  SmallVector<Instruction *, 8> Worklist;
+  int IsSigned = -1;
+  Worklist.push_back(Exit);
----------------
The mixing of the variable with a bool makes me slightly concerned.  It might just be me, but perhaps we could clean this up with two bools or an enum.

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:132
@@ +131,3 @@
+        if (IsSigned == -1)
+          IsSigned = IsSExtInst;
+        else
----------------
This is the part that concerns me with mixing int with bool.


http://reviews.llvm.org/D12202