[PATCH] Strength reduce intrinsics with overflow into regular arithmetic operations if possible.

[Michael Gottesman]

I would comment this a little differently. I think it is good to have the Hacker's Delight mention, but IIRC LLVM has some specific rules about this. I would just ask on the list or irc. The actual implementation looks fine to me (it is exactly the same as hacker's delight).


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1016
@@ +1015,3 @@
+  if (IntegerType *IT = dyn_cast<IntegerType>(LHS->getType())) {
+    unsigned BitWidth = IT->getBitWidth();
+    unsigned SignBits = ComputeNumSignBits(LHS, 0, CxtI) +
----------------
I would put the comment below about Hacker's delight up here and add a quick high level explanation of what you are doing.

================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1017
@@ +1016,3 @@
+    unsigned BitWidth = IT->getBitWidth();
+    unsigned SignBits = ComputeNumSignBits(LHS, 0, CxtI) +
+                        ComputeNumSignBits(RHS, 0, CxtI);
----------------
Make sure to mention that underestimating the number of sign bits gives you a more conservative answer so the fact that we are underestimating can not cause us to get the wrong answer.

================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1021
@@ +1020,3 @@
+    // Multiplying n * m significant bits yields a result of n + m significant
+    // bits.
+    // See "Hacker's Delight" by Henry Warren, chapter 2: Overflow Detection.
----------------
You should add why it is interesting that the result is n+m significant bits. Otherwise the line does not add anything.

================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1029
@@ +1028,3 @@
+    if (SignBits == BitWidth + 1) {
+      // Handle the ambiguous case. It overflows only when both arguments
+      // are negative and the true product is exactly the minimum negative
----------------
There are 2x ambiguous cases. I would say what they actually are. Also you should mention that we are not handling the second case.

I would also specify that you are only handling a part of the first ambiguous case that can be known easily via the information at hand pi.e. that there is a low likelihood that we will have enough information to check the product, but we *can* check the sign = )].

================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1038
@@ +1037,3 @@
+      if (LHSNonNegative || RHSNonNegative)
+        // A simple check: No overflow if at least one side is not negative.
+        return true;
----------------
I would remove this if you do what I suggested with the comment above.

http://reviews.llvm.org/D6477

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/