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

[Sanjay Patel via llvm-commits]

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.


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

> On Mon, May 15, 2017 at 5:48 PM, Philip Reames
> <listmail at philipreames.com> wrote:
> > On 05/15/2017 05:37 PM, Davide Italiano via llvm-commits wrote:
> >>
> >> On Mon, May 15, 2017 at 5:32 PM, Sanjay Patel <spatel at rotateright.com>
> >> wrote:
> >>>
> >>> This is just documenting the current status. If we choose not to
> >>> transform
> >>> these in InstCombine, then we should have a record of that limitation
> >>> that
> >>> we acknowledge is a good thing. This way someone won't come along in
> the
> >>> future and wonder if the transform doesn't exist intentionally or it's
> >>> just
> >>> an oversight. I can add some test comments once we have a conclusion in
> >>> D33172.
> >>>
> >> Putting these tests in `InstCombine` if we decide to do the
> >> transformation somewhere else is just ... odd.
> >> I don't think we should document all the transformations we don't do
> >> otherwise we will end up with a bunch of useless and/or redundant
> >> tests in tree. Please revert this change until we have reached a
> >> consensus.
> >
> > Davide, to me, Sanjay's actions here seem reasonable.  Given his prior
> work,
> > I'm more than wiling to believe that he'll update/remove/move the tests
> as
> > needed based on the conclusion of the review.  I'd vote for leaving
> these in
> > tree given that it makes the process of review easier as the changes
> being
> > made become easy to see.
> >>
>
> I'm fairly sure Sanjay is a diligent developer but this is not my point.
> The standard practice in LLVM is that of not committing code when
> there are pending design discussions. At times, after code has been
> committed if people raise objections we generally try to revert sooner
> rather than later, then start a discussion and iterate from there.
> There's an ongoing discussion on whether this transformation should
> live and I've raised objections on having it in InstCombine, see
> https://reviews.llvm.org/D33172
> So, committing these tests anyway seems a little weird to me.
>
> --
> Davide
>
> "There are no solved problems; there are only problems that are more
> or less solved" -- Henri Poincare
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170516/a0542015/attachment-0001.html>