[llvm-commits] PATCH: Preserving the 'nsw' flag in the instruction combiner.

[Nick Lewycky]

Pranav Bhandarkar wrote:
> Hi Eli and Nick,
>
>>> A couple notes on the patch:
>>>
>>> 1. When submitting a patch, please include the test in the patch itself.
>>> 2. Using APInt::sadd_ov and APInt::ssub_ov might simplify the logic here a
> bit.
>
> Thanks for your suggestions. I have incorporated your suggestions in the
> attached revision of my patch. Please do let me know if this is ok to
> commit.

Thanks!

+// Return true, if NO Signed Wrap should be maintained for I.

"No Signed Wrap".

+// PreCondition - The operation 'B (I.getOpcode) C' has been simplified 
to 'V'

Just "Precondition", not "PreCondition".

+//
+// This function then checks the following
+// 1. If 'B', 'C' and 'V' are all ConstantInts
+// 2. If so, Perform checks to see if 'V' overflows

Perform shouldn't be capitalized.

+//   a) If either of B or C is zero then there is no overflow; return True.

True shouldn't be capitalized.

+//   b) If B and C have the same sign
+//       -> if Opcode = Add, then
+//                      -> if B>0 && V<B, overflow occurred; return false.
+//                      -> if B<0 && V>B, overflow occurred; return false.
+//       -> if Opcode = Sub, then
+//                      -> No overflow can occur in this case; return true.
+//   c) If B and C do not have the same sign
+//       -> Need to check only for Sub. The checks are the same as Add 
above
+//          because sub changes the sign of C in (B - C).
+//

Sounds good!

Please merge not_nsw_preserve.ll into nsw_preserve.ll. You can name the 
functions @test1 and @test2.

+static bool MaintainNoSignedWrap (BinaryOperator &I, Value *B, Value *C) {

There's a stylistic issue here and on other lines; we don't put a space 
before the ( in a function call, even though we do put one after "if". 
See http://llvm.org/docs/CodingStandards.html#micro_spaceparen .

+  Instruction::BinaryOps Opcode = I.getOpcode();
and
+  bool Overflow = false;

Please sink each of Opcode and Overflow down to right before the code 
that uses them.

+  if (Opcode == Instruction::Add) {
+    BVal.sadd_ov (CVal, Overflow);
+  }
+  else {
+    BVal.ssub_ov (CVal, Overflow);
+  }

Again a style issue, please put the "else" on the same line as the 
preceding "}".

-          I.clearSubclassOptionalData();
+          if (MaintainNoSignedWrap (I, B, C)) {
+            I.clearSubclassOptionalData();
+            I.setHasNoSignedWrap (true);
+          }
+          else
+            I.clearSubclassOptionalData();
+

How about:

   bool NSW = MaintainNoSignedWrap(I, B, C);
   I.clearSubclassOptionalData();
   I.setHasNoSignedWrap(NSW);

?

+          cast<BinaryOperator>(New)->setHasNoSignedWrap (true);

Earlier, the code reads "Instruction *New = 
BinaryOperator::Create(...)". Why not change that to "BinaryOperator 
*New = ..." to avoid the cast here?

Nick