[PATCH] Added instruction combine to transform few more negative values addition to subtraction (Part 3)

Jingyue Wu jingyue at google.com
Wed Jun 25 20:57:05 PDT 2014


Looks good overall. 

I have a more high-level question. Why are these logics in checkForNegativeOperand instead of visitXor? Aren't transformations such as
```
XOR(AND(Z, ~C), (~C + 1)) => NEG(OR(Z, C))
```
beneficial even the XOR is not the RHS of an ADD?

================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:962
@@ -961,2 +961,3 @@
 //   ADD(XOR(OR(Z, NOT(C)), C)), 1) == NEG(AND(Z, C))
 //   TODO: ADD(XOR(AND(Z, ~C), ~C), 1) == NEG(OR(Z, C)) if C is even
+//   XOR(AND(Z, ~C), (~C + 1)) == NEG(OR(Z, C)) if C is odd
----------------
Make sure this TODO goes away after you rebase. 

================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:998
@@ -997,1 +997,3 @@
 
+  // Restore LSH and RHS
+  LHS = I.getOperand(0);
----------------
typo "LHS"

================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1007
@@ +1006,3 @@
+  // C2 is ODD
+  // LHS = XOR(Y, C1), Y = AND(Z, C2), C1 == (C2 + 1) => LHS == NOT(AND(Z, ~C2))
+  // ADD(LHS, RHS) == SUB(RHS, AND(Z, ~C2))
----------------
```
LHS == NOT(AND(Z, ~C2))
```
is not consistent with previous comments using
```
NEG(OR(Z, C))
```

Are they mathematically equivalent? If so, could you make the comments consistent? 

================
Comment at: test/Transforms/InstCombine/add2.ll:154
@@ +153,3 @@
+define i32 @test17(i32 %x) {
+ %x.not = and i32 %x, -1431655766
+ %add3 = xor i32 %x.not, -1431655765
----------------
I'd love to see some comments on what math expression these IR instructions compute, and what those magic numbers (e.g., -1431655766) mean. 

Same for the next two test cases.

http://reviews.llvm.org/D4210






More information about the llvm-commits mailing list