[PATCH] D18841: [InstCombine] Canonicalize icmp instructions based on dominating conditions.

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/IR/ConstantRange.cpp:132
@@ +131,3 @@
+                                                 const APInt &C) {
+  assert(makeAllowedICmpRegion(Pred, C) == makeSatisfyingICmpRegion(Pred, C));
+  return makeAllowedICmpRegion(Pred, C);
----------------
Can you add a comment on why this assert is valid (i.e. that we're relying on knowledge of how the two `makeAllowedXXXRegion` s are implemented).

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:118
@@ -117,1 +117,3 @@
 
+/// isBranchOnSignBitCheck - Given an exploded icmp instruction, return true if
+/// any use of this comparison is a branch on sign bit comparison.
----------------
We no longer repeat function names in comments.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:119
@@ +118,3 @@
+/// isBranchOnSignBitCheck - Given an exploded icmp instruction, return true if
+/// any use of this comparison is a branch on sign bit comparison.
+static bool isBranchOnSignBitCheck(ICmpInst &I, bool isSignBit) {
----------------
This isn't an exploded icmp right?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3308
@@ +3307,3 @@
+        return replaceInstUsesWith(I, Builder->getTrue());
+      if (!isBranchOnSignBitCheck(I, isSignBit) && !I.isEquality()) {
+        if (Intersection.isSingleElement()) {
----------------
Add a comment on why you need `isBranchOnSignBitCheck` here.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3309
@@ +3308,3 @@
+      if (!isBranchOnSignBitCheck(I, isSignBit) && !I.isEquality()) {
+        if (Intersection.isSingleElement()) {
+          const APInt *API = Intersection.getSingleElement();
----------------
These are better written as:

```
if (auto *AI = Intersection.getSingleElement())
  return new ICmpInst(..., Builder->getInt(*AI));
```

etc.


http://reviews.llvm.org/D18841