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

[Sanjay Patel via llvm-commits]

spatel added a comment.

In http://reviews.llvm.org/D21775#469456, @timshen wrote:

> > 1. Create all N tests in the existing test files. I don't think there's much value in the commuted variants, so this would actually be N = 16*17 = 272 I think.
>
> > 2. Run the script to generate the CHECK lines.
>
> > 3. Commit these test changes to trunk.
>
>
> I prefer not to commit this to the trunk, but only locally, since @t6 in or-fcmp.ll is testing an actual existing bug (sorry for not put that explicitly). It'd be weird to check-in something that is known wrong, and make them pass later.
>
> How about I compare the auto-generated CHECKs and post the functional differences here in the comments, we inspect it, and then check-in the patch with all N tests (with correct CHECKs) together?


I started looking over the diffs in the gist, but I'm going to suggest again that you check in the tests with current output as a preliminary step to this patch. IMO, the fact that we have miscompiles is more reason to do it that way. The advantages are:

1. There's a direct before/after comparison in the commit log of the functional changes from this patch.
2. Post-commit reviewers can easily parse those diffs inline in email.
3. It's also easier for us to review the test diffs here in Phab in one shot alongside the code changes.

There should be no concern about documenting buggy behavior in a regression test; I do this all the time. Then there's no guesswork about the old codegen. Just add "; FIXME: miscompile" or "; FIXME: missed fold" before tests that you know are misbehaving.

To substantially reduce the size of the patch: remove the "bb:" label from each test and the corresponding CHECK line. These are all single basic-block tests, so there's no value in those lines.


http://reviews.llvm.org/D21775