This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] add motivational comment for tests; NFC
ClosedPublic

Authored by spatel on May 16 2017, 8:19 AM.

Details

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.

Diff Detail

Event Timeline

spatel created this revision.May 16 2017, 8:19 AM
davide edited edge metadata.May 16 2017, 9:08 AM

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

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.)

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.

davide accepted this revision.May 16 2017, 9:33 AM
This revision is now accepted and ready to land.May 16 2017, 9:33 AM
This revision was automatically updated to reflect the committed changes.