[InstCombine] add motivational comment for tests; NFC

[spatel]

Summary

The referenced tests are derived from:
https://bugs.llvm.org/show_bug.cgi?id=32791
and:
https://reviews.llvm.org/D33172

The motivation for including negative tests may not be clear, so I'm proposing an explanatory comment here.
In the post-commit thread for r303133:
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170515/453793.html
...it was mentioned that we don't want to add redundant tests. This is a valid point. But in this case, we have a patch under review (D33172) that demonstrates that no existing regression tests are affected by a proposed code change, but these are. Therefore, I think these tests have value not visible in any existing regression tests regardless of whether they show a transform.

[davide]

The wording is OK. What kind of infloop are you talking about here? Can you please elaborate?

[spatel]

In D33242#756203, @davide wrote:

The wording is OK. What kind of infloop are you talking about here? Can you please elaborate?

Consider that we have this canonicalization for branch conditions:

if (FPred == FCmpInst::FCMP_ONE || FPred == FCmpInst::FCMP_OLE ||
    FPred == FCmpInst::FCMP_OGE) {
  FCmpInst *Cond = cast<FCmpInst>(BI.getCondition());
  Cond->setPredicate(FCmpInst::getInversePredicate(FPred));

I would assume that the same rule applies to a select condition; ie, why would we treat equivalent-functionality selects and branches any differently?

But currently, we have this opposite canonicalization for select conditions:

// Canonicalize to use ordered comparisons by swapping the select
// operands.
//
// e.g.
// (X ugt Y) ? X : Y -> (X ole Y) ? Y : X

If we have tests for both of those patterns, then a proposed change of the canonicalization rules should show the conflict.
(On closer inspection, I don't think the FP test included here covers a potential conflict predicate...so we might want another test, but I'll see if I can find if a similar test already exists first.)

[davide]

This is fine. It's just a theoretical concern, but OK. Please keep in mind that the transform you're proposing won't work if the first icmp is located in a different position in the function, for that you need value numbering.

test/Transforms/InstCombine/logical-select.ll

;
   %iftmp.0.0 = select i1 %t0, i32 -1, i32 0
   %t1 = and i32 %iftmp.0.0, %c
   %iftmp = select i1 %t0, i32 0, i32 -1
   %t2 = and i32 %iftmp, %d
   %t3 = or i32 %t1, %t2
   ret i32 %t3
 }
 
+; TODO: For the next 4 tests, are there potential canonicalizations and/or folds for these
+; in InstCombine? Independent of that, tests like this that may not show any transforms 
+; still have value because they can help identify conflicting canonicalization rules that 
+; lead to infinite looping. 
+
 ; PR32791 - https://bugs.llvm.org//show_bug.cgi?id=32791
 ; Fold two selects with inverted predicates and zero operands.
 define i32 @fold_inverted_icmp_preds(i32 %a, i32 %b, i32 %c, i32 %d) {
 ; CHECK-LABEL: @fold_inverted_icmp_preds(
 ; CHECK-NEXT:    [[CMP1:%.*]] = icmp slt i32 %a, %b
 ; CHECK-NEXT:    [[SEL1:%.*]] = select i1 [[CMP1]], i32 %c, i32 0
 ; CHECK-NEXT:    [[CMP2:%.*]] = icmp sge i32 %a, %b
 ; CHECK-NEXT:    [[SEL2:%.*]] = select i1 [[CMP2]], i32 %d, i32 0