[PATCH] D21775: [InstCombine] Simplify and correct folding fcmps with the same children

[Sanjay Patel via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

Some good boolean logic puzzles in these changes. :)
See inline comments for some nits, then LGTM.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:46
@@ +45,3 @@
+         "Unexpected FCmp predicate!");
+  // Take the advantage of the bit pattern of FCmpInst::Predicate here.
+  //                                                   U L G E
----------------
Take the advantage -> Take advantage

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:49
@@ +48,3 @@
+  static_assert(FCmpInst::FCMP_FALSE ==  0,         // 0 0 0 0
+                "FCMP_* predicates have unexpected bit patterns");
+  static_assert(FCmpInst::FCMP_OEQ   ==  1,         // 0 0 0 1
----------------
I would remove the message parameter from all of these static_asserts to tighten it up. If someone tries to change these values, it should be obvious why we don't want that. On that note, can you add a comment to the definition of CmpInst::Predicate in IntsrTypes.h that says something like, "These enum values have the magical property of matching the logical and/or of the predicates. Any changes to these values will require changes to InstCombine."

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:96
@@ -80,5 +95,3 @@
 /// This is the complement of getFCmpCode, which turns an opcode and two
-/// operands into either a FCmp instruction. isordered is passed in to determine
-/// which kind of predicate to use in the new fcmp instruction.
-static Value *getFCmpValue(bool isordered, unsigned code,
-                           Value *LHS, Value *RHS,
+/// operands into either a FCmp instruction.
+static Value *getFCmpValue(unsigned Code, Value *LHS, Value *RHS,
----------------
...or a constant true/false value.

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1125-1133
@@ +1124,11 @@
+
+  // Simplify (fcmp cc0 x, y) & (fcmp cc1 x, y).
+  // Suppose the relation between x and y is R, where R is one of
+  // U(1000), L(0100), G(0010) or E(0001), and CC0 and CC1 are the bitmasks for
+  // testing the desired relations.
+  //
+  // Since (R & CC0) and (R & CC1) are either R or 0, we actually have this:
+  //    bool(R & CC0) && bool(R & CC1)
+  //  = bool((R & CC0) & (R & CC1))
+  //  = bool(R & (CC0 & CC1)) <= by re-association, commutation, and idempotency
+  if (Op0LHS == Op1LHS && Op0RHS == Op1RHS)
----------------
This comment and the similar one below didn't make anything clearer to me. I would leave it out, but that's just my suggestion.


http://reviews.llvm.org/D21775