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