[llvm] r303133 - [InstCombine] add tests for PR32791; NFC

Sanjay Patel via llvm-commits llvm-commits at lists.llvm.org
Tue May 16 09:13:18 PDT 2017


On Mon, May 15, 2017 at 8:03 PM, Davide Italiano <davide at freebsd.org> wrote:

> On Mon, May 15, 2017 at 6:35 PM, Sanjay Patel <spatel at rotateright.com>
> wrote:
> > This isn't "code"; it's test coverage. In any case, I won't be able
> revert
> > this myself until tomorrow morning (mountain time zone). Since you
> obviously
> > object to this commit, can you revert it for me?
> >
> > note that in addition to a potential transform, these tests add coverage
> for
> > a potential infinite loop from conflicting InstCombines. Having
> encountered
> > more than my fair share of those, I'd like to see far more of these
> negative
> > regression tests, so I hit them during development rather than via bug
> > report.
> >
>
> There's no rush reverting this, as it's not breaking things.
> I'm fine with leaving the tests for the infinite loop, but please add
> a comment explaining that.
>


Patch posted for review:
https://reviews.llvm.org/D33242



> I'd appreciate if we can continue the discussion on whether this
> transformation should live on phabricator before we go forward with
> this.
>

Sure - it wasn't my intention to imply that we are adding InstCombine folds
based on these tests, but I can see how that might be assumed. I'll add
more commentary with similar commits. In many cases like this, I have tried
to improve the LLVM regression test directory cultural norms by
using/requesting meaningful names for files and tests and/or adding
comments, but recent commits show that "foo" and "poo" without any
explanation are still accepted.



> --
> Davide
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170516/fa6c97ef/attachment.html>


More information about the llvm-commits mailing list